[FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout
Michael Niedermayer
michael at niedermayer.cc
Thu Nov 16 03:48:47 EET 2017
On Tue, Nov 14, 2017 at 11:49:26PM +0100, pkv.stream wrote:
> Le 14/11/2017 à 1:13 PM, Michael Niedermayer a écrit :
> >On Sun, Nov 12, 2017 at 06:26:18PM +0100, pkv.stream wrote:
> >>Le 12/11/2017 à 5:38 PM, Michael Niedermayer a écrit :
> >>>On Sun, Nov 12, 2017 at 05:07:04PM +0100, Kv Pham wrote:
> >>>>Le 12 nov. 2017 5:01 PM, "Michael Niedermayer"<michael at niedermayer.cc> a
> >>>>écrit :
> >>>>
> >>>>On Sat, Nov 11, 2017 at 02:15:25AM +0100, pkv.stream wrote:
> >>>>>Le 11/11/2017 à 1:07 AM, Michael Niedermayer a écrit :
> >>>>>>On Fri, Nov 10, 2017 at 10:27:48PM +0100, pkv.stream wrote:
> >>>>>>>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 at 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 I fixed that by using the flag AV_DICT_IGNORE_SUFFIX ; now
> >>>>>>>-channel_layout:a works as expected.
> >>>>>>>I fixed also all the styling issues you spotted (mixed declarations,
> >>>>>>>{} for if etc).
> >>>>>>>
> >>>>>>>Still not understanding your initial comment though :
> >>>>>>>'why is the channel_layout option per file and not per stream?'
> >>>>>>>
> >>>>>>>do you mean o->g->codec_opts is not where I should retrieve the
> >>>>>>>channel_layout ? if not where from ?
> >>>>>>>
> >>>>>>>Thanks
> >>>>>>>
> >>>>>>>>[...]
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>_______________________________________________
> >>>>>>>>ffmpeg-devel mailing list
> >>>>>>>>ffmpeg-devel at ffmpeg.org
> >>>>>>>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>>>> ffmpeg_opt.c | 12 ++++++++++--
> >>>>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>>>>849898d28e989ffa5cc598c6fe4d43847b636132 0001-ffmpeg-fix-channel_
> >>>>layout-bug-on-non-default-layout.patch
> >>>>>>> From 6d4f1c14135f9473b77e1c649d0e7bbaeba00a50 Mon Sep 17 00:00:00 2001
> >>>>>>>From: pkviet<pkv.stream at 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.
> >>>>>>>---
> >>>>>>> fftools/ffmpeg_opt.c | 12 ++++++++++--
> >>>>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>>diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> >>>>>>>index ca6f10d..cb25d7b 100644
> >>>>>>>--- a/fftools/ffmpeg_opt.c
> >>>>>>>+++ b/fftools/ffmpeg_opt.c
> >>>>>>>@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext
> >>>>*o, AVFormatContext *oc, in
> >>>>>>> AVStream *st;
> >>>>>>> OutputStream *ost;
> >>>>>>> AVCodecContext *audio_enc;
> >>>>>>>+ AVDictionaryEntry *output_layout;
> >>>>>>> ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
> >>>>>>> st = ost->st;
> >>>>>>>@@ -1799,7 +1800,10 @@ static OutputStream *new_audio_stream(OptionsContext
> >>>>*o, AVFormatContext *oc, in
> >>>>>>> char *sample_fmt = NULL;
> >>>>>>> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels,
> >>>>oc, st);
> >>>>>>>-
> >>>>>>>+ output_layout = av_dict_get(o->g->codec_opts,"channel_layout",
> >>>>NULL, AV_DICT_IGNORE_SUFFIX);
> >>>>>>i didnt try but i assume
> >>>>>>this would fail if there are 2 audio streams each with different
> >>>>>>layout
> >>>>>ah ok, I understand now; thanks for elaborating.
> >>>>>i tested with two output streams, it's not failing and working correctly.
> >>>>>
> >>>>>ex: ffmpeg -channel_layout octagonal -i source.wav -c:a pcm_s16le
> >>>>>-channel_layout octagonal out1.wav -c:a pcm_s16le -channel_layout
> >>>>>quad out2.wav
> >>>>>
> >>>>>(if I pick layout 0x5 as in your examples it fails because the
> >>>>>rematrix is not supported, so this is expected behavior although
> >>>>>failing; but with pan filter it eventually works)
> >>>>>
> >>>>>Tested also with two source streams used for two ouputs. Working too.
> >>>>have you tried with different layouts for each stream ?
> >>>>
> >>>>
> >>>>Yes.
> >>>>Different layouts for input streams as well as output streams.
> >>>>
> >>>>Other tests to do ?
> >>>just test with 2 streams:
> >>>
> >>>./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -map 0:a -map 0:a -channel_layout:a:0 quad -channel_layout:a:1 5.1 -y test.nut
> >>>
> >>>av_dict_get() will likely return the same layout twice instead of the
> >>>correct layout for each stream
> >>>
> >>>[...]
> >>>
> >>ok should be fixed with this new patch version.
> >>Changed
> >>
> >>o->g->codec_opts with ost->encoder_opts
> >>
> >>Now it's working correctly with two streams.
> >>Regards
> >>
> >>
> >>>_______________________________________________
> >>>ffmpeg-devel mailing list
> >>>ffmpeg-devel at ffmpeg.org
> >>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> ffmpeg_opt.c | 12 ++++++++++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>a7c71fb1ccd7d91b61033be70dfd324b4e3f3c34 0001-ffmpeg-fix-channel_layout-bug-on-non-default-layout.patch
> >> From fb7f7f6e01cc242e13d0e640f583dffe6e7a8ada Mon Sep 17 00:00:00 2001
> >>From: pkviet<pkv.stream at 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.
> >>---
> >> fftools/ffmpeg_opt.c | 12 ++++++++++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> >>index ca6f10d..8941d66 100644
> >>--- a/fftools/ffmpeg_opt.c
> >>+++ b/fftools/ffmpeg_opt.c
> >>@@ -1785,6 +1785,7 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in
> >> AVStream *st;
> >> OutputStream *ost;
> >> AVCodecContext *audio_enc;
> >>+ AVDictionaryEntry *output_layout;
> >> ost = new_output_stream(o, oc, AVMEDIA_TYPE_AUDIO, source_index);
> >> st = ost->st;
> >>@@ -1799,7 +1800,10 @@ static OutputStream *new_audio_stream(OptionsContext *o, AVFormatContext *oc, in
> >> char *sample_fmt = NULL;
> >> MATCH_PER_STREAM_OPT(audio_channels, i, audio_enc->channels, oc, st);
> >>-
> >>+ output_layout = av_dict_get(ost->encoder_opts,"channel_layout", NULL, AV_DICT_IGNORE_SUFFIX);
> >>+ if (output_layout) {
> >>+ audio_enc->channel_layout = strtoull(output_layout->value, NULL, 10);
> >>+ }
> >why is this handled different from audio_channels ?
> >that is why is this not using MATCH_PER_STREAM_OPT() ?
> >(yes this would require some changes but i wonder why this would be
> > handled differently)
> Hi
> I did try to use the MATCH_PER_STREAM_OPT() but didn't manage to
> have it working. Also I was a bit hesitant to modify the
> OptionsContext struct, and preferred something minimal.
> If you think this can definitely be made to work without too much
> coding and prefer such a solution, I'll retry working on a
> MATCH_PER_STREAM_OPT() solution.
i dont really know if it "can definitely be made to work without too much
coding", it just seemed odd how this is handled differently
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171116/b76eee62/attachment.sig>
More information about the ffmpeg-devel
mailing list