[FFmpeg-devel,1/2] Expose and start using skip_remaining

Bitstream readers sometimes have already checked there are enough
bits, and the check is redundant.
---
 libavcodec/bitstream.h          |  8 +++++---
 libavcodec/bitstream_template.h | 22 +++++++++++-----------
 libavcodec/get_bits.h           |  1 +
 3 files changed, 17 insertions(+), 14 deletions(-)

Christophe Gisquet:
> Bitstream readers sometimes have already checked there are enough
> bits, and the check is redundant.

This patch aims to do two things; and these should be in separate
patches so that one can see immediately where you just change the name
and where you change the actual code.

> ---
>  libavcodec/bitstream.h          |  8 +++++---
>  libavcodec/bitstream_template.h | 22 +++++++++++-----------
>  libavcodec/get_bits.h           |  1 +
>  3 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/libavcodec/bitstream.h b/libavcodec/bitstream.h
> index 35b7873b9c..dd043fb349 100644
> --- a/libavcodec/bitstream.h
> +++ b/libavcodec/bitstream.h
> @@ -95,6 +95,7 @@
>  # define bits_peek_signed   bits_peek_signed_le
>  # define bits_peek_signed_nz bits_peek_signed_nz_le
>  # define bits_skip          bits_skip_le
> +# define bits_skip_remaining bits_skip_remaining_le
>  # define bits_seek          bits_seek_le
>  # define bits_align         bits_align_le
>  # define bits_read_xbits    bits_read_xbits_le
> @@ -124,6 +125,7 @@
>  # define bits_peek_signed   bits_peek_signed_be
>  # define bits_peek_signed_nz bits_peek_signed_nz_be
>  # define bits_skip          bits_skip_be
> +# define bits_skip_remaining bits_skip_remaining_be
>  # define bits_seek          bits_seek_be
>  # define bits_align         bits_align_be
>  # define bits_read_xbits    bits_read_xbits_be
> @@ -146,7 +148,7 @@
>          n     = table[index].len;                           \
>                                                              \
>          if (max_depth > 1 && n < 0) {                       \
> -            bits_skip(bc, bits);                            \
> +            skip_remaining(bc, bits);                       \

This is problematic, because you seem to think that bits_peek(bc, bits)
ensures that there are at least `bits` available in the cache; yet this
is not so, because it can happen that one reached the end of input in
which case no refilling happens. See
https://github.com/mkver/FFmpeg/commit/fba57506a9cf6be2f4aa5eeee7b10d54729fd92a
for a way that fixes this.

Now that I have written this, I have to admit that the current code here
is also very problematic: bits_skip() is also suffering from the fallacy
that priv_refill_64() always works. Even worse, the code simply
increments the buffer ptr without checking the bounds (the whole branch
for n >= 64 is of course nonsense for BITS_RL_VLC. In fact, I think that
we will end up with the exact same state in case the reloading in
bits_peek() failed with bits_skip() and skip_remaining().
Luckily BITS_RL_VLC is absolutely unused.

Needless to say, a proper fix involves something along the lines of my
patch above. But this patch is based around the assumption that the
combined amount of bits consumed in any get_vlc2/GET_RL_VLC/BITS_RL_VLC
call can't exceed 32. Is this assumption actually still true now that we
have multi-vlc stuff?

https://github.com/mkver/FFmpeg/commit/9b5a977957968c0718dea55a5b15f060ef6201dc
and
https://github.com/mkver/FFmpeg/commits/aligned32_le_bitstream_reader
are probably also of interest to you.

>                                                              \
>              nb_bits = -n;                                   \
>                                                              \
> @@ -154,7 +156,7 @@
>              level = table[index].level;                     \
>              n     = table[index].len;                       \
>              if (max_depth > 2 && n < 0) {                   \
> -                bits_skip(bc, nb_bits);                     \
> +                skip_remaining(bc, nb_bits);                \
>                  nb_bits = -n;                               \
>                                                              \
>                  index = bits_peek(bc, nb_bits) + level;     \
> @@ -163,7 +165,7 @@
>              }                                               \
>          }                                                   \
>          run = table[index].run;                             \
> -        bits_skip(bc, n);                                   \
> +        skip_remaining(bc, n);                              \
>      } while (0)
>  
>  #endif /* AVCODEC_BITSTREAM_H */
> diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h
> index 0308e3a924..3f90fc6a07 100644
> --- a/libavcodec/bitstream_template.h
> +++ b/libavcodec/bitstream_template.h
> @@ -175,7 +175,7 @@ static inline uint64_t BS_FUNC(priv_val_show)(BSCTX *bc, unsigned int n)
>  #endif
>  }
>  
> -static inline void BS_FUNC(priv_skip_remaining)(BSCTX *bc, unsigned int n)
> +static inline void BS_FUNC(skip_remaining)(BSCTX *bc, unsigned int n)
>  {
>  #ifdef BITSTREAM_TEMPLATE_LE
>      bc->bits >>= n;
> @@ -192,7 +192,7 @@ static inline uint64_t BS_FUNC(priv_val_get)(BSCTX *bc, unsigned int n)
>      av_assert2(n > 0 && n < 64);
>  
>      ret = BS_FUNC(priv_val_show)(bc, n);
> -    BS_FUNC(priv_skip_remaining)(bc, n);
> +    BS_FUNC(skip_remaining)(bc, n);
>  
>      return ret;
>  }
> @@ -375,7 +375,7 @@ static inline int BS_FUNC(peek_signed)(BSCTX *bc, unsigned int n)
>  static inline void BS_FUNC(skip)(BSCTX *bc, unsigned int n)
>  {
>      if (n < bc->bits_valid)
> -        BS_FUNC(priv_skip_remaining)(bc, n);
> +        BS_FUNC(skip_remaining)(bc, n);
>      else {
>          n -= bc->bits_valid;
>          bc->bits       = 0;
> @@ -389,7 +389,7 @@ static inline void BS_FUNC(skip)(BSCTX *bc, unsigned int n)
>          }
>          BS_FUNC(priv_refill_64)(bc);
>          if (n)
> -            BS_FUNC(priv_skip_remaining)(bc, n);
> +            BS_FUNC(skip_remaining)(bc, n);
>      }
>  }
>  
> @@ -425,7 +425,7 @@ static inline int BS_FUNC(read_xbits)(BSCTX *bc, unsigned int n)
>  {
>      int32_t cache = BS_FUNC(peek)(bc, 32);
>      int sign = ~cache >> 31;
> -    BS_FUNC(priv_skip_remaining)(bc, n);
> +    BS_FUNC(skip_remaining)(bc, n);
>  
>      return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign;
>  }
> @@ -508,14 +508,14 @@ static inline int BS_FUNC(read_vlc)(BSCTX *bc, const VLCElem *table,
>      int n        = table[idx].len;
>  
>      if (max_depth > 1 && n < 0) {
> -        BS_FUNC(priv_skip_remaining)(bc, bits);
> +        BS_FUNC(skip_remaining)(bc, bits);
>          code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table);
>          if (max_depth > 2 && n < 0) {
> -            BS_FUNC(priv_skip_remaining)(bc, nb_bits);
> +            BS_FUNC(skip_remaining)(bc, nb_bits);
>              code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table);
>          }
>      }
> -    BS_FUNC(priv_skip_remaining)(bc, n);
> +    BS_FUNC(skip_remaining)(bc, n);
>  
>      return code;
>  }
> @@ -534,17 +534,17 @@ static inline int BS_FUNC(read_vlc_multi)(BSCTX *bc, uint8_t *dst,
>          code = table[idx].sym;
>          n = table[idx].len;
>          if (max_depth > 1 && n < 0) {
> -            BS_FUNC(priv_skip_remaining)(bc, bits);
> +            BS_FUNC(skip_remaining)(bc, bits);
>              code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table);
>              if (max_depth > 2 && n < 0) {
> -                BS_FUNC(priv_skip_remaining)(bc, nb_bits);
> +                BS_FUNC(skip_remaining)(bc, nb_bits);
>                  code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table);
>              }
>          }
>          AV_WN16(dst, code);
>          ret = n > 0;
>      }
> -    BS_FUNC(priv_skip_remaining)(bc, n);
> +    BS_FUNC(skip_remaining)(bc, n);
>  
>      return ret;
>  }
> diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
> index 0594e104bb..2e24632cb5 100644
> --- a/libavcodec/get_bits.h
> +++ b/libavcodec/get_bits.h
> @@ -79,6 +79,7 @@ typedef BitstreamContext GetBitContext;
>  #define get_bits_left       bits_left
>  #define skip_bits_long      bits_skip
>  #define skip_bits           bits_skip
> +#define skip_remaining      bits_skip_remaining
>  #define get_bits            bits_read_nz
>  #define get_bitsz           bits_read
>  #define get_bits_long       bits_read

