[FFmpeg-devel] [PATCH v2 00/16] Replace native DCA decoder with libdcadec based one

[Hendrik Leppkes]

On Mon, Jan 25, 2016 at 2:57 AM, James Almer <jamrial at gmail.com> wrote:
> On 1/22/2016 7:25 PM, Michael Niedermayer wrote:
>> On Thu, Jan 21, 2016 at 09:44:06PM +0300, foo86 wrote:
>>> Updated version of the patch. I choose to split it into even smaller commits to
>>> make reviewing easier; you may prefer to squash it as needed.
>>>
>>> Changes since the first version:
>>>
>>> * Removed checkasm test for dcadsp
>>> * Removed FATE test for dca-xll (didn't check if this works)
>>> * Core decoder now uses butterflies_fixed() for sumdiff decoding
>>> * avpriv_request_sample() is now used for reporting missing features
>>> * Core decoder now stays in fixed point mode during intermittent XLL decoding errors
>>> * X96 extension is no longer parsed (and left unused) when decoding XLL
>>> * Minor code refactoring
>>>
>>> foo86 (16):
>>>   avcodec/dca: remove old decoder
>>>   avcodec/dca: remove unused assembly
>>>   avcodec/dca: remove unused data
>>>   tests/fate/audio: remove dca-xll test
>>>   tests/checkasm: remove dcadsp test
>>>   avcodec/dca: add REV1AUX sync word
>>>   avcodec/dca: add more tables
>>>   avcodec/dca: add math helpers and fixed point DCT
>>>   avcodec/synth_filter: fix whitespace
>>>   avcodec/synth_filter: add more filters
>>>   avcodec/dca: add DSP implementation
>>>   avcodec/dca: add generic defines
>>>   avcodec/dca: add core decoder
>>>   avcodec/dca: add EXSS parser
>>>   avcodec/dca: add XLL decoder
>>>   avcodec/dca: add new decoder based on libdcadec
>>
>> This breaks decoding https://trac.ffmpeg.org/raw-attachment/ticket/3550/WorstCase.wav
>> didnt investgate what or why
>
> Foo86: When you fix this, if it's a trivial change to any of the patches you
> sent then just resend that patch alone, not the full set.
> That is, unless you're also making other changes, like disabling fate-dca,
> fate-dts_es and any remaining reference to CONFIG_DCA_DECODER after patch 5.

The decoder in itself looks fine to me, short of the regression michael found.
If you could look at that, that would be great.

I can squash and re-shuffle the commits appropriately for pushing and
make sure all intermediate steps still build, so don't worry about
that as much.
I would love to get this in soon, we can do further improvements and
more FATE coverage after.

There doesn't seem to be much agreement yet of the order of pushing.
I would argue that since we're replacing the decoder entirely anyway,
a tiny period in between where we don't actually have a dca decoder
wouldn't break any bisect flow, since it would probably end there
anyway.
So considering that, it feels cleaner to me to push the removal first,
and then the additions for the new decoder for "prettier" history.

- Hendrik