[FFmpeg-devel] [PATCH 1/3] avcodec/get_bits: add cached bitstream reader

[foo86]

On Fri, Jul 14, 2017 at 05:12:25PM +0200, Hendrik Leppkes wrote:
> On Fri, Jul 14, 2017 at 4:08 PM, foo86 <foobaz86 at gmail.com> wrote:
> > On Thu, Jul 13, 2017 at 12:27:03PM +0200, Paul B Mahol wrote:
> >> +static inline unsigned int get_bits(GetBitContext *s, int n)
> >>  {
> >> +#ifdef CACHED_BITSTREAM_READER
> >> +    register int tmp = 0;
> >> +#ifdef BITSTREAM_READER_LE
> >> +    uint64_t left = 0;
> >> +#endif
> >> +
> >> +    av_assert2(n>0 && n<=32);
> >> +    if (n > s->bits_left) {
> >> +        n -= s->bits_left;
> >> +#ifdef BITSTREAM_READER_LE
> >> +        left = s->bits_left;
> >> +#endif
> >> +        tmp = get_val(s, s->bits_left);
> > This triggers an assert in get_val() if s->bits_left == 0.
> >
> >> +        refill_32(s);
> >> +    }
> >> +
> >> +#ifdef BITSTREAM_READER_LE
> >> +    tmp = get_val(s, n) << left | tmp;
> >> +#else
> >> +    tmp = get_val(s, n) | tmp << n;
> > This causes undefined behavior if n > 30.
> 
> get_bits is only valid until n = 25 in the "non-cached" case, so its
> not a problem to impose the same limitation on the cached reader.
> In fact, if they are to share the exact same API, it should probably
> follow that they also share the same constraints, so that we can do
> proper performance comparisons between the two, instead of having to
> re-write the using code.

Cached bitstream reader currently uses get_bits() to implement
get_bits_long(), which means cached get_bits() must support reading
values up to 32 bits.

I agree however that cached/uncached bistream readers should have the
same API contraints. That means cached get_bits_long() should probably
have a separate implementation.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel