[FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout
pkv.stream
pkv.stream at gmail.com
Wed Nov 15 00:49:26 EET 2017
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.
Thanks
More information about the ffmpeg-devel
mailing list