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

[foo86]

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.

> +#endif
> +
> +#else
>      register int tmp;
>      OPEN_READER(re, s);
>      av_assert2(n>0 && n<=25);
>      UPDATE_CACHE(re, s);
> -    tmp = SHOW_SBITS(re, s, n);
> +    tmp = SHOW_UBITS(re, s, n);
>      LAST_SKIP_BITS(re, s, n);
>      CLOSE_READER(re, s);
> +#endif
>      return tmp;
>  }

The code under #ifdef CACHED_BITSTREAM_READER can probably be simplified
like this (analogous to show_bits()):

    if (n > s->bits_left)
        refill_32(s);

    tmp = get_val(s, n);

This avoids UB and is simpler/faster. Or am I missing something here?