| Message ID | 1516757059-20279-1-git-send-email-redmcg@redmandi.dyndns.org |
|---|---|
| State | New |
| Headers | show |
> 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
> 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
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
> 在 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 --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);
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(-)