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

[Cyril Russo]

>>> 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.
>    
Ok, then let's sum this up, we have to add a sce_count, cpe_count and 
lfe_count member to the context.
Then, in codec init, set them to 0.

If the codec's workaround_bugs contains a specific flag, then we bypass 
get_che actual code if and only if codec's frame_number is 0.
In the bypassed version, we map as you said.

I propose this addition:
In the code I've tried to fix (top of get_che), I change this:

    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 {
            // Log warning
            // Enable workaround_bugs in the context,
            // Count the used mapped element and increment sce/cpe/lfe count accordingly
            // Map the badding tagged element
        }
    }



> 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?
>    
Can you check this again ?

Cheers,
Cyril