diff mbox

[FFmpeg-devel] avformat/hlsenc: Check ret on avformat_write_header

Message ID 1516757059-20279-1-git-send-email-redmcg@redmandi.dyndns.org
State New
Headers show

Commit Message

Brendan McGrath Jan. 24, 2018, 1:24 a.m. UTC
Encoding currently fails when using hls_ts_options with the fmp4
segment type.

This is due to the fact that avformat_write_header does not process
the passed options when the avformat is already initialized.

When using fmp4, the hls_ts_options are parsed and the avformat
initialized within hls_mux_init.

This patch checks the return of avformat_write_header so that if
it is greater than zero (indicating the avformat is already
initialized) then it does not error.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
I'm not sure if hls_ts_options is supposed to be used with fmp4 as the
description is currently:
hls_ts_options    <string>     E....... set hls mpegts list of options for the container format used for hls

If it is (and the code within hls_mux_init seems to indicate that it is) - then this
patch fixes that (and the next will change the description - perhaps the option should
be renamed too?).

I also considered checking if ret was < 0 and erroring immediately rather than relying
on av_dict_count. But I wasn't sure if that check had been intentionally left out
so the av_dict_free would take place.

Let me know if the preference is for another if statement for ret < 0.

 libavformat/hlsenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Liu Jan. 26, 2018, 3:12 a.m. UTC | #1
