[FFmpeg-devel] Correct libvorbis.c behaviour with >2 channels

[James Darnley]

On 2 June 2010 11:03, Robert Swain <robert.swain at gmail.com> wrote:
> On 2 June 2010 09:19, Martin Storsj? <martin at martin.st> wrote:
>> On Wed, 2 Jun 2010, Michael Niedermayer wrote:
>>
>>> On Fri, May 28, 2010 at 07:57:54PM +0200, James Darnley wrote:
>>> > I fail hard at sending out correct patches. ?New one, git format-patch
>>>
>>> > From 1325bfbc5240cec90937f3d26f2663073050ccda Mon Sep 17 00:00:00 2001
>>> > From: James Darnley <james.darnley at gmail.com>
>>> > Date: Fri, 28 May 2010 14:36:53 +0200
>>> > Subject: [PATCH 2/3] Fix libvorbis encoding with more than 2 channels
>>> >
>>> > ---
>>> > ?libavcodec/Makefile ? ?| ? ?2 +-
>>> > ?libavcodec/libvorbis.c | ? 12 +++++-------
>>> > ?2 files changed, 6 insertions(+), 8 deletions(-)
>>>
>>> if the bug and fix are confirmed then this should be ok
>>
>> The actual bug with more than one channel is obviously fixed by this, but
>> this also remaps the channels, and I'm not really sure if this is the
>> correct way.
>>
>> Robert, Alex, or anyone else that are familiar with mapping of channels in
>> these kinds of codecs - care to give a comment on this?
>
> I don't think this is correct. What you're trying to do is map from
> wave format extensible order to Vorbis order. The first few channel
> layouts match, coincidentally. But, when you get to 5.1 and beyond
> there are problems. For 5.1 the input wave format extensible order
> will be:
>
> FL FR FC LFE RL RR
>
> If you use the ff_vorbis_channel_layout_offsets table as you are:
>
> { 0, 2, 1, 5, 3, 4, },
>
> which gives:
>
> FL FC FR RR LFE RL
>
> and what we actually want (Vorbis order for 5.1):
>
> FL FC FR RL RR LFE
>
> You don't need to re-apply the mapping but rather apply the inverse which is:
>
> { 0, 2, 1, 4, 5, 3, },
>
> This can be constructed from the decoder mapping table by noting the
> position of a channel number in the _offsets array[1]. Or we could
> just have a table for encoding.
>

Where would you like me to put a new table with the correct input
mapping?  Is vorbis_data.c, next to the existing _offset array okay?
If the vorbis encoder ever supports more than 2 channels this would be
a logical place to have it.

I didn't notice the incorrect mapping because I only have stereo headphones.