> On 19 Dec 2017, at 12:11, Karthick Jeyapal <kjeya...@akamai.com> wrote:
> 
> 
> 
> On 12/19/17 9:29 AM, 刘歧 wrote:
>> 
>>> On 19 Dec 2017, at 11:55, Karthick Jeyapal <kjeya...@akamai.com> wrote:
>>> 
>>> 
>>> 
>>> On 12/18/17 2:17 PM, Steven Liu wrote:
>>>> Signed-off-by: Steven Liu <l...@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



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to