| Message ID | 31846c44-6a8b-887a-8cfa-9a4e7fa07302@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 05, 2017 at 12:37:13AM +0200, pkv.stream wrote: > Le 04/10/2017 à 11:18 PM, Moritz Barsnick a écrit : > >On Mon, Oct 02, 2017 at 21:50:50 +0200, pkv.stream wrote: > >> if (!ost->stream_copy) { > >>- char *sample_fmt = NULL; > >>+ > >>+ char *sample_fmt = NULL; > >This is very obviously a patch which will not be accepted. > > > > > >>- MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); > >>+ AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, "channel_layout",NULL, AV_DICT_MATCH_CASE); > >>+ if (output_layout) > >>+ ost->enc_ctx->channel_layout = strtoull(output_layout->value, NULL, 10); > >>+ > >>+ MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); > >Your indentation is totally wrong, and makes use of tabs. Please follow > >the ffmpeg style. > > styling fixed > thanks for your input > > > ffmpeg_opt.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > 2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch > From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 > From: pkviet <pkv.stream@gmail.com> > Date: Mon, 2 Oct 2017 11:14:31 +0200 > Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout > > Fix for ticket 6706. > The -channel_layout option was not working when the channel layout was not > a default one (ex: for 4 channels, quad was interpreted as 4.0 which is > the default layout for 4 channels; or octagonal interpreted as 7.1). > This led to the spurious auto-insertion of an auto-resampler filter > remapping the channels even if input and output had identical channel > layouts. > The fix operates by directly calling the channel layout if defined in > options. If the layout is undefined, the default layout is selected as > before the fix. > --- > ffmpeg_opt.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c > index 100fa76..cf5a63c 100644 > --- a/ffmpeg_opt.c > +++ b/ffmpeg_opt.c > @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in > > MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); > > + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, > + "channel_layout", > + NULL, AV_DICT_MATCH_CASE); This doesnt look right not an issue of the patch as such but why is the channel_layout option per file and not per stream? also currently mixed statemts and declarations arent allowed > + if (output_layout) > + ost->enc_ctx->channel_layout = strtoull(output_layout->value, NULL, 10); audio_enc > + > MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); > if (sample_fmt && > (audio_enc->sample_fmt = av_get_sample_fmt(sample_fmt)) == AV_SAMPLE_FMT_NONE) { > @@ -2524,7 +2530,10 @@ loop_end: > (count + 1) * sizeof(*f->sample_rates)); > } > if (ost->enc_ctx->channels) { > - f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); > + if (ost->enc_ctx->channel_layout) > + f->channel_layout = ost->enc_ctx->channel_layout; > + else > + f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); if () { } else is better as tat way future patches do not need to change existing lines maing patches more readable if ever a line is added [...]
Hi Michael, >> ffmpeg_opt.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> 2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch >> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 >> From: pkviet <pkv.stream@gmail.com> >> Date: Mon, 2 Oct 2017 11:14:31 +0200 >> Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout >> >> Fix for ticket 6706. >> The -channel_layout option was not working when the channel layout was not >> a default one (ex: for 4 channels, quad was interpreted as 4.0 which is >> the default layout for 4 channels; or octagonal interpreted as 7.1). >> This led to the spurious auto-insertion of an auto-resampler filter >> remapping the channels even if input and output had identical channel >> layouts. >> The fix operates by directly calling the channel layout if defined in >> options. If the layout is undefined, the default layout is selected as >> before the fix. >> --- >> ffmpeg_opt.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c >> index 100fa76..cf5a63c 100644 >> --- a/ffmpeg_opt.c >> +++ b/ffmpeg_opt.c >> @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in >> >> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); >> >> + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, >> + "channel_layout", >> + NULL, AV_DICT_MATCH_CASE); > This doesnt look right > > not an issue of the patch as such but > why is the channel_layout option per file and not per stream? just my ignorance; do you mean this is not the right way to retrieve the channel_layout option from an audio stream ? Could you give me a hint on how to retrieve correctly the option ? the goal is to get rid of : l. 2526 in ffmpeg_opt.c : f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); from commit https://github.com/FFmpeg/FFmpeg/commit/198e8b8e774659eacaa7058c7f5704029af5bbbf#diff-3662f9bbf5b9e46a224a4e65b17254ba (merge from libav) which is causing the auto-insert of a filter which rematrix a non default channel layout into a default one (ex: octagonal to 7.1). > > also currently mixed statemts and declarations arent allowed > yes sorry; I've learned that from the ongoing discussion on the 'for int loop' and will fix if I am able to find the right way to solve the ticket >> + if (output_layout) >> + ost->enc_ctx->channel_layout = strtoull(output_layout->value, NULL, 10); > audio_enc > ok >> + >> MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); >> if (sample_fmt && >> (audio_enc->sample_fmt = av_get_sample_fmt(sample_fmt)) == AV_SAMPLE_FMT_NONE) { >> @@ -2524,7 +2530,10 @@ loop_end: >> (count + 1) * sizeof(*f->sample_rates)); >> } >> if (ost->enc_ctx->channels) { >> - f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); >> + if (ost->enc_ctx->channel_layout) >> + f->channel_layout = ost->enc_ctx->channel_layout; >> + else >> + f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); > if () { > } else > > is better as tat way future patches do not need to change existing > lines maing patches more readable if ever a line is added sure, thanks for the explanation ; will fix > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote: > Hi Michael, > > >> ffmpeg_opt.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >>2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch > >> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 > >>From: pkviet <pkv.stream@gmail.com> > >>Date: Mon, 2 Oct 2017 11:14:31 +0200 > >>Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout > >> > >>Fix for ticket 6706. > >>The -channel_layout option was not working when the channel layout was not > >>a default one (ex: for 4 channels, quad was interpreted as 4.0 which is > >>the default layout for 4 channels; or octagonal interpreted as 7.1). > >>This led to the spurious auto-insertion of an auto-resampler filter > >>remapping the channels even if input and output had identical channel > >>layouts. > >>The fix operates by directly calling the channel layout if defined in > >>options. If the layout is undefined, the default layout is selected as > >>before the fix. > >>--- > >> ffmpeg_opt.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >> > >>diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c > >>index 100fa76..cf5a63c 100644 > >>--- a/ffmpeg_opt.c > >>+++ b/ffmpeg_opt.c > >>@@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in > >> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); > >>+ AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, > >>+ "channel_layout", > >>+ NULL, AV_DICT_MATCH_CASE); > >This doesnt look right > > > >not an issue of the patch as such but > >why is the channel_layout option per file and not per stream? > > > just my ignorance; do you mean this is not the right way to retrieve > the channel_layout option from an audio stream ? I think there is more buggy with how the channel_layout is handled try this: ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav and this: ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav while it may appear that the are both working this is deceiving. I think only the channel number gets actually used in the 2nd case Look at what your code with av_dict_get() reads. It does not obtain the 5 in the 2nd case [...]
Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit : > On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote: >> Hi Michael, >> >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> 2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch >>>> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 >>>> From: pkviet <pkv.stream@gmail.com> >>>> Date: Mon, 2 Oct 2017 11:14:31 +0200 >>>> Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout >>>> >>>> Fix for ticket 6706. >>>> The -channel_layout option was not working when the channel layout was not >>>> a default one (ex: for 4 channels, quad was interpreted as 4.0 which is >>>> the default layout for 4 channels; or octagonal interpreted as 7.1). >>>> This led to the spurious auto-insertion of an auto-resampler filter >>>> remapping the channels even if input and output had identical channel >>>> layouts. >>>> The fix operates by directly calling the channel layout if defined in >>>> options. If the layout is undefined, the default layout is selected as >>>> before the fix. >>>> --- >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c >>>> index 100fa76..cf5a63c 100644 >>>> --- a/ffmpeg_opt.c >>>> +++ b/ffmpeg_opt.c >>>> @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in >>>> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); >>>> + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, >>>> + "channel_layout", >>>> + NULL, AV_DICT_MATCH_CASE); >>> This doesnt look right >>> >>> not an issue of the patch as such but >>> why is the channel_layout option per file and not per stream? >> >> just my ignorance; do you mean this is not the right way to retrieve >> the channel_layout option from an audio stream ? > I think there is more buggy with how the channel_layout is handled > > try this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav > and this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav > > while it may appear that the are both working this is deceiving. > I think only the channel number gets actually used in the 2nd case > > Look at what your code with av_dict_get() reads. > It does not obtain the 5 in the 2nd case Hi Michael, Thanks for the explanation; actually I have the impression that it's worse than that. In the second case indeed av_dict_get() doesn't get the 5 (was not aware of channel_layout:a syntax , is it documented somewhere ?) In the first case, the 5 is recognized but the auto resampler spouts errors : ffmpeg -loglevel debug -channel_layout octagonal -i test.mkv -channel_layout 5 test.wav ... Successfully opened the file. Parsing a group of options: output url test.wav. Applying option channel_layout (set channel layout) with argument 5. Successfully parsed a group of options. Opening an output file: test.wav. [file @ 0000018F66CF5C20] Setting default whitelist 'file,crypto' Successfully opened the file. Stream mapping: Stream #0:1 -> #0:0 (pcm_s24le (native) -> pcm_s16le (native)) Press [q] to stop, [?] for help cur_dts is invalid (this is harmless if it occurs once at the start per stream) Last message repeated 3 times detected 8 logical cores [graph_0_in_0_1 @ 0000018F66D05760] Setting 'time_base' to value '1/48000' [graph_0_in_0_1 @ 0000018F66D05760] Setting 'sample_rate' to value '48000' [graph_0_in_0_1 @ 0000018F66D05760] Setting 'sample_fmt' to value 's32' [graph_0_in_0_1 @ 0000018F66D05760] Setting 'channel_layout' to value '0x737' [graph_0_in_0_1 @ 0000018F66D05760] tb:1/48000 samplefmt:s32 samplerate:48000 chlayout:0x737 [format_out_0_0 @ 0000018F687E0C60] Setting 'sample_fmts' to value 's16' [format_out_0_0 @ 0000018F687E0C60] Setting 'channel_layouts' to value '0x5' [format_out_0_0 @ 0000018F687E0C60] auto-inserting filter 'auto_resampler_0' between the filter 'Parsed_anull_0' and the filter 'format_out_0_0' [AVFilterGraph @ 0000018F66D48DE0] query_formats: 4 queried, 6 merged, 3 already done, 0 delayed [auto_resampler_0 @ 0000018F687E0EC0] [SWR @ 0000018F66D2CE80] Using fltp internally between filters [auto_resampler_0 @ 0000018F687E0EC0] [SWR @ 0000018F66D2CE80] Output channel layout '2 channels (FL+FC)' is not supported [auto_resampler_0 @ 0000018F687E0EC0] Failed to configure output pad on auto_resampler_0 Error reinitializing filters! Failed to inject frame into filter network: Invalid argument Error while processing the decoded data for stream #0:1 Not sure why swr_build_matrix is issuing an error. Is this a case of a bug hiding another bug ? namely (1) wrongly matrixing to default channel_layout instead of real one , (2) auto-inserted filter which seems only able to deal with non default layout ( that is to say, anything other than mono stereo 2.1 4.0 5.0 5.1 7.1 hexadecagonal). If you have ideas to suggest for solving this mess, I'll try to work some more on this but this bug is growing too big for me, so without input, I'll leave it to senior devs. Regards > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit : > On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote: >> Hi Michael, >> >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> 2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch >>>> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 >>>> From: pkviet <pkv.stream@gmail.com> >>>> Date: Mon, 2 Oct 2017 11:14:31 +0200 >>>> Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout >>>> >>>> Fix for ticket 6706. >>>> The -channel_layout option was not working when the channel layout was not >>>> a default one (ex: for 4 channels, quad was interpreted as 4.0 which is >>>> the default layout for 4 channels; or octagonal interpreted as 7.1). >>>> This led to the spurious auto-insertion of an auto-resampler filter >>>> remapping the channels even if input and output had identical channel >>>> layouts. >>>> The fix operates by directly calling the channel layout if defined in >>>> options. If the layout is undefined, the default layout is selected as >>>> before the fix. >>>> --- >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c >>>> index 100fa76..cf5a63c 100644 >>>> --- a/ffmpeg_opt.c >>>> +++ b/ffmpeg_opt.c >>>> @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in >>>> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); >>>> + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, >>>> + "channel_layout", >>>> + NULL, AV_DICT_MATCH_CASE); >>> This doesnt look right >>> >>> not an issue of the patch as such but >>> why is the channel_layout option per file and not per stream? >> >> just my ignorance; do you mean this is not the right way to retrieve >> the channel_layout option from an audio stream ? > I think there is more buggy with how the channel_layout is handled > > try this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav > and this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav > > while it may appear that the are both working this is deceiving. > I think only the channel number gets actually used in the 2nd case > > Look at what your code with av_dict_get() reads. > It does not obtain the 5 in the 2nd case > > [...] > on second thought, for the first of your commands with my av_dict_get() patch, it's expected behaviour when downmixing that an error is issued if a non standard layout is used; swr_build_matrix can't possibly know how to downmix for layout 0x5. So end-users should use a pan filter. in order for the second command to be passed correctly, I haven't found yet a solution > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Le 10/11/2017 à 1:12 AM, Michael Niedermayer a écrit : > On Thu, Nov 09, 2017 at 09:37:33PM +0100, pkv.stream wrote: >> Hi Michael, >> >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> 2af07f4366efdfaf1018bb2ea29be672befe0823 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch >>>> From 4ec55dc88923108132307b41300a1abddf32e6f7 Mon Sep 17 00:00:00 2001 >>>> From: pkviet <pkv.stream@gmail.com> >>>> Date: Mon, 2 Oct 2017 11:14:31 +0200 >>>> Subject: [PATCH] ffmpeg: fix channel_layout bug on non-default layout >>>> >>>> Fix for ticket 6706. >>>> The -channel_layout option was not working when the channel layout was not >>>> a default one (ex: for 4 channels, quad was interpreted as 4.0 which is >>>> the default layout for 4 channels; or octagonal interpreted as 7.1). >>>> This led to the spurious auto-insertion of an auto-resampler filter >>>> remapping the channels even if input and output had identical channel >>>> layouts. >>>> The fix operates by directly calling the channel layout if defined in >>>> options. If the layout is undefined, the default layout is selected as >>>> before the fix. >>>> --- >>>> ffmpeg_opt.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c >>>> index 100fa76..cf5a63c 100644 >>>> --- a/ffmpeg_opt.c >>>> +++ b/ffmpeg_opt.c >>>> @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in >>>> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); >>>> + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, >>>> + "channel_layout", >>>> + NULL, AV_DICT_MATCH_CASE); >>> This doesnt look right >>> >>> not an issue of the patch as such but >>> why is the channel_layout option per file and not per stream? >> >> just my ignorance; do you mean this is not the right way to retrieve >> the channel_layout option from an audio stream ? > I think there is more buggy with how the channel_layout is handled > > try this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout 5 test.wav > and this: > ./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -channel_layout:a 5 test.wav > > while it may appear that the are both working this is deceiving. > I think only the channel number gets actually used in the 2nd case > > Look at what your code with av_dict_get() reads. > It does not obtain the 5 in the 2nd case > > [...] > ok finally got what you meant with the second case; I had not realized that stream specifiers could be applied also to -channel_layout option. thanks > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c index 100fa76..cf5a63c 100644 --- a/ffmpeg_opt.c +++ b/ffmpeg_opt.c @@ -1804,6 +1804,12 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st); + AVDictionaryEntry *output_layout = av_dict_get(o->g->codec_opts, + "channel_layout", + NULL, AV_DICT_MATCH_CASE); + if (output_layout) + ost->enc_ctx->channel_layout = strtoull(output_layout->value, NULL, 10); + MATCH_PER_STREAM_OPT(sample_fmts, str, sample_fmt, oc, st); if (sample_fmt && (audio_enc->sample_fmt = av_get_sample_fmt(sample_fmt)) == AV_SAMPLE_FMT_NONE) { @@ -2524,7 +2530,10 @@ loop_end: (count + 1) * sizeof(*f->sample_rates)); } if (ost->enc_ctx->channels) { - f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); + if (ost->enc_ctx->channel_layout) + f->channel_layout = ost->enc_ctx->channel_layout; + else + f->channel_layout = av_get_default_channel_layout(ost->enc_ctx->channels); } else if (ost->enc->channel_layouts) { count = 0; while (ost->enc->channel_layouts[count])