[FFmpeg-devel] [PATCH] ffmpeg: guess channel layout from number of channels if needed

[Mina Nagy Zaki]

On Sat, Aug 20, 2011 at 08:08:52PM +0200, Stefano Sabatini wrote:
> On date Friday 2011-08-19 02:22:36 +0300, Mina Nagy Zaki encoded:
> > Guess channel layout from number of channels instead of setting the encoder's
> > channel_layout to 0
> > 
> > This breaks a number of acodec-pcm regtests due to wav headers now containing a
> > channel layout mask instead of 0x0
> > ---
> >  ffmpeg.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ffmpeg.c b/ffmpeg.c
> > index 2c66076..e5ee33b 100644
> > --- a/ffmpeg.c
> > +++ b/ffmpeg.c
> > @@ -2313,10 +2313,12 @@ static int transcode(AVFormatContext **output_files,
> >                  choose_sample_fmt(ost->st, ost->enc);
> >                  if (!codec->channels) {
> >                      codec->channels = icodec->channels;
> > -                    codec->channel_layout = icodec->channel_layout;
> > +                    if (icodec->channel_layout)
> > +                        codec->channel_layout = icodec->channel_layout;
> > +                    else
> > +                        codec->channel_layout =
> > +                            avcodec_guess_channel_layout(codec->channels, 0, NULL);
> 
> nit:
> 
> codec->channel_layout = icodec->channel_layout ? icodec->channel_layout :
>                         avcodec_guess_channel_layout(codec->channels, 0, NULL);
> 
> simpler to read
> 
> But this logic looks busted.
> 
> For example if you set the output channel layout but not the number of
> channels, it will override the specified chlayout value by setting it
> to the input channel layout, which is not the expected behavior.
> 
> In that case the output number of channels should be guessed from the
> layout, and should not derived from the input chlayout.
> 
> So I'd keep channels_nb/channel layout setting logic separated.
> 
> The tricky part is that chlayout and number of channels are
> dependent on each other (so a consistency check should be added).
> 

True. I can't seem to find which options allows setting a layout explicitly
instead of just the number of channels (-ac). If specifying a layout is allowed
then what about specifying conflicting layout and number of channels? Shouldn't
that be checked during option parsing? It seems redundant allowing both number
and layout setting though.

> >                  }
> > -                if (av_get_channel_layout_nb_channels(codec->channel_layout) != codec->channels)
> > -                    codec->channel_layout = 0;
> >                  ost->audio_resample = codec->sample_rate != icodec->sample_rate || audio_sync_method > 1;
> >                  icodec->request_channels = codec->channels;
> >                  ist->decoding_needed = 1;
> 
> Which regtests are changed?
> 
> For fixing regtests, you simply edit the files in test/ref/ with the
> resulting new output files.

The acodec-pcm tests pcm_s24be pcm_s32be pcm_f32be pcm_f64be pcm_zork