[FFmpeg-devel] [PATCH] AAC: reject multiple channel configurations

[Robert Swain]

2009/7/9 Alex Converse <alex.converse at gmail.com>:
> On Wed, Jul 8, 2009 at 12:55 AM, Alex Converse<alex.converse at gmail.com> wrote:
>> On Tue, Jul 7, 2009 at 6:16 PM, Robert Swain<robert.swain at gmail.com> wrote:
>>> 2009/7/7 Alex Converse <alex.converse at gmail.com>:

>>>> Invalid second channel configurations are one of the biggest causes of
>>>> crashes in the AAC deocder in my experience. See issue 1254 for an
>>>> example. If they are even legal is dubious at best. From 14496-3:2005:
>>>
>>> [...]
>>>
>>>> Even if they are legal we aren't reliably able to handle them at the
>>>> moment so they should be disabled.
>>>
>>> Agreed. Thank you for the extensive and thorough argument, it's much
>>> appreciated. :)
>>>
>>>> The attached patch does not break conformance.
>>>
>>> Which issues does it fix? It would be good to have this in the commit
>>> message if possible.
>>>
>>
>> Issue 1254 (as mentioned above :) )

Yeah, I just wondered if there were others. :)

>> However, I went to test one more thing before applying and realized
>> this breaks ADTS files with PCEs. (MPEG never should have allowed PCEs
>> to be sent in band it's such a disaster.)
>>
>> For ADTS files with PCEs:
>>
>> Parser inits
>> AAC Decoder inits
>> -> no extradata, avccontext->channels = 0 --> no output configuration
>> AAC Decoder decodes first frame
>> ?-> AAC Decoder finds PCE in the frame
>> ? ? -> output configures
>> AAC Decoder closes
>> ?-> output unconfigures
>> AAC Decoder inits
>> ?-> avccontext->channels > 0 --> output configures
>> AAC Decoder decodes first frame
>> ?-> AAC Decoder finds PCE in the frame
>> ? ?-> Danger Will Robinson!
>>
>> Options:

[...]

>> D: Something awesome that I haven't thought of yet.
>
> Option D: (attached)

Looks good, assuming that there's no case other than ADTS where the
number of channels is defined but extradata is not which now becomes
broken as a consequence. As far as I'm aware we only support AAC with
extradata or ADTS headers.

One minor nit - I'm not sure there's need to zero
ac->output_configured in aac_decode_close(). I think the codec close
functions are only needed for freeing allocated buffers from the codec
context, not for zeroing variables/buffers the context. It doesn't do
any harm though. :)

OK to apply.

Regards,
Rob