[FFmpeg-devel] [PATCH 3/4] avformat/hlsenc: use hlsenc_io_* APIs
Karthick Jeyapal
kjeyapal at akamai.com
Tue Dec 19 09:00:42 EET 2017
On 12/19/17 11:32 AM, 刘歧 wrote:
>
>> On 19 Dec 2017, at 12:11, Karthick Jeyapal <kjeyapal at akamai.com> wrote:
>>
>>
>>
>> On 12/19/17 9:29 AM, 刘歧 wrote:
>>>> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeyapal at akamai.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/18/17 2:17 PM, Steven Liu wrote:
>>>>> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
>>>>> ---
>>>>> libavformat/hlsenc.c | 23 ++++++++++++-----------
>>>>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>>>> index 0eebcb4462..0cb75ff198 100644
>>>>> --- a/libavformat/hlsenc.c
>>>>> +++ b/libavformat/hlsenc.c
>>>>> @@ -440,7 +440,7 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
>>>>> av_dict_set(&options, "method", "DELETE", 0);
>>>>> if ((ret = vs->avf->io_open(vs->avf, &out, path, AVIO_FLAG_WRITE, &options)) < 0)
>>>>> goto fail;
>>>>> - ff_format_io_close(vs->avf, &out);
>>>>> + hlsenc_io_close(vs->avf, &out, path);
>>>> Will not actually close, when http_persistent is 1. I think it is better to leave this as ff_format_io_close
>>>>> } else if (unlink(path) < 0) {
>>>>> av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
>>>>> path, strerror(errno));
>>>>> @@ -463,7 +463,7 @@ static int hls_delete_old_segments(AVFormatContext *s, HLSContext *hls,
>>>>> av_free(sub_path);
>>>>> goto fail;
>>>>> }
>>>>> - ff_format_io_close(vs->avf, &out);
>>>>> + hlsenc_io_close(vs->avf, &out, sub_path);
>>>> Will not actually close, when http_persistent is 1.
>>>>> } else if (unlink(sub_path) < 0) {
>>>>> av_log(hls, AV_LOG_ERROR, "failed to delete old segment %s: %s\n",
>>>>> sub_path, strerror(errno));
>>>>> @@ -556,8 +556,10 @@ static int do_encrypt(AVFormatContext *s, VariantStream *vs)
>>>>> }
>>>>> ff_data_to_hex(hls->key_string, key, sizeof(key), 0);
>>>>> - if ((ret = s->io_open(s, &pb, hls->key_file, AVIO_FLAG_WRITE, NULL)) < 0)
>>>>> - return ret;
>>>>> + ret = hlsenc_io_open(s, &pb, hls->key_file, NULL);
>>>> We needn't call hlsenc_io_open if we are not planning to use a persistent connection for it. In this case pb is uninitialized and hlsenc_io_open will most probably cause a crash or undefined behavior. You can get around that issue by initializing pb to NULL. But I think that is unnecessary and are better placed with s->io_open().
>>>>> + if (ret < 0) {
>>>>> + return ret;;
>>>> Extra semicolon
>>>>> + }
>>>>> avio_seek(pb, 0, SEEK_CUR);
>>>>> avio_write(pb, key, KEYSIZE);
>>>>> avio_close(pb);
>>>>> @@ -588,7 +590,7 @@ static int hls_encryption_start(AVFormatContext *s)
>>>>> ff_get_line(pb, hls->iv_string, sizeof(hls->iv_string));
>>>>> hls->iv_string[strcspn(hls->iv_string, "\r\n")] = '\0';
>>>>> - ff_format_io_close(s, &pb);
>>>>> + hlsenc_io_close(s, &pb, hls->key_info_file);
>>>>> if (!*hls->key_uri) {
>>>>> av_log(hls, AV_LOG_ERROR, "no key URI specified in key info file\n");
>>>>> @@ -606,7 +608,7 @@ static int hls_encryption_start(AVFormatContext *s)
>>>>> }
>>>>> ret = avio_read(pb, key, sizeof(key));
>>>>> - ff_format_io_close(s, &pb);
>>>>> + hlsenc_io_close(s, &pb, hls->key_file);
>>>> Will not actually close, when http_persistent is 1.
>>>>> if (ret != sizeof(key)) {
>>>>> av_log(hls, AV_LOG_ERROR, "error reading key file %s\n", hls->key_file);
>>>>> if (ret >= 0 || ret == AVERROR_EOF)
>>>>> @@ -1812,7 +1814,6 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>>> vs->init_range_length = range_length;
>>>>> avio_open_dyn_buf(&oc->pb);
>>>>> vs->packets_written = 0;
>>>>> - ff_format_io_close(s, &vs->out);
>>>>> hlsenc_io_close(s, &vs->out, vs->base_output_dirname);
>>>>> }
>>>>> } else {
>>>>> @@ -1845,7 +1846,7 @@ static int hls_write_packet(AVFormatContext *s, AVPacket *pkt)
>>>>> if (ret < 0) {
>>>>> return ret;
>>>>> }
>>>>> - ff_format_io_close(s, &vs->out);
>>>>> + hlsenc_io_close(s, &vs->out, vs->avf->filename);
>>>>> }
>>>>> ret = hls_append_segment(s, hls, vs, vs->duration, vs->start_pos, vs->size);
>>>>> vs->start_pos = new_start_pos;
>>>>> @@ -1925,14 +1926,14 @@ static int hls_write_trailer(struct AVFormatContext *s)
>>>>> if (ret < 0) {
>>>>> return ret;
>>>>> }
>>>>> - ff_format_io_close(s, &vs->out);
>>>>> + hlsenc_io_close(s, &vs->out, vs->avf->filename);
>>>> Will not actually close, when http_persistent is 1. hls_write_trailer should always call ff_format_io_close()
>>>>> }
>>>>> av_write_trailer(oc);
>>>>> if (oc->pb) {
>>>>> vs->size = avio_tell(vs->avf->pb) - vs->start_pos;
>>>>> if (hls->segment_type != SEGMENT_TYPE_FMP4)
>>>>> - ff_format_io_close(s, &oc->pb);
>>>>> + hlsenc_io_close(s, &oc->pb, oc->filename);
>>>> Will not actually close, when http_persistent is 1. hls_write_trailer should always call ff_format_io_close()
>>>>> if ((hls->flags & HLS_TEMP_FILE) && oc->filename[0]) {
>>>>> hls_rename_temp_file(s, oc);
>>>>> @@ -1948,7 +1949,7 @@ static int hls_write_trailer(struct AVFormatContext *s)
>>>>> if (vtt_oc->pb)
>>>>> av_write_trailer(vtt_oc);
>>>>> vs->size = avio_tell(vs->vtt_avf->pb) - vs->start_pos;
>>>>> - ff_format_io_close(s, &vtt_oc->pb);
>>>>> + hlsenc_io_close(s, &vtt_oc->pb, vtt_oc->filename);
>>>> Will not actually close, when http_persistent is 1. hls_write_trailer should always call ff_format_io_close()
>>> I think, indent code to only one is better than more, so remove the hlsenc_io_* and all use hlsenc_io_*,
>>> I choose use hlsenc_io_* and improve hlsenc_io_*
>> I agree having a common function for io_* is better. But I think in this case trying to make this common func, could make the code for hlsenc_io_close() and its callers a bit more complicated. Maybe I am wrong here. You can try a shot at it.
>> But if you choose that case, then hlsenc_io_close should be first improved to handle the all the use-cases, before pushing this patch. Otherwise this patch will break the existing behavior and cause resource/memory leaks.
> I see you call hlsenc_io_open and hlsenc_io_close in m3u8 file and http_persistent 1, Is that have no resource/memory leaks?
Oops. That's a mistake. Thanks for pointing it out.
I have sent a patch to fix the same.
http://ffmpeg.org/pipermail/ffmpeg-devel/2017-December/222666.html
>> Thanks and regards,
>> Karthick
>>>
>>> Thanks
>>>
>>> Steven
>>>>> }
>>>>> av_freep(&vs->basename);
>>>>> av_freep(&vs->base_output_dirname);
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
More information about the ffmpeg-devel
mailing list