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

[James Almer]

On 1/21/2016 12:11 PM, Hendrik Leppkes wrote:
> On Thu, Jan 21, 2016 at 1:50 PM, wm4 <nfxjfg at googlemail.com> wrote:
>> On Thu, 21 Jan 2016 11:46:03 +0100
>> Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>>
>>> 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?
>>
>> Why would this be important? Except maybe that it would be nice if
>> FATE worked at any point in the history (to make bisecting easier).
> 
> That would mean removing and re-adding some stuff, which seems
> somewhat annoying. But I don't really care either way.

We can either apply patches 2 to 6 first then patches 1 and 7 squashed into
one, or all 7 as they are.

Assuming the latter, all dca tests should be commented out and the lfe check
in checkasm removed with patch 1, then the dca tests re-enabled (and dca-xll
updated) with patch 7.
With the former, it would be a matter of updating dca-xll and checkasm in
the squashed patch.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>