[FFmpeg-devel] [PATCH] avcodec/get_bits: Avoid reading multiple times in get_bits_long

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Due to non-byte-alignment a read of 32 bits guarantees only
> 25 usable bits; therefore get_bits_long() (which is used to
> potentially read more than 25 bits) performs two reads in case
> it needs to read more than 25 bits and combines the result.
> 
> Yet this is not necessary: One can just read 64 bits at a time
> to get 32 usable bits (57 would be possible). This commit does so.
> 
> This reduced the size of .text by 30144B for GCC 11.4 and 5648B
> for Clang 14 (both with -O3).
> 
> (get_bits_long() is a building block of show_bits_long()
> and get_ue_golomb_long(), so this patch affects these, too.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>  libavcodec/get_bits.h | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
> index 0594e104bb..f43bcae91e 100644
> --- a/libavcodec/get_bits.h
> +++ b/libavcodec/get_bits.h
> @@ -187,21 +187,28 @@ static inline unsigned int show_bits(GetBitContext *s, int n);
>  
>  #define CLOSE_READER(name, gb) (gb)->index = name ## _index
>  
> +#define UPDATE_CACHE_BE_EXT(name, gb, bits, dst_bits) name ## _cache = \
> +    AV_RB ## bits((gb)->buffer + (name ## _index >> 3)) << (name ## _index & 7) >> (bits - dst_bits)
> +
> +#define UPDATE_CACHE_LE_EXT(name, gb, bits, dst_bits) name ## _cache = \
> +    (uint ## dst_bits ## _t)(AV_RL ## bits((gb)->buffer + (name ## _index >> 3)) >> (name ## _index & 7))
> +
> +/* Using these two macros ensures that 32 bits are available. */
> +# define UPDATE_CACHE_LE_32(name, gb) UPDATE_CACHE_LE_EXT(name, (gb), 64, 32)
> +
> +# define UPDATE_CACHE_BE_32(name, gb) UPDATE_CACHE_BE_EXT(name, (gb), 64, 32)
> +
>  # ifdef LONG_BITSTREAM_READER
>  
> -# define UPDATE_CACHE_LE(name, gb) name ## _cache = \
> -      AV_RL64((gb)->buffer + (name ## _index >> 3)) >> (name ## _index & 7)
> +# define UPDATE_CACHE_LE(name, gb) UPDATE_CACHE_LE_32(name, (gb))
>  
> -# define UPDATE_CACHE_BE(name, gb) name ## _cache = \
> -      AV_RB64((gb)->buffer + (name ## _index >> 3)) >> (32 - (name ## _index & 7))
> +# define UPDATE_CACHE_BE(name, gb) UPDATE_CACHE_BE_32(name, (gb))
>  
>  #else
>  
> -# define UPDATE_CACHE_LE(name, gb) name ## _cache = \
> -      AV_RL32((gb)->buffer + (name ## _index >> 3)) >> (name ## _index & 7)
> +# define UPDATE_CACHE_LE(name, gb) UPDATE_CACHE_LE_EXT(name, (gb), 32, 32)
>  
> -# define UPDATE_CACHE_BE(name, gb) name ## _cache = \
> -      AV_RB32((gb)->buffer + (name ## _index >> 3)) << (name ## _index & 7)
> +# define UPDATE_CACHE_BE(name, gb) UPDATE_CACHE_BE_EXT(name, (gb), 32, 32)
>  
>  #endif
>  
> @@ -209,12 +216,14 @@ static inline unsigned int show_bits(GetBitContext *s, int n);
>  #ifdef BITSTREAM_READER_LE
>  
>  # define UPDATE_CACHE(name, gb) UPDATE_CACHE_LE(name, gb)
> +# define UPDATE_CACHE_32(name, gb) UPDATE_CACHE_LE_32(name, (gb))
>  
>  # define SKIP_CACHE(name, gb, num) name ## _cache >>= (num)
>  
>  #else
>  
>  # define UPDATE_CACHE(name, gb) UPDATE_CACHE_BE(name, gb)
> +# define UPDATE_CACHE_32(name, gb) UPDATE_CACHE_BE_32(name, (gb))
>  
>  # define SKIP_CACHE(name, gb, num) name ## _cache <<= (num)
>  
> @@ -414,15 +423,26 @@ static inline unsigned int get_bits_long(GetBitContext *s, int n)
>      av_assert2(n>=0 && n<=32);
>      if (!n) {
>          return 0;
> -    } else if (n <= MIN_CACHE_BITS) {
> +    } else if ((!HAVE_FAST_64BIT || av_builtin_constant_p(n <= MIN_CACHE_BITS))
> +               && n <= MIN_CACHE_BITS) {
>          return get_bits(s, n);
>      } else {
> +#if HAVE_FAST_64BIT
> +        unsigned tmp;
> +        OPEN_READER(re, s);
> +        UPDATE_CACHE_32(re, s);
> +        tmp = SHOW_UBITS(re, s, n);
> +        LAST_SKIP_BITS(re, s, n);
> +        CLOSE_READER(re, s);
> +        return tmp;
> +#else
>  #ifdef BITSTREAM_READER_LE
>          unsigned ret = get_bits(s, 16);
>          return ret | (get_bits(s, n - 16) << 16);
>  #else
>          unsigned ret = get_bits(s, 16) << (n - 16);
>          return ret | get_bits(s, n - 16);
> +#endif
>  #endif
>      }
>  }

Will apply this tonight unless there are objections.

- Andreas