Hi, On Fri, Aug 10, 2018 at 6:54 PM, Michael Niedermayer <mich...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
