diff mbox

[FFmpeg-devel,v2,1/2] Move bitswap_32() into a header file.

Message ID E1cQDcM-0007Mm-8N@pannekake.samfundet.no
State Superseded
Headers show

Commit Message

Steinar H. Gunderson Jan. 8, 2017, 1:14 p.m. UTC
Allows more codecs than mpeg12video to make use of it. Rename to av_bitswap_32()
because it's a public symbol now.
---
 libavcodec/bitstream.c | 14 +++-----------
 libavcodec/mathops.h   |  8 ++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

James Almer Jan. 8, 2017, 1:52 p.m. UTC | #1
On 1/8/2017 10:14 AM, Steinar H. Gunderson wrote:
> Allows more codecs than mpeg12video to make use of it. Rename to av_bitswap_32()
> because it's a public symbol now.

mathops.h is not a public header. You can keep the bitswap_32() name or use
ff_bitswap_32() instead.

Also, when moving code in general we expect a patch that makes use of said
code as part of a set.

> ---
>  libavcodec/bitstream.c | 14 +++-----------
>  libavcodec/mathops.h   |  8 ++++++++
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
> index 6c8dca1d85..5d37430540 100644
> --- a/libavcodec/bitstream.c
> +++ b/libavcodec/bitstream.c
> @@ -128,14 +128,6 @@ static int alloc_table(VLC *vlc, int size, int use_static)
>      return index;
>  }
>  
> -static av_always_inline uint32_t bitswap_32(uint32_t x)
> -{
> -    return (uint32_t)ff_reverse[ x        & 0xFF] << 24 |
> -           (uint32_t)ff_reverse[(x >> 8)  & 0xFF] << 16 |
> -           (uint32_t)ff_reverse[(x >> 16) & 0xFF] << 8  |
> -           (uint32_t)ff_reverse[ x >> 24];
> -}
> -
>  typedef struct VLCcode {
>      uint8_t bits;
>      uint16_t symbol;
> @@ -192,7 +184,7 @@ static int build_table(VLC *vlc, int table_nb_bits, int nb_codes,
>              nb = 1 << (table_nb_bits - n);
>              inc = 1;
>              if (flags & INIT_VLC_LE) {
> -                j = bitswap_32(code);
> +                j = av_bitswap_32(code);
>                  inc = 1 << n;
>              }
>              for (k = 0; k < nb; k++) {
> @@ -225,7 +217,7 @@ static int build_table(VLC *vlc, int table_nb_bits, int nb_codes,
>                  subtable_bits = FFMAX(subtable_bits, n);
>              }
>              subtable_bits = FFMIN(subtable_bits, table_nb_bits);
> -            j = (flags & INIT_VLC_LE) ? bitswap_32(code_prefix) >> (32 - table_nb_bits) : code_prefix;
> +            j = (flags & INIT_VLC_LE) ? av_bitswap_32(code_prefix) >> (32 - table_nb_bits) : code_prefix;
>              table[j][1] = -subtable_bits;
>              ff_dlog(NULL, "%4x: n=%d (subtable)\n",
>                      j, codes[i].bits + table_nb_bits);
> @@ -325,7 +317,7 @@ int ff_init_vlc_sparse(VLC *vlc_arg, int nb_bits, int nb_codes,
>              return -1;                                                      \
>          }                                                                   \
>          if (flags & INIT_VLC_LE)                                            \
> -            buf[j].code = bitswap_32(buf[j].code);                          \
> +            buf[j].code = av_bitswap_32(buf[j].code);                       \
>          else                                                                \
>              buf[j].code <<= 32 - buf[j].bits;                               \
>          if (symbols)                                                        \
> diff --git a/libavcodec/mathops.h b/libavcodec/mathops.h
> index 5168dc2ce0..65dc8f21f1 100644
> --- a/libavcodec/mathops.h
> +++ b/libavcodec/mathops.h
> @@ -249,4 +249,12 @@ static inline int8_t ff_u8_to_s8(uint8_t a)
>      return b.s8;
>  }
>  
> +static av_always_inline uint32_t av_bitswap_32(uint32_t x)
> +{
> +    return (uint32_t)ff_reverse[ x        & 0xFF] << 24 |
> +           (uint32_t)ff_reverse[(x >> 8)  & 0xFF] << 16 |
> +           (uint32_t)ff_reverse[(x >> 16) & 0xFF] << 8  |
> +           (uint32_t)ff_reverse[ x >> 24];
> +}
> +
>  #endif /* AVCODEC_MATHOPS_H */
>
Steinar H. Gunderson Jan. 8, 2017, 1:55 p.m. UTC | #2
On Sun, Jan 08, 2017 at 10:52:52AM -0300, James Almer wrote:
> Also, when moving code in general we expect a patch that makes use of said
> code as part of a set.

I don't understand. I send a patch where I both make the move and add a
new use. Then I get told to split them into two separate patches and do so;
now I get told that they need to come together? :-)

/* Steinar */
James Almer Jan. 8, 2017, 2:01 p.m. UTC | #3
On 1/8/2017 10:55 AM, Steinar H. Gunderson wrote:
> On Sun, Jan 08, 2017 at 10:52:52AM -0300, James Almer wrote:
>> Also, when moving code in general we expect a patch that makes use of said
>> code as part of a set.
> 
> I don't understand. I send a patch where I both make the move and add a
> new use. Then I get told to split them into two separate patches and do so;
> now I get told that they need to come together? :-)
> 
> /* Steinar */

