| Message ID | 20171218084736.13970-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
On 12/18/17 2:17 PM, Steven Liu wrote: > Signed-off-by: Steven Liu <lq@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() > } > av_freep(&vs->basename); > av_freep(&vs->base_output_dirname);
> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeyapal@akamai.com> wrote: > > > > On 12/18/17 2:17 PM, Steven Liu wrote: >> Signed-off-by: Steven Liu <lq@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_* Thanks Steven >> } >> av_freep(&vs->basename); >> av_freep(&vs->base_output_dirname); > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 12/19/17 9:29 AM, 刘歧 wrote: > >> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeyapal@akamai.com> wrote: >> >> >> >> On 12/18/17 2:17 PM, Steven Liu wrote: >>> Signed-off-by: Steven Liu <lq@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. Thanks and regards, Karthick > > > Thanks > > Steven >>> } >>> av_freep(&vs->basename); >>> av_freep(&vs->base_output_dirname); >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> On 19 Dec 2017, at 12:11, Karthick Jeyapal <kjeyapal@akamai.com> wrote: > > > > On 12/19/17 9:29 AM, 刘歧 wrote: >> >>> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeyapal@akamai.com> wrote: >>> >>> >>> >>> On 12/18/17 2:17 PM, Steven Liu wrote: >>>> Signed-off-by: Steven Liu <lq@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? > > Thanks and regards, > Karthick >> >> >> Thanks >> >> Steven >>>> } >>>> av_freep(&vs->basename); >>>> av_freep(&vs->base_output_dirname); >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 12/19/17 11:32 AM, 刘歧 wrote: > >> On 19 Dec 2017, at 12:11, Karthick Jeyapal <kjeyapal@akamai.com> wrote: >> >> >> >> On 12/19/17 9:29 AM, 刘歧 wrote: >>>> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeyapal@akamai.com> wrote: >>>> >>>> >>>> >>>> On 12/18/17 2:17 PM, Steven Liu wrote: >>>>> Signed-off-by: Steven Liu <lq@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@ffmpeg.org >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
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); } 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); } 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); + if (ret < 0) { + return ret;; + } 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); 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); } 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); 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); } av_freep(&vs->basename); av_freep(&vs->base_output_dirname);
Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavformat/hlsenc.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)