[FFmpeg-devel] [DEVEL][PATCH v3] ffmpeg: fix channel_layout bug on non-default layout

[pkv.stream]

Le 18/11/2017 à 9:26 PM, Michael Niedermayer a écrit :
> On Sat, Nov 18, 2017 at 11:41:54AM +0100, pkv.stream wrote:
>> Hi Michael
>>>>>>   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
>> I have another version of the patch working with MATCH_PER_STREAM_OPT() ;
>> but the changes to code are more important, and it's a bit hacky
>> (defines an internal OptionDef) ... so not sure it is any better
>> than the few lines of patch v3.
>> I've checked that stream specifiers ( :a:0 ) are passed correctly
>> and that two streams with different layouts are also treated
>> correctly (the previous patch without MATCH_PER_STREAM_OPT() works
>> also; those were two points you singled out in your review).
>>   I have no real opinion on the best course, which to pick or even to
>> let the bug hanging (I'm only trying to fix it due to my work on the
>> aac PCE patch of atomnuker ; the bug prevents full use of the new
>> PCE capability).
>> It's Ok with me if you decide to forgo these attempts to fix the bug
>> and let the bug stay.
>> I'm not impacted by the bug in my case use (encode 16 channels in
>> aac); just trying to be thorough in my (akward) contributions and
>> trying to give back to the project.
>> Tell me the best course; or if you see a way to make my
>> MATCH_PER_STREAM_OPT() code less hacky.
> iam sure theres a way to do this less hacky
> why do you need a 2nd table ? or rather why does it not work if you
> put the entry in the main table ? (so there are 2 entries one for
> OPT_SPEC and one for teh callback, will it not send the data to both
> matching entries ?

Hi
it does work in the main table.
I didn't want to pollute it. But if you say it is OK then I'll update 
accordingly and send a separate patch then.
thanks