Hello,

Le ven. 8 sept. 2023 à 00:39, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> a écrit :
> This is problematic, because you seem to think that bits_peek(bc, bits)
> ensures that there are at least `bits` available in the cache;

read_vlc* also makes that assumption? Anyway, I'd put that behaviour
(of checking) under (!)UNCHECKED_BITSTREAM_READER,
and effectively this is about corrupt/unsupported bitstreams. Maybe
some parts of ffmpeg have been wrong for 15 years, and that should be
done instead of expecting the reader desyncs and/or checks at the
upper level of the loop the exhaustion of the bitstream.

> https://github.com/mkver/FFmpeg/commit/fba57506a9cf6be2f4aa5eeee7b10d54729fd92a
> for a way that fixes this.

I can only notice now I neither have the time, nor am enough
interested to embark in that scrutiny of current code. I'm OK to wait
for the ffmpeg project to have decided on a solution for the specifics
you are discussing.

> the assumption that the
> combined amount of bits consumed in any get_vlc2/GET_RL_VLC/BITS_RL_VLC
> call can't exceed 32. Is this assumption actually still true now that we
> have multi-vlc stuff?

It doesn't change anything there: it operates as the first stage/level
of any get_vlc, only it can output more than 1 symbol.

> https://github.com/mkver/FFmpeg/commit/9b5a977957968c0718dea55a5b15f060ef6201dc
> and
> https://github.com/mkver/FFmpeg/commits/aligned32_le_bitstream_reader
> are probably also of interest to you.

They probably would, had I the time. My goal was really to prevent the
prores and multi-symbol from bitrotting too much, but I wasn't
expecting these roadblocks.

I'm sorry to say I'm dropping the patch series.