[FFmpeg-devel] [PATCH 0/7] Replace native DCA decoder with libdcadec based one

[Hendrik Leppkes]

On Fri, Jan 15, 2016 at 6:12 AM, James Almer <jamrial at gmail.com> wrote:
> On 1/14/2016 9:59 PM, Andreas Cadhalpun wrote:
>> On 14.01.2016 17:25, foo86 wrote:
>>> Full diff output has been omitted for deleted files. If git complains about
>>> applying the first patch, this can be also pulled from dca-replace branch at
>>> [1].
>>
>> I'd prefer if you would post full patches here, i.e. including deleted files.
>
> For future, smaller patches if anything. Doing so here would mean thousands of
> extra lines for no reason or benefit. And i remember there being a problem with
> git send-email regarding huge emails.
>
>>
>> That aside, the new decoder seems fine from a security point of view, with
>> only some rare overflows in the dsp functions left.
>>
>> However, this series breaks FATE.
>> The fate-dca-xll test should probably be disabled, as the reference was created
>> with the old, not bitexact decoder. (It currently fails due to the disable_xll
>> option being gone, but fixing that reveals the changed output.)
>>
>> The checkasm test needs to be updated:
>
> Removed, actually. The new lfe_fir dsp functions don't yet have arch optimized
> versions, so even if you adapt this code now it will not be really tested and
> reported as part of the checkasm output since there's nothing to compare the C
> version with.
> It can be readded and adapted once said arch optimized functions are written.
>

Other than the comments already made here, this set looks fine to me.
The missing changes are trivial removals of (now broken) tests, so I
could do that when pushing as well if we all agree.

New tests should then be added afterwards.

Everyone else, if you still have outstanding comments on this set,
please do speak up soon, otherwise I would say we give it another
couple days and then just push it, so we can move forward.
Anyone any opinions if this should be squashed to avoid leaving us
without a dca decoder for a few commits? Probably should be, right?

- Hendrik