[FFmpeg-devel] [PATCH] Ignore bad mapping in AAC (issue 1882)

[Robert Swain]

On 29 May 2010 11:18, Cyril Russo <stage.nexvision at laposte.net> wrote:
> Le 29/05/2010 09:21, Robert Swain a ?crit :
>> On 28 May 2010 21:54, Alex Converse<alex.converse at gmail.com> ?wrote:
>>> On Fri, May 28, 2010 at 2:51 AM, Cyril
>>> Russo<stage.nexvision at laposte.net>wrote:
>>>> Le 27/05/2010 21:20, Alex Converse a ?crit :
>>>> ?On Thu, May 27, 2010 at 2:28 PM, Cyril
>>>> Russo<stage.nexvision at laposte.net
>>>>>> wrote:
>>>>>> ?The attached patch allows aac decoder to continue on a badly mapped
>>>>>> AAC
>>>>>> file (as shown in issue 1882 and the attached samples) by ignoring the
>>>>>> badly
>>>>>> tagged channel, instead of refusing to decode.
>>>>>> It doesn't fix the mapping of such FAAC generated's stream (a bit too
>>>>>> complex for my AAC knowledge).
>>>>>
>>>>> I don't have the sample to test this on since roundup is down. However
>>>>> based
>>>>> on visual inspection, this seems to return the same underlying channel
>>>>> element to two bitstream channel elements
>>>>
>>>> Yes, Alex, you've already answered on the bug tracker. The file contains
>>>> SCE[0] CPE[0] CPE[0] LFE[0]
>>>> The patch make libavcodec ignore the first CPE[0], so the file is, at
>>>> least, decoded as 3.1 stream (and not 5.1 like intended, but it's not
>>>> rejected).
>>>>
>>>>
>>>
>>> But it isn't "ignoring" the first CPE, it's writing over the first CPE
>>> clobbering channel element state from the previous frame.
>>>
>>> Consider the output of the follow debug print:
>>>
>>> av_log(NULL, AV_LOG_ERROR, "<%d.%x @ %p>\n", elem_type, elem_id,
>>> elem_type<
>>> TYPE_DSE ? che : NULL);
>>>
>>> <0.0 @ 0x7ff109c6a010>
>>> <6.5 @ (nil)>
>>> <1.0 @ 0x7ff109d0e010>
>>> <6.a @ (nil)>
>>> <1.0 @ 0x7ff109d0e010>
>>> <6.a @ (nil)>
>>> <3.0 @ 0x7ff109c6a010>
>>>
>>> Both CPEs have the same address.
>>>
>>
>>
>
> Yes, you're right. At the same time, the stream had (wrongly) said to do
> this, no ?

It had wrongly assigned the same (0) element id to two separate
elements. I'm rather thinking that the encoder didn't assign element
ids to any of the elements and just wrote 0 because it had to write
something to work at all. But that's just a guess. :)

>> Personally, I would reject the patch because it's wrong. I think what
>> is needed is some option to use IDs from 0 as the elements come along.
>> So this stream is SCE0 CPE0 CPE0 LFE0 and we have an option to ignore
>> this as parse it as SCE0 CPE0 CPE1 LFE0 which is what we used to do
>> before we figured out all the channel configuration stuff and now
>> adhere to the spec.
>
> In the roundup issue's list, I've tried this approach, but I don't know the
> consequences. Is it ok to have a stream with only SCE0 CPE1 LFE0 for example
> ?

As noted, we think it should be SCE 0, CPE 0, CPE 1, LFE 0, like other
5.1 streams. The problem is just that the second CPE was given the
same element id as the first.

The consequence of having an override option to assign element ids
naively starting from 0 may be strange channel ordering. At least then
the channels could be decoded completely and one would have the
correct number of channels in the output rather than the file being
rejected.

When we have some channel mapping API somewhere in libavfilter or so,
the channels could then be manually reordered to whatever the user
requires. We cannot really do this automatically for all files if the
syntax elements in the AAC bitstream are not in any defined order.

> If not, then what is the get_che's "fix mapping" part utility ?

If you mean the part that has a comment about SCE[0] CPE[0] CPE[1]
LFE[0], I think the comment covers the reason for the hack pretty
well:

 129         /* Some streams incorrectly code 5.1 audio as SCE[0]
CPE[0] CPE[1] SCE[1]
 130            instead of SCE[0] CPE[0] CPE[1] LFE[0]. If we seem to have
 131            encountered such a stream, transfer the LFE[0] element
to the SCE[1]'s mapping */

But to elaborate - we want to have a consistent mapping for 5.1
streams and we want to use the TYPE_LFE for the LFE element rather
than using an SCE element. This is to make the stream behave properly
with respect to the way the spec and decoder handle 5.1 streams.

> Do you mean get_che top part:
>
> ? ?if (ac->tag_che_map[type][elem_id]) {
> ? ? ? ?return ac->tag_che_map[type][elem_id];
> ? ?}
> is changed to (pseudo code):
> ? ?if (ac->tag_che_map[type][elem_id]) {
> ? ? ? ?if (ac->tags_mapped == tags_per_config[ac->m4ac.chan_config]) {
> ? ? ? ? ? ?return ac->tag_che_map[type][elem_id];
> ? ? ? ?} else {
> ? ? ? ? ? ?ac->tag_che_map[type][elem_id + 1] = ac->che[type][elem_id];
> ? ? ? ? ? ?ac->tags_mapped++;
> ? ? ? ? ? ?return ac->tag_che_map[type][elem_id + 1];
> ? ? ? ?}
> ? ?}

I don't think this would work for your SCE[0] CPE[0] CPE[0] LFE[0]
case because then both CPEs would be mapped to elem_id + 1 = 0 + 1 =
1.

I rather mean to have an option in the context and avcodec.h exposed
to the user to toggle a completely different (and naive) mapping mode.
The mode would bypass the main code of get_che () and rather keep a
count of the number of syntax elements of a particular type that have
been encountered thus far. I think Alex is thinking the same but I
don't quite understand his (16*count + elem_id) comment on roundup.

Here is what I'm thinking:

- have a flag for this mapping mode in the context and expose it to
the user through avcodec.h
- have syntax element count variables in the context
- map syntax elements using the following algorithm:
  if syntax element has been mapped already, use the mapping
  else map to [type][syntax element count] and increment syntax element count

This should then do the following for the SCE[0] CPE[0] CPE[0] LFE[0] stream:

Receive SCE[0], not yet mapped, sce_count is currently 0, map to
[TYPE_SCE][sce_count], sce_count++.
Receive CPE[0], not yet mapped, cpe_count is currently 0, map to
[TYPE_CPE][cpe_count], cpe_count++.
Receive CPE[0] (except this is the second CPE[0]), not yet mapped,
cpe_count is currently 1, map to [TYPE_CPE][cpe_count], cpe_count++.
Receive LFE[0], not yet mapped, lfe_count is currently 0, map to
[TYPE_LFE][lfe_count], lfe_count++.

The difficult part of this code is deciding when something is not yet
mapped. I think it has to conduct the mapping based on merely the
syntax elements encountered in the first frame ignoring the element
ids and then consider them to be mapped.

It's been a while since I looked at and thought about this code and
part of the spec, but I have a vague memory that we will have to
implement the workaround in other places in the decoder as well. That
is, I think there are other parts of the decoder that depend on the
element ids being correct. Maybe it would be better to note when
parsing the first frame if multiple elements with the same element id
are encountered and then fudge the element ids that are used from
there onward. Alex, what do you think?

Regards,
Rob