[FFmpeg-devel] [PATCH 1/2] Expose and start using skip_remaining
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Fri Sep 8 01:40:30 EEST 2023
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
More information about the ffmpeg-devel
mailing list