[FFmpeg-devel] [PATCH]Make CH_LAYOUT_5POINTx_BACK the normal case

[Stefano Sabatini]

On date Thursday 2011-08-25 09:20:08 +0000, Carl Eugen Hoyos encoded:
> Stefano Sabatini <stefano.sabatini-lala <at> poste.it> writes:
> 
> > > AV_CH_LAYOUT_5POINTx_BACK is what every other application expects for
> > > 5.0/5.1 when reading a wav file which is where FFmpeg's channel layout comes
> > > from, so this should be the "normal" case, and AV_CH_LAYOUT_5POINTx the
> > > exception.
> > 
> > How is this related to the channel_layout_map array?
> 
> How is it not related to it?

I see channel_layout_map is used for mapping strings <-> channel
layouts, so the only difference I'd expect by this change is that now
the mapping is changed. In other words the string is only used for
describing the channel layout to *users*, so it is an UI change.

Or no?

> You suggested that we should not call two different channel layouts (one of them
> very common - most common after stereo, I suppose - one them invalid, to use a
> hard word) with the same name ("5.x"). You send a patch and I agreed before
> reading the source again.

> We should not give the "invalid" layout the short name, but the common layout
> that other applications (including the relevant one for wav) expect.
> 
> ...
> 
> > > -    { "5.0",         5,  AV_CH_LAYOUT_5POINT0 },
> > > -    { "5.0(back)",   5,  AV_CH_LAYOUT_5POINT0_BACK },
> > > -    { "5.1",         6,  AV_CH_LAYOUT_5POINT1 },
> > > -    { "5.1(back)",   6,  AV_CH_LAYOUT_5POINT1_BACK },
> > 
> > > +    { "5.0(side)",   5,  AV_CH_LAYOUT_5POINT0 },
> > > +    { "5.0",         5,  AV_CH_LAYOUT_5POINT0_BACK },
> > 
> > now this is totally confusing,
> 

> Why is "5.x"/"5.x(side)" (with "5.x" being what users want) more confusing than
> "5.x"/"5.x(back)" (with "5.x(back)" being the intended case). Sorry, but I
> honestly don't understand.

Rule of least surprise, try to create strict one-to-one relations
between related symbols, rather than ask the reader/programmer to
know/keep in mind that "5.0" means AV_CH_LAYOUT_5POINT0_BACK and
"5.0(side)" means AV_CH_LAYOUT_5POINT0.

Programmers brainloops are better spent fixing real problems rather
than creating concepts/words mappings here and there. If the code is
clear, spotting the problem is easy, if the code is obfuscated the
programmer needs to go through all the low-level drudgery of silly
unnecessary details and loses her focus.
 
> > what about:
> > 
> > { "5.0(side)",   5,  AV_CH_LAYOUT_5POINT0_SIDE },
> > { "5.0(back)",   5,  AV_CH_LAYOUT_5POINT0_BACK },
> 
> How do users know which is the one they want?
> 
> > you can add a new macro AV_CH_LAYOUT_5POINT0_SIDE in place of 
> > AV_CH_LAYOUT_5POINT0 (you can keep the old one and remove it at the
> > next bump).
> 
> I don't disagree but this is completely unrelated (we had the not completely
> intuitive API for long, and I am in principle against changing API if not
> needed, so I believe we can leave it, but we should not introduce an UI that
> misleads the user that is why the posted patch is important.)

What I'm suggesting is:

#define AV_CH_LAYOUT_5POINT0_SIDE <- same definition as AV_CH_LAYOUT_5POINT0
...

#if lavu < NEXT
// deprecated channel layout definitions, the _SIDE variant should be used
// instead, to avoid confusion with the AV_CH_LAYOUT_5POINT0_BACK macros
#define AV_CH_LAYOUT_5POINT0 ...
#define AV_CH_LAYOUT_5POINT1 ...
#define AV_CH_LAYOUT_7POINT0 ...
#endif

then you should also be able to create a shorter "5.0" alias for
"5.0(back)" pointing to AV_CH_LAYOUT_5POINT0_BACK. Since the
descriptive string was intended to be human readable, changing it
should cause no regressions.

Do it with more than one patch if you want, but please do it.
-- 
FFmpeg = Furious & Funny Majestic Ponderous Ecumenical Gangster