[FFmpeg-devel,3/4] avformat/hlsenc: use hlsenc_io_* APIs

Submitted by Steven Liu on Dec. 18, 2017, 8:47 a.m.

Details

Message ID 20171218084736.13970-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu Dec. 18, 2017, 8:47 a.m.
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

kjeyapal@akamai.com Dec. 19, 2017, 3:55 a.m.
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);
Steven Liu Dec. 19, 2017, 3:59 a.m.
> 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
kjeyapal@akamai.com Dec. 19, 2017, 4:11 a.m.
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
Steven Liu Dec. 19, 2017, 6:02 a.m.
> 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
kjeyapal@akamai.com Dec. 19, 2017, 7 a.m.
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
>
>

Patch hide | download patch | download mbox

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);