> On 24 Jan 2018, at 09:24, Brendan McGrath <redmcg@redmandi.dyndns.org> wrote:
> 
> Encoding currently fails when using hls_ts_options with the fmp4
> segment type.
> 
> This is due to the fact that avformat_write_header does not process
> the passed options when the avformat is already initialized.
> 
> When using fmp4, the hls_ts_options are parsed and the avformat
> initialized within hls_mux_init.
> 
> This patch checks the return of avformat_write_header so that if
> it is greater than zero (indicating the avformat is already
> initialized) then it does not error.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> I'm not sure if hls_ts_options is supposed to be used with fmp4 as the
> description is currently:
> hls_ts_options    <string>     E....... set hls mpegts list of options for the container format used for hls
> 
> If it is (and the code within hls_mux_init seems to indicate that it is) - then this
> patch fixes that (and the next will change the description - perhaps the option should
> be renamed too?).
> 
> I also considered checking if ret was < 0 and erroring immediately rather than relying
> on av_dict_count. But I wasn't sure if that check had been intentionally left out
> so the av_dict_free would take place.
> 
> Let me know if the preference is for another if statement for ret < 0.
> 
> libavformat/hlsenc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 42e437f..d83d3b9 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1878,7 +1878,7 @@ static int hls_write_header(AVFormatContext *s)
> 
>         av_dict_copy(&options, hls->format_options, 0);
>         ret = avformat_write_header(vs->avf, &options);
> -        if (av_dict_count(options)) {
> +        if (ret <= 0 && av_dict_count(options)) {
>             av_log(s, AV_LOG_ERROR, "Some of provided format options in '%s' are not recognized\n", hls->format_options_str);
>             ret = AVERROR(EINVAL);
>             av_dict_free(&options);
> -- 
> 2.7.4
> 
> 

Should be ok


Thanks
Steven
Steven Liu Jan. 26, 2018, 3:30 a.m. UTC | #2
> On 24 Jan 2018, at 09:24, Brendan McGrath <redmcg@redmandi.dyndns.org> wrote:
> 
> Encoding currently fails when using hls_ts_options with the fmp4
> segment type.
> 
> This is due to the fact that avformat_write_header does not process
> the passed options when the avformat is already initialized.
> 
> When using fmp4, the hls_ts_options are parsed and the avformat
> initialized within hls_mux_init.
> 
> This patch checks the return of avformat_write_header so that if
> it is greater than zero (indicating the avformat is already
> initialized) then it does not error.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> I'm not sure if hls_ts_options is supposed to be used with fmp4 as the
> description is currently:
> hls_ts_options    <string>     E....... set hls mpegts list of options for the container format used for hls
> 
> If it is (and the code within hls_mux_init seems to indicate that it is) - then this
> patch fixes that (and the next will change the description - perhaps the option should
> be renamed too?).

maybe fmp4 need not use this option in short time.
> 
> I also considered checking if ret was < 0 and erroring immediately rather than relying
> on av_dict_count. But I wasn't sure if that check had been intentionally left out
> so the av_dict_free would take place.
> 
> Let me know if the preference is for another if statement for ret < 0.
> 
> libavformat/hlsenc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 42e437f..d83d3b9 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -1878,7 +1878,7 @@ static int hls_write_header(AVFormatContext *s)
> 
>         av_dict_copy(&options, hls->format_options, 0);
>         ret = avformat_write_header(vs->avf, &options);
> -        if (av_dict_count(options)) {
> +        if (ret <= 0 && av_dict_count(options)) {
>             av_log(s, AV_LOG_ERROR, "Some of provided format options in '%s' are not recognized\n", hls->format_options_str);
>             ret = AVERROR(EINVAL);
>             av_dict_free(&options);
> -- 
> 2.7.4
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Brendan McGrath Jan. 27, 2018, 11:17 p.m. UTC | #3
On 26/01/18 14:30, 刘歧 wrote:
>
>> On 24 Jan 2018, at 09:24, Brendan McGrath <redmcg@redmandi.dyndns.org> wrote:
>>
>> Encoding currently fails when using hls_ts_options with the fmp4
>> segment type.
>>
>> This is due to the fact that avformat_write_header does not process
>> the passed options when the avformat is already initialized.
>>
>> When using fmp4, the hls_ts_options are parsed and the avformat
>> initialized within hls_mux_init.
>>
>> This patch checks the return of avformat_write_header so that if
>> it is greater than zero (indicating the avformat is already
>> initialized) then it does not error.
>>
>> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
>> ---
>> I'm not sure if hls_ts_options is supposed to be used with fmp4 as the
>> description is currently:
>> hls_ts_options    <string>     E....... set hls mpegts list of options for the container format used for hls
>>
>> If it is (and the code within hls_mux_init seems to indicate that it is) - then this
>> patch fixes that (and the next will change the description - perhaps the option should
>> be renamed too?).
> maybe fmp4 need not use this option in short time.
I guess the options are:
a) Use 'hls_ts_options' for both mpegts and fmp4 (just change the 
description and ignore the fact the name isn't quite right);
b) Create a new option exclusively for fmp4 (say 'hls_fmp4_options' and 
change the code to use this for fmp4); or
c) Create a new option for both mpegts and fmp4 (say 
'hls_container_options' and deprecate 'hls_ts_options' - but allowing 
both for mpegts in the interim)

a) is he easiest to implement - but b) and c) are more user friendly. If 
there's a precedent - we should probably follow that.
>> I also considered checking if ret was < 0 and erroring immediately rather than relying
>> on av_dict_count. But I wasn't sure if that check had been intentionally left out
>> so the av_dict_free would take place.
>>
>> Let me know if the preference is for another if statement for ret < 0.
>>
>> libavformat/hlsenc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 42e437f..d83d3b9 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -1878,7 +1878,7 @@ static int hls_write_header(AVFormatContext *s)
>>
>>          av_dict_copy(&options, hls->format_options, 0);
>>          ret = avformat_write_header(vs->avf, &options);
>> -        if (av_dict_count(options)) {
>> +        if (ret <= 0 && av_dict_count(options)) {
>>              av_log(s, AV_LOG_ERROR, "Some of provided format options in '%s' are not recognized\n", hls->format_options_str);
>>              ret = AVERROR(EINVAL);
>>              av_dict_free(&options);
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> 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 Jan. 28, 2018, 4:07 a.m. UTC | #4
> 在 2018年1月28日,上午7:17,Brendan McGrath <redmcg@redmandi.dyndns.org> 写道:
> 
> On 26/01/18 14:30, 刘歧 wrote:
>> 
>>> On 24 Jan 2018, at 09:24, Brendan McGrath <redmcg@redmandi.dyndns.org> wrote:
>>> 
>>> Encoding currently fails when using hls_ts_options with the fmp4
>>> segment type.
>>> 
>>> This is due to the fact that avformat_write_header does not process
>>> the passed options when the avformat is already initialized.
>>> 
>>> When using fmp4, the hls_ts_options are parsed and the avformat
>>> initialized within hls_mux_init.
>>> 
>>> This patch checks the return of avformat_write_header so that if
>>> it is greater than zero (indicating the avformat is already
>>> initialized) then it does not error.
>>> 
>>> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
>>> ---
>>> I'm not sure if hls_ts_options is supposed to be used with fmp4 as the
>>> description is currently:
>>> hls_ts_options    <string>     E....... set hls mpegts list of options for the container format used for hls
>>> 
>>> If it is (and the code within hls_mux_init seems to indicate that it is) - then this
>>> patch fixes that (and the next will change the description - perhaps the option should
>>> be renamed too?).
>> maybe fmp4 need not use this option in short time.
> I guess the options are:
> a) Use 'hls_ts_options' for both mpegts and fmp4 (just change the description and ignore the fact the name isn't quite right);
> b) Create a new option exclusively for fmp4 (say 'hls_fmp4_options' and change the code to use this for fmp4); or
> c) Create a new option for both mpegts and fmp4 (say 'hls_container_options' and deprecate 'hls_ts_options' - but allowing both for mpegts in the interim)
> 
> a) is he easiest to implement - but b) and c) are more user friendly. If there's a precedent - we should probably follow that.

Yes, your way is absolute right

Thanks

Steven
diff mbox

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 42e437f..d83d3b9 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -1878,7 +1878,7 @@  static int hls_write_header(AVFormatContext *s)
 
         av_dict_copy(&options, hls->format_options, 0);
         ret = avformat_write_header(vs->avf, &options);
-        if (av_dict_count(options)) {
+        if (ret <= 0 && av_dict_count(options)) {
             av_log(s, AV_LOG_ERROR, "Some of provided format options in '%s' are not recognized\n", hls->format_options_str);
             ret = AVERROR(EINVAL);
             av_dict_free(&options);