My bad, I see now this is patch 1 of 2. I didn't notice the second one
since it's on a separate thread instead of as a reply to this one. Hence
why i mentioned this first patch alone is not of much use.

Disregard that part then. Just address the name of the function.
Steinar H. Gunderson Jan. 8, 2017, 2:44 p.m. UTC | #4
On Sun, Jan 08, 2017 at 11:01:14AM -0300, James Almer wrote:
> Disregard that part then. Just address the name of the function.

Will do. Waiting with sending of a new patch to see if there are more
comments coming that I can address in the same batch.

/* Steinar */
diff mbox

Patch

diff --git a/libavcodec/bitstream.c b/libavcodec/bitstream.c
index 6c8dca1d85..5d37430540 100644
--- a/libavcodec/bitstream.c
+++ b/libavcodec/bitstream.c
@@ -128,14 +128,6 @@  static int alloc_table(VLC *vlc, int size, int use_static)
     return index;
 }
 
-static av_always_inline uint32_t bitswap_32(uint32_t x)
-{
-    return (uint32_t)ff_reverse[ x        & 0xFF] << 24 |
-           (uint32_t)ff_reverse[(x >> 8)  & 0xFF] << 16 |
-           (uint32_t)ff_reverse[(x >> 16) & 0xFF] << 8  |
-           (uint32_t)ff_reverse[ x >> 24];
-}
-
 typedef struct VLCcode {
     uint8_t bits;
     uint16_t symbol;
@@ -192,7 +184,7 @@  static int build_table(VLC *vlc, int table_nb_bits, int nb_codes,
             nb = 1 << (table_nb_bits - n);
             inc = 1;
             if (flags & INIT_VLC_LE) {
-                j = bitswap_32(code);
+                j = av_bitswap_32(code);
                 inc = 1 << n;
             }
             for (k = 0; k < nb; k++) {
@@ -225,7 +217,7 @@  static int build_table(VLC *vlc, int table_nb_bits, int nb_codes,
                 subtable_bits = FFMAX(subtable_bits, n);
             }
             subtable_bits = FFMIN(subtable_bits, table_nb_bits);
-            j = (flags & INIT_VLC_LE) ? bitswap_32(code_prefix) >> (32 - table_nb_bits) : code_prefix;
+            j = (flags & INIT_VLC_LE) ? av_bitswap_32(code_prefix) >> (32 - table_nb_bits) : code_prefix;
             table[j][1] = -subtable_bits;
             ff_dlog(NULL, "%4x: n=%d (subtable)\n",
                     j, codes[i].bits + table_nb_bits);
@@ -325,7 +317,7 @@  int ff_init_vlc_sparse(VLC *vlc_arg, int nb_bits, int nb_codes,
             return -1;                                                      \
         }                                                                   \
         if (flags & INIT_VLC_LE)                                            \
-            buf[j].code = bitswap_32(buf[j].code);                          \
+            buf[j].code = av_bitswap_32(buf[j].code);                       \
         else                                                                \
             buf[j].code <<= 32 - buf[j].bits;                               \
         if (symbols)                                                        \
diff --git a/libavcodec/mathops.h b/libavcodec/mathops.h
index 5168dc2ce0..65dc8f21f1 100644
--- a/libavcodec/mathops.h
+++ b/libavcodec/mathops.h
@@ -249,4 +249,12 @@  static inline int8_t ff_u8_to_s8(uint8_t a)
     return b.s8;
 }
 
+static av_always_inline uint32_t av_bitswap_32(uint32_t x)
+{
+    return (uint32_t)ff_reverse[ x        & 0xFF] << 24 |
+           (uint32_t)ff_reverse[(x >> 8)  & 0xFF] << 16 |
+           (uint32_t)ff_reverse[(x >> 16) & 0xFF] << 8  |
+           (uint32_t)ff_reverse[ x >> 24];
+}
+
 #endif /* AVCODEC_MATHOPS_H */