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

[Michael Niedermayer]

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.


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

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.


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20090420/57d77b5c/attachment.pgp>