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

[Alex Converse]

On Thu, Jul 9, 2009 at 6:28 AM, Robert Swain<robert.swain at gmail.com> wrote:
> 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. :)
>

You are correct as usual.

> OK to apply.

applied with one minor "{" on the wrong line (in new code) issue fixed.

Regards,
Alex