[FFmpeg-devel] [PATCH 0/2] cached bistream: small improvements

[FFmpeg-devel] [PATCH 0/2] cached bistream: small improvements

[Christophe Gisquet]

Preparatory patch independently beneficial. Note: all of these
are for the sake of simplicity, from 2020, but needed cleaner
rebasing.

Christophe Gisquet (2):
  Expose and start using skip_remaining
  read_xbits: request fewer bits

 libavcodec/bitstream.h          |  8 +++++---
 libavcodec/bitstream_template.h | 36 +++++++++++++++++++++------------
 libavcodec/get_bits.h           |  1 +
 3 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Christophe Gisquet]

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(-)

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);                       \
                                                             \
             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
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] read_xbits: request fewer bits

[Christophe Gisquet]

This would have also helped a bitstream reader with a cache
of 32 bits.
---
 libavcodec/bitstream_template.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h
index 3f90fc6a07..c27e8108b2 100644
--- a/libavcodec/bitstream_template.h
+++ b/libavcodec/bitstream_template.h
@@ -423,8 +423,18 @@ static inline const uint8_t *BS_FUNC(align)(BSCTX *bc)
  */
 static inline int BS_FUNC(read_xbits)(BSCTX *bc, unsigned int n)
 {
-    int32_t cache = BS_FUNC(peek)(bc, 32);
-    int sign = ~cache >> 31;
+    int32_t cache;
+    int sign;
+
+    if (n > bc->bits_valid)
+        BS_FUNC(priv_refill_32)(bc);
+
+#if defined(BITSTREAM_READER_LE)
+    cache = bc->bits & 0xFFFFFFFF;
+#else
+    cache = bc->bits >> 32;
+#endif
+    sign = ~cache >> 31;
     BS_FUNC(skip_remaining)(bc, n);
 
     return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign;
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] Expose and start using skip_remaining

[Andreas Rheinhardt]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] read_xbits: request fewer bits

[Andreas Rheinhardt]

Christophe Gisquet:
> This would have also helped a bitstream reader with a cache
> of 32 bits.
> ---
>  libavcodec/bitstream_template.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h
> index 3f90fc6a07..c27e8108b2 100644
> --- a/libavcodec/bitstream_template.h
> +++ b/libavcodec/bitstream_template.h
> @@ -423,8 +423,18 @@ static inline const uint8_t *BS_FUNC(align)(BSCTX *bc)
>   */
>  static inline int BS_FUNC(read_xbits)(BSCTX *bc, unsigned int n)
>  {
> -    int32_t cache = BS_FUNC(peek)(bc, 32);
> -    int sign = ~cache >> 31;
> +    int32_t cache;
> +    int sign;
> +
> +    if (n > bc->bits_valid)
> +        BS_FUNC(priv_refill_32)(bc);
> +
> +#if defined(BITSTREAM_READER_LE)
> +    cache = bc->bits & 0xFFFFFFFF;
> +#else
> +    cache = bc->bits >> 32;
> +#endif
> +    sign = ~cache >> 31;
>      BS_FUNC(skip_remaining)(bc, n);
>  
>      return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign;

Great, this function has the same issue at the end of input as read_vlc.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] Expose and start using skip_remaining

[Christophe Gisquet]

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.
-- 
Christophe
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".