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

[Cyril Russo]

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 ?
> 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 ?
If not, then what is the get_che's "fix mapping" part utility ?

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];
         }
     }

Regards,
Cyril