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

[Michael Niedermayer]

On Tue, Apr 03, 2018 at 01:38:12PM +0200, Paul B Mahol wrote:
[...]

>  
> -static inline void skip_bits_long(GetBitContext *s, int n)
> +static inline void refill_32(GetBitContext *s)
>  {
> -#if UNCHECKED_BITSTREAM_READER
> -    s->index += n;
> +#ifdef CACHED_BITSTREAM_READER
> +#if !UNCHECKED_BITSTREAM_READER
> +    if (s->index >> 3 >= s->buffer_end - s->buffer)
> +        return;
> +#endif
> +
> +#ifdef BITSTREAM_READER_LE
> +    s->cache       = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache;
>  #else
> -    s->index += av_clip(n, -s->index, s->size_in_bits_plus8 - s->index);
> +    s->cache       = s->cache | (uint64_t)AV_RB32(s->buffer + (s->index >> 3)) << (32 - s->bits_left);
> +#endif
> +    s->index     += 32;
> +    s->bits_left += 32;
> +#endif
> +}

please split the movement of code into a seperate patch, this is unreadable


> +
> +static inline void refill_64(GetBitContext *s)
> +{
> +#ifdef CACHED_BITSTREAM_READER
> +#if !UNCHECKED_BITSTREAM_READER
> +    if (s->index >> 3 >= s->buffer_end - s->buffer)
> +        return;
> +#endif
> +
> +#ifdef BITSTREAM_READER_LE
> +    s->cache = AV_RL64(s->buffer + (s->index >> 3));
> +#else
> +    s->cache = AV_RB64(s->buffer + (s->index >> 3));
> +#endif
> +    s->index += 64;
> +    s->bits_left = 64;
> +#endif
> +}

all uses of refill_64 are under CACHED_BITSTREAM_READER, so there should
not be a need for an empty function
and the ifdef can be then merged with the next, simplifying the code


> +
> +#ifdef CACHED_BITSTREAM_READER
> +static inline uint64_t get_val(GetBitContext *s, unsigned n, int is_le)
> +{
> +    uint64_t ret;
> +    av_assert2(n>0 && n<=63);
> +    if (is_le) {
> +        ret = s->cache & ((UINT64_C(1) << n) - 1);
> +        s->cache >>= n;
> +    } else {
> +        ret = s->cache >> (64 - n);
> +        s->cache <<= n;
> +    }
> +    s->bits_left -= n;
> +    return ret;
> +}
> +#endif
> +
> +#ifdef CACHED_BITSTREAM_READER

these 2 ifdef CACHED_BITSTREAM_READER can be merged


> +static inline unsigned show_val(const GetBitContext *s, unsigned n)
> +{
> +#ifdef BITSTREAM_READER_LE
> +    return s->cache & ((UINT64_C(1) << n) - 1);
> +#else
> +    return s->cache >> (64 - n);
> +#endif
> +}
> +#endif
> +

> +/**
> + * Show 1-25 bits.
> + */
> +static inline unsigned int show_bits(GetBitContext *s, int n)
> +{
> +    register int tmp;
> +#ifdef CACHED_BITSTREAM_READER
> +    if (n > s->bits_left)
> +        refill_32(s);
> +
> +    tmp = show_val(s, n);
> +#else
> +    OPEN_READER_NOSIZE(re, s);
> +    av_assert2(n>0 && n<=25);
> +    UPDATE_CACHE(re, s);
> +    tmp = SHOW_UBITS(re, s, n);
>  #endif
> +    return tmp;
>  }

This is more moved code in which it is impossible to see what is changed


[...]
>  
> -static inline int get_sbits(GetBitContext *s, int n)
> +/**
> + * Read 1-25 bits.
> + */
> +static inline unsigned int get_bits(GetBitContext *s, int n)

also unreadable and confusing, the code moves make these matchup incorrectly


>  {
>      register int tmp;
> +#ifdef CACHED_BITSTREAM_READER
> +
> +    av_assert2(n>0 && n<=32);
> +    if (n > s->bits_left) {
> +        refill_32(s);
> +        if (s->bits_left < 32)
> +            s->bits_left = n;
> +    }
> +
> +#ifdef BITSTREAM_READER_LE
> +    tmp = get_val(s, n, 1);
> +#else
> +    tmp = get_val(s, n, 0);
> +#endif
> +#else
>      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;
>  }
>  

> -/**
> - * Read 1-25 bits.
> - */
> -static inline unsigned int get_bits(GetBitContext *s, int n)
> +static inline int get_sbits(GetBitContext *s, int n)
>  {
>      register int tmp;
> +#ifdef CACHED_BITSTREAM_READER
> +    av_assert2(n>0 && n<=25);
> +    tmp = sign_extend(get_bits(s, n), n);
> +#else
>      OPEN_READER(re, s);
>      av_assert2(n>0 && n<=25);
>      UPDATE_CACHE(re, s);
> -    tmp = SHOW_UBITS(re, s, n);
> +    tmp = SHOW_SBITS(re, s, n);
>      LAST_SKIP_BITS(re, s, n);
>      CLOSE_READER(re, s);
> +#endif
>      return tmp;
>  }

more mismatched functions that are diffed against each other

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180403/7a0cc171/attachment.sig>