[FFmpeg-devel] [RFC v2 3/3] daaladec: Implement a native Daala decoder

[Andreas Cadhalpun]

Hi,

On 30.12.2015 02:09, Ronald S. Bultje wrote:
> On Tue, Dec 29, 2015 at 6:54 PM, Andreas Cadhalpun <
> andreas.cadhalpun at googlemail.com> wrote:
>> I prefer that input for such performance-sensitive dsp code gets sanitized,
>> so that overflows are (nearly) impossible.
> 
> 
> I don't think it makes sense to make the decoder 5% slower just so that
> ubsan is happy. We don't do that for other video decoders either. And yes,
> the coefficient decoding loop is typically a major hotspot in video codecs.

It's not quite 5%, only 2-3%, but I agree that this slowdown is not acceptable.
Maybe it's possible to do the input sanitizing earlier, outside of performance
sensitive code.

On 30.12.2015 03:12, Ganesh Ajjanagadde wrote:
> 2. If things can't be transformed to be safe without leading to
> significant loss of performance, such things must be clearly
> documented as comments, fully justified, and very carefully
> scrutinized IMHO.

I agree. If the overflows can't be reasonably fixed, they should at least
be documented.

On 30.12.2015 04:13, Rostislav Pehlivanov wrote:
> I agree with BBB (Ronald), this project claims to have the fastest decoders
> available and we shouldn't sacrifice performance. Since basically the only
> input of the decoder goes through the entropy decoding system occasionally
> checking for overflows in a few performance sensitive parts there is worth
> it, as it has been pointed out.

If a few more checks in the entropy decoding system would prevent these
overflows, that would be great.

> Also I'd like to remind people that this is an RFC and  WIP. Overflow
> checking and fuzzing aren't high priority yet.

Experience has shown that trying to fix such issues a few month after a
decoder has been added is rather tedious.

> I wanted to know if I was doing something horribly wrong, typos, naive
> code, asserts/crashes on normal files, glitches, etc.

None of that seems to be the case. ;)

However, I think this decoder should be flagged with AV_CODEC_CAP_EXPERIMENTAL,
at least until it can decode more than I-frames.

One thing I find rather annoying: there are lots of quite long #define's
in daaladsp.c, which is bad for debugging purposes, as the debugger treats
each #define only as one line.
Since most of these defines are used exactly once, I'm wondering why you
don't use the code directly. I think that would also improve readability.

Best regards,
Andreas