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

[Rostislav Pehlivanov]

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.
Also I'd like to remind people that this is an RFC and  WIP. Overflow
checking and fuzzing aren't high priority yet. I wanted to know if I was
doing something horribly wrong, typos, naive code, asserts/crashes on
normal files, glitches, etc.

On 30 December 2015 at 02:12, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:

> On Tue, Dec 29, 2015 at 5:09 PM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> > Hi,
> >
> > On Tue, Dec 29, 2015 at 6:54 PM, Andreas Cadhalpun <
> > andreas.cadhalpun at googlemail.com> wrote:
> >
> >> Hi,
> >>
> >> On 29.12.2015 19:46, Ronald S. Bultje wrote:
> >> > On Tue, Dec 29, 2015 at 11:49 AM, Andreas Cadhalpun <
> >> > andreas.cadhalpun at googlemail.com> wrote:
> >> >> Do you have a sample causing overflows in the vp9 decoder?
> >> >
> >> >
> >> > Nope, but I'm going to assume they're not hard to create, just use a
> few
> >> > high same-sign quantized coefficients (e.g. SHORT_MAX) and a high
> >> quantizer
> >> > (qidx=255, dq_ac=1828). Both dequant into int16_t (1828*SHORT_MAX
> doesn't
> >> > fit in 16 bits) as well as the idct results themselves (because you're
> >> > adding and subtracting from an already near-edge value which should
> fit
> >> in
> >> > 16bits) will overflow.
> >>
> >> I guess hard depends on who you ask. I've fuzzed the vp9 decoder and
> >> haven't yet come across a sample that triggers a signed integer overflow
> >> there.
> >> In the case of this daala decoder, 3/4 of the fuzzed samples do...
> >>
> >> >> (Overflows in dsp code are typically not a security concern.)
> >> >>
> >> >> Well, the overflows in the imdct calculation of the aac_fixed decoder
> >> >> ultimately
> >> >> caused crashes.
> >> >
> >> >
> >> > I would prefer if for video codecs, unless specifically proven
> otherwise
> >> > for that codec with specific content, we assume by default that
> overflows
> >> > in performance-sensitive dsp code (specifically idct) are OK. ubsan
> may
> >> not
> >> > like us on fuzzed content, but so be it.
> >>
> >> 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.
>
> Basically, I think we need to strike a very fine balance here, and I
> offer my own thoughts on this below:
> 1. On the one hand, it is difficult to reason how undefined behavior
> in a tight loop can impact other places in the code. It may or may not
> be safe in a practical context. If things can be changed to be safe at
> negligible performance impact, it should be done as a defensive
> programming measure IMHO. Theoretically, undefined means that anything
> can happen, but practically the range of possibilities is far more
> limited (no one runs ubsan in production). We already do not take into
> account all theoretical aspects: for example, nothing dictates the
> choice of the IEEE-754 floating point format, but some code does
> assume it in FFmpeg. As such, we are anyway operating in a "practical"
> context, and theoretical purity must be balanced with practical
> impact, something I forget myself a lot.
>
> 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.
>
> 3. Note that integer overflow is a hard problem, with endless debate
> all round: https://lwn.net/Articles/547649/,
> https://news.ycombinator.com/item?id=8765714,
> https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md
> - even Rust essentially cops out by default on release builds. The
> only thing is that they took more responsibility regarding the
> semantics than the C standard - although C leaves it undefined, in
> practice Rust and C will generate the same code here AFAIK (note: I
> have not studied Rust, just casually read about it). The real problem
> comes in what clients do with the outputs of such overflow: in Rust
> for instance array bounds derived from such arithmetic will be
> checked, in C it will lead to big trouble. Then again, the funny thing
> is: just having "safety" is not enough, the burden is anyway on the
> programmer to handle such "weird" cases via some form of checking
> code, regardless of the language, or careful thought to ensure that
> such things don't happen (e.g the naive (min+max)/2 vs min +
> (max-min)/2 for binary search). No language can do the "right thing"
> automatically: the "right thing" varies depending on the situation.
> See the next point.
>
> 4. Even with integer overflow checks, note that the method of handling
> can vary significantly, as can be seen in FFmpeg. It is not easy to
> audit every line, and create brittle checks that make hard to
> understand code even harder to understand. Wish there were
> alternatives, but I see none: some things are inherently not simple.
>
> >
> > Ronald
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>