diff mbox series

[FFmpeg-devel,4/5] lavc/bitstream: make skip_remaining() public

Message ID 20220617133206.23643-4-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/5] get_bits: move check_marker() to mpegvideodec.h | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Anton Khirnov June 17, 2022, 1:32 p.m. UTC 
Also rename it to bitstream_skip_cache(), which is more descriptive and
follows the naming conventions of tis API.
---
 libavcodec/bitstream.h          |  1 +
 libavcodec/bitstream_template.h | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

Comments

Andreas Rheinhardt June 20, 2022, 11:39 a.m. UTC | #1 
Anton Khirnov:
> Also rename it to bitstream_skip_cache(), which is more descriptive and
> follows the naming conventions of tis API.
> ---
>  libavcodec/bitstream.h          |  1 +
>  libavcodec/bitstream_template.h | 19 ++++++++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/bitstream.h b/libavcodec/bitstream.h
> index 3fa63695d3..364245453b 100644
> --- a/libavcodec/bitstream.h
> +++ b/libavcodec/bitstream.h
> @@ -116,6 +116,7 @@ static inline void bitstream_unget(BitstreamContext *bc, uint64_t value,
>  #define bitstream_peek          bitstream_peek_be
>  #define bitstream_peek_signed   bitstream_peek_signed_be
>  #define bitstream_skip          bitstream_skip_be
> +#define bitstream_skip_cache    bitstream_skip_cache_be
>  #define bitstream_seek          bitstream_seek_be
>  #define bitstream_align         bitstream_align_be
>  #define bitstream_read_xbits    bitstream_read_xbits_be
> diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h
> index 14a420139c..717473ca40 100644
> --- a/libavcodec/bitstream_template.h
> +++ b/libavcodec/bitstream_template.h
> @@ -225,7 +225,12 @@ static inline int BS_FUNC(bitstream_peek_signed)(BitstreamContext *bc, unsigned
>      return sign_extend(BS_FUNC(bitstream_peek)(bc, n), n);
>  }
>  
> -static inline void BS_FUNC(skip_remaining)(BitstreamContext *bc, unsigned int n)
> +/**
> + * Skip n bits from the cache. This may only be called if at least n bits are
> + * guaranteed to be in the cache, e.g. right after bitstream_peek(n).
> + * Otherwise use bitstream_skip().
> + */
> +static inline void BS_FUNC(bitstream_skip_cache)(BitstreamContext *bc, unsigned int n)
>  {
>  #ifdef BITSTREAM_LE
>      bc->bits >>= n;
> @@ -241,7 +246,7 @@ static inline void BS_FUNC(skip_remaining)(BitstreamContext *bc, unsigned int n)
>  static inline void BS_FUNC(bitstream_skip)(BitstreamContext *bc, unsigned int n)
>  {
>      if (n < bc->bits_left)
> -        BS_FUNC(skip_remaining)(bc, n);
> +        BS_FUNC(bitstream_skip_cache)(bc, n);
>      else {
>          n -= bc->bits_left;
>          bc->bits      = 0;
> @@ -255,7 +260,7 @@ static inline void BS_FUNC(bitstream_skip)(BitstreamContext *bc, unsigned int n)
>          }
>          BS_FUNC(refill_64)(bc);
>          if (n)
> -            BS_FUNC(skip_remaining)(bc, n);
> +            BS_FUNC(bitstream_skip_cache)(bc, n);
>      }
>  }
>  
> @@ -291,7 +296,7 @@ static inline int BS_FUNC(bitstream_read_xbits)(BitstreamContext *bc, unsigned i
>  {
>      int32_t cache = BS_FUNC(bitstream_peek)(bc, 32);
>      int sign = ~cache >> 31;
> -    BS_FUNC(skip_remaining)(bc, n);
> +    BS_FUNC(bitstream_skip_cache)(bc, n);
>  
>      return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign;
>  }
> @@ -374,14 +379,14 @@ static inline int BS_FUNC(bitstream_read_vlc)(BitstreamContext *bc, VLC_TYPE (*t
>      int n        = table[idx][1];
>  
>      if (max_depth > 1 && n < 0) {
> -        BS_FUNC(skip_remaining)(bc, bits);
> +        BS_FUNC(bitstream_skip_cache)(bc, bits);
>          code = BS_FUNC(set_idx)(bc, code, &n, &nb_bits, table);
>          if (max_depth > 2 && n < 0) {
> -            BS_FUNC(skip_remaining)(bc, nb_bits);
> +            BS_FUNC(bitstream_skip_cache)(bc, nb_bits);
>              code = BS_FUNC(set_idx)(bc, code, &n, &nb_bits, table);
>          }
>      }
> -    BS_FUNC(skip_remaining)(bc, n);
> +    BS_FUNC(bitstream_skip_cache)(bc, n);
>  
>      return code;
>  }

How do you intend to avoid the issue described here:
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297850.html?
Bitstream_peek() does not guarantee that there are bits bits in the
cache even though your documentation says otherwise.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/bitstream.h b/libavcodec/bitstream.h
index 3fa63695d3..364245453b 100644
--- a/libavcodec/bitstream.h
+++ b/libavcodec/bitstream.h
@@ -116,6 +116,7 @@  static inline void bitstream_unget(BitstreamContext *bc, uint64_t value,
 #define bitstream_peek          bitstream_peek_be
 #define bitstream_peek_signed   bitstream_peek_signed_be
 #define bitstream_skip          bitstream_skip_be
+#define bitstream_skip_cache    bitstream_skip_cache_be
 #define bitstream_seek          bitstream_seek_be
 #define bitstream_align         bitstream_align_be
 #define bitstream_read_xbits    bitstream_read_xbits_be
diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h
index 14a420139c..717473ca40 100644
--- a/libavcodec/bitstream_template.h
+++ b/libavcodec/bitstream_template.h
@@ -225,7 +225,12 @@  static inline int BS_FUNC(bitstream_peek_signed)(BitstreamContext *bc, unsigned
     return sign_extend(BS_FUNC(bitstream_peek)(bc, n), n);
 }
 
-static inline void BS_FUNC(skip_remaining)(BitstreamContext *bc, unsigned int n)
+/**
+ * Skip n bits from the cache. This may only be called if at least n bits are
+ * guaranteed to be in the cache, e.g. right after bitstream_peek(n).
+ * Otherwise use bitstream_skip().
+ */
+static inline void BS_FUNC(bitstream_skip_cache)(BitstreamContext *bc, unsigned int n)
 {
 #ifdef BITSTREAM_LE
     bc->bits >>= n;
@@ -241,7 +246,7 @@  static inline void BS_FUNC(skip_remaining)(BitstreamContext *bc, unsigned int n)
 static inline void BS_FUNC(bitstream_skip)(BitstreamContext *bc, unsigned int n)
 {
     if (n < bc->bits_left)
-        BS_FUNC(skip_remaining)(bc, n);
+        BS_FUNC(bitstream_skip_cache)(bc, n);
     else {
         n -= bc->bits_left;
         bc->bits      = 0;
@@ -255,7 +260,7 @@  static inline void BS_FUNC(bitstream_skip)(BitstreamContext *bc, unsigned int n)
         }
         BS_FUNC(refill_64)(bc);
         if (n)
-            BS_FUNC(skip_remaining)(bc, n);
+            BS_FUNC(bitstream_skip_cache)(bc, n);
     }
 }
 
@@ -291,7 +296,7 @@  static inline int BS_FUNC(bitstream_read_xbits)(BitstreamContext *bc, unsigned i
 {
     int32_t cache = BS_FUNC(bitstream_peek)(bc, 32);
     int sign = ~cache >> 31;
-    BS_FUNC(skip_remaining)(bc, n);
+    BS_FUNC(bitstream_skip_cache)(bc, n);
 
     return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign;
 }
@@ -374,14 +379,14 @@  static inline int BS_FUNC(bitstream_read_vlc)(BitstreamContext *bc, VLC_TYPE (*t
     int n        = table[idx][1];
 
     if (max_depth > 1 && n < 0) {
-        BS_FUNC(skip_remaining)(bc, bits);
+        BS_FUNC(bitstream_skip_cache)(bc, bits);
         code = BS_FUNC(set_idx)(bc, code, &n, &nb_bits, table);
         if (max_depth > 2 && n < 0) {
-            BS_FUNC(skip_remaining)(bc, nb_bits);
+            BS_FUNC(bitstream_skip_cache)(bc, nb_bits);
             code = BS_FUNC(set_idx)(bc, code, &n, &nb_bits, table);
         }
     }
-    BS_FUNC(skip_remaining)(bc, n);
+    BS_FUNC(bitstream_skip_cache)(bc, n);
 
     return code;
 }