Hi, On Mon, Dec 12, 2011 at 10:50 AM, Alex Converse <[email protected]>wrote:
> On Mon, Dec 12, 2011 at 8:55 AM, Ronald S. Bultje <[email protected]>wrote: > >> On Dec 12, 2011 8:50 AM, "Laurent Aimar" <[email protected]> wrote: >> >> > On Mon, Dec 12, 2011 at 08:35:16AM -0800, Ronald S. Bultje wrote: >> > > Hi, >> > > >> > > On Mon, Dec 12, 2011 at 7:54 AM, Laurent Aimar <[email protected]> >> wrote: >> > > >> > > I think that with your patch, get_bits_count() will never be >> negative and >> > > this >> > > will creates issues with some decoders. >> > > >> > > >> > > We should fix those decoders. get_bits_left() < 0 is a bug and we >> should >> > > document that as undefined behaviour, IMO. >> > In itself it is not a bug. It is prefectly fine for get_bits_left() to >> > return < 0 without creating any issue if the user of the get bits >> function >> > ensure that the 'overread' does not exceed the mandatory padding there >> is >> > at the end of each buffer. >> > >> > Now, it can of course be decided to make get_bits_left() returning < 0 >> > a non valid use case, but it's a change from what I think was previously >> > understood (at least that's my impression reading various decoder >> codes). >> > It will probably need a lot of code review before the change can be done >> > safely. >> >> I agree, but I do feel this change has lowest effect on performance while >> still covering all of get_bits. >> >> Want to help review? >> > > > static inline void skip_bits_long(GetBitContext *s, int n){ > +#if UNCHECKED_BITSTREAM_READER > s->index += n; > +#else > + s->index = FFMIN(s->size_in_bits, s->index + n); > If we make this FFMIN(s->size_in_bits + 1, s->index + n) can't we get the > best of both worlds for just one additional add? > Yeah I think that's a good idea, better yet we can add s->size_in_bits_plus1 to the context and use that to prevent the extra instruction. Maybe we can even get rid of s->size_in_bits, but that's not so important. Ronald
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
