[FFmpeg-devel] [PATCH 1/2] avcodec/vp9: Check in decode_tiles() if there is data remaining

Ronald S. Bultje rsbultje at gmail.com
Sat Aug 11 05:41:21 EEST 2018


Hi,

On Fri, Aug 10, 2018 at 6:54 PM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Fri, Aug 10, 2018 at 02:16:56AM -0400, Ronald S. Bultje wrote:
> > My objection:
> > if a file has exactly symbols of 8 bits in arithdata, then after all
> this,
> > the arithcoder will signal empty and EOF, even though no error occured.
> > Imagine a bitcoder (non-arith) of this situation.
> [..]
> > After get_bits(gb, 8),
> > the data pointer will have reached the end, and the bits_left is 0, but
> > that does not indicate an error, quite the contrary. It just means that
> the
> > byte boundary happened to align with the exact end of the file. This can
> > happen.
>
> Yes but thats not something we do with bitcoders.
> bits_left being 0 does indicate an error when the next
> step unconditionally reads 1 or more bits.
> The same was the intend of the patch here, check if the end was reached
> before more reads.
> vp56_rac_renorm() always reads data if(bits >= 0 && c->buffer < c->end)
>
> I had thought that will get executed in all cases, i may have missed
> something and the check should be moved by a few lines


For example, you're checking the arithcoder in pass=2 also, but no
bitstream reading occurs in that pass...

> My suggestion:
> > add an eof flag to the arithcoder. When we have reached the above
> condition
> > where new data is needed but not present, simply set the EOF flag, and
> > check that for errors. If it's set, you can error out.
>
> This is possible but it will make the code likely slower as the reading
> happens in a av_always_inline function which itself is used by several
> av_always_inline functions. So this else context->flag = 1;
> could end up being added to many points in the binary.
>
> I can do this of course if you prefer it just seems sub-optimal to me
> unless you have some idea on how to do this without increasing the
> complexity of the rac functions


But it's an error branch, it is not normally executed. It just makes the
binary a few bytes larger.

Here's another way of looking at this, which isn't so much about exact
bytes and instructions (which will all change in the noise range), but
about code maintainability: you're likely going to want to do fixes like
this for vp8, vp9, vp56, etc., and similar for users of other arithcoders
like cabac or the multisymbol one in daala/av1 and so on. Each of these
decoders are in and by themselves pretty complex creatures, and the people
understanding what's going on under the hood are few and far between, and
half of them are no longer active. I'm not saying you're not intelligent,
but I do think bugs like the one above can creep in because you're not
intimately familiar with all bells and whistles in each decoder codebase.
That being true, a case could be made that noise-range changes in
instruction count or byte size is less important than ease of maintenance.
An EOF flag is easy to maintain and hard to misread, whereas the example
above demonstrates that the inferred checks are brittle and will be much
harder to debug if they do make it into our codebase because someone forgot
to review them.

So it's a balance between priorities. Does every cycle count? Or is
maintenance more important? Each of us is correct in our point, nobody is
wrong, but we need to balance which one is more important...

Ronald


More information about the ffmpeg-devel mailing list