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

[Justin Ruggles]

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?

>> 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)