[FFmpeg-cvslog] r18623 - trunk/libavcodec/ac3enc.c

[Justin Ruggles]

Michael Niedermayer wrote:
> On Sun, Apr 19, 2009 at 05:32:49PM -0400, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>> On Sun, Apr 19, 2009 at 03:15:44PM -0400, Justin Ruggles wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Sun, Apr 19, 2009 at 05:06:13PM +0200, jbr wrote:
>>>>>> Author: jbr
>>>>>> Date: Sun Apr 19 17:06:13 2009
>>>>>> New Revision: 18623
>>>>>>
>>>>>> Log:
>>>>>> Add channel layout support to the AC-3 encoder.
>>>>>>
>>>>>> Modified:
>>>>>>    trunk/libavcodec/ac3enc.c
>>>>>>
>>>>>> Modified: trunk/libavcodec/ac3enc.c
>>>>>> ==============================================================================
>>>>>> --- trunk/libavcodec/ac3enc.c	Sun Apr 19 17:05:32 2009	(r18622)
>>>>>> +++ trunk/libavcodec/ac3enc.c	Sun Apr 19 17:06:13 2009	(r18623)
>>>>>> @@ -30,6 +30,7 @@
>>>>>>  #include "get_bits.h" // for ff_reverse
>>>>>>  #include "put_bits.h"
>>>>>>  #include "ac3.h"
>>>>>> +#include "audioconvert.h"
>>>>>>  
>>>>>>  typedef struct AC3EncodeContext {
>>>>>>      PutBitContext pb;
>>>>>> @@ -609,37 +610,67 @@ static int compute_bit_allocation(AC3Enc
>>>>>>      return 0;
>>>>>>  }
>>>>>>  
>>>>>> +static av_cold int set_channel_info(AC3EncodeContext *s, int channels,
>>>>>> +                                    int64_t *channel_layout)
>>>>>> +{
>>>>>> +    int ch_layout;
>>>>>> +
>>>>>> +    if (channels < 1 || channels > AC3_MAX_CHANNELS)
>>>>>> +        return -1;
>>>>>> +    if ((uint64_t)*channel_layout > 0x7FF)
>>>>>> +        return -1;
>>>>>> +    ch_layout = *channel_layout;
>>>>>> +    if (!ch_layout)
>>>>>> +        ch_layout = avcodec_guess_channel_layout(channels, CODEC_ID_AC3, NULL);
>>>>> this is incorrect the way it is used
>>>>> the channel layout must be set on the demuxer/decoder side, one cannot
>>>>> guess the layout from the decoder or the user app by using the encoder
>>>>> CODEC_ID.
>>>> The way it is done currently in ffmpeg.c, the encoder doesn't know the
>>>> values from the decoder (not initialized yet), only the demuxer and the
>>>> commandline.  The demuxer reports the values in the original stream, not
>>>> taking into account any change by the decoder for downmixing based on
>>>> request_channels.  The recent patch I applied to reset channel_layout to
>>>> 0 if it didn't match number of channels was done so that the encoder
>>>> would at least not see the wrong value based on the demuxer and could
>>>> try to guess the best layout based on the number of channels given in
>>>> the commandline.  This is not ideal, but better than having the wrong value.
>>> the encoder MUST NEVER guess the layout (i possibly might not have realized
>>> this if it was discussed previously,dont remember exactly ...)
>> If the layout is set to 0 it is undefined.  The AC3 encoder must either
>> know or guess what the intended output channel layout is in order to set
>> the right channel coding mode, but that is not always known at the time
>> the encoder is initialized.  The previous behavior was to ignore
>> channel_layout completely and always guess...
>>
>>> if layout is undefined the encoder can
>>> A. return -1
>>> B. store a undefined layout if the format permitts that
>>> but it must never guess a layout and pretend that this was what its input
>>> was, this is IMO a serious bug that creates VERY broken files.
>> If I changed the AC3 encoder to always require a channel layout it would
>> hardly ever work because currently very few demuxers/decoders set the
>> layout, and some don't even have a specific layout.  Would we always
>> require user intervention in that case?
> 
> please print at least a warning that the file claims to use a layout that
> may not represent the truth.

That sounds reasonable. I'll add one.

> 
>>>> A better solution might be to swap the order of initialization so that
>>>> decoders are initialized first, then channels and channel_layout are
>>>> copied from the decoder to the encoder before it is initialized.  But
>>>> I'm not sure if there is some specific reason that the encoders are
>>>> currently initialized first.
>>> you talk nonsese, we gather all needed values from the decoders in
>>> av_find_stream_info(), that is for example width&height in mpeg where
>>> its not known to the demuxer (mpeg-ps being an example)
>> >From what I can tell, no decoder is ever opened in av_find_stream_info()
>> if the parser gets the required parameters.  However, the parser cannot
>> know if the decoder will honor request_channels and do mixing internally.
> 
> av_find_stream_info() will open a decoder if the parser does not provide
> the required parameters, channel_layout might not be
> checked yet, patch is welcome

Adding channel_layout to the list of required parameters is fine with
me.  av_find_stream_info() and try_decode_frame() don't follow normal
decoding rules for the return value of avcodec_decode_audio3().  That
can be fixed though.  One result is that the FLAC decoder would fail
here, but that's ok... it would just generate a lot of av_log noise,
which would remind me to hurry up and finish the FLAC parser. :)

> about request_channels, it is 0 unless the user app overrides it, and
> if it does its the user apps problem
> but you could easily refrain from setting channel_layout if request_channels
> is set in the parser.

Yes, that is actually what the AC-3 parser does, but ffmpeg will need to
be changed in order for it to work properly because, from what I can
tell, the codec AVOption parameters (including request_channels) have
not been read from the commandline at that point.  I will see if I can
come up with a patch.

Thanks,
Justin