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

[Michael Niedermayer]

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


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is not what we do, but why we do it that matters.
-------------- 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/20090419/864b7517/attachment.pgp>