[FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.

[Hendrik Leppkes]

On Wed, Jan 6, 2016 at 12:40 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Tue, Jan 5, 2016 at 3:28 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> On Tue, Jan 5, 2016 at 10:46 PM, Andreas Cadhalpun
>> <andreas.cadhalpun at googlemail.com> wrote:
>>> On 05.01.2016 21:38, foo86 wrote:
>>>> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote:
>>>>> On 03.01.2016 18:49, foo86 wrote:
>>>>>> +// 5.3.1 - Bit stream header
>>>>>> +static int parse_frame_header(DCA2CoreDecoder *s)
>>>>>> +{
>>>>> [...]
>>>>>> +    // Source PCM resolution
>>>>>> +    s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = get_bits(&s->gb, 3)];
>>>>>
>>>>> This can cause an out-of-bounds read if get_bits returns 7, because ff_dca_bits_per_sample
>>>>> only has 7 elements.
>>>>
>>>> Fixed locally, thanks.
>>>
>>> Thanks.
>>>
>>>> P.S. To avoid resending this huge patch, I've put the fixes accumulated
>>>> so far in a private dcadec2 branch on github [1] (will be rebased
>>>> frequently against FFmpeg master).
>>>>
>>>> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2
>>>
>>> OK. This decoder seems to be quite robust in handling fuzzed samples,
>>> so from a security point of view it should be fine to replace the
>>> old dca decoder with this one.
>>>
>>
>>
>> So that leaves us with a bunch of positive comments, on this side
>> anyway, and noone opposed yet.
>>
>> Arguments for a switch include:
>> - Nearly complete coverage of all DTS features, well tested and
>> confirmed bit-exact (only DTS Express is missing, which is technically
>> its own little codec using DTS EXSS headers)
>> - Slightly faster (~5%)
>> - Active maintainer
>> - Andreas seal of security approval ;)
>>
>> If anyone thinks we should not replace our decoder, speak now or
>> forever hold your peace (and bring proper arguments).
>> I will try to do a code review to the best of my abilities in the upcoming days.
>
> For now, I definitely think we should replace our decoder.
> Just a clarification: in the long run, isn't it a good idea to get
> this into the repo and not use an external library? For instance, if
> and when asm code gets written for this, isn't it easier to work
> within FFmpeg's codebase, which has some infrastructure for writing
> asm?
>

The whole idea of this patch is to integrate the decoder fully into
our codebase, and no longer use the external library.

PS:
What I forgot to mention in my last mail, foo86 if you have any
questions about ripping out the old dca decoder, feel free to contact
me.

- Hendrik