[3/3] put_bits: cache 64 bits when HAVE_FAST_64BIT

Message ID 20170228191155.13636-3-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Feb. 28, 2017, 7:11 p.m.
and fix fate-lavf-gif: sync_put_bits before writing each chunk.
Cached bits from the previous chunk get written in the next chunk.
This causes inconsistent output if put_bits caches 64 bits instead
of 32.  sync_put_bits make sure < 8 bits are still cached.
---
 libavcodec/put_bits.h | 89 +++++++++++++++++++++++++++++++++++++++------------
 libavformat/gif.c     |  1 +
 tests/ref/lavf/gif    |  2 +-
 3 files changed, 70 insertions(+), 22 deletions(-)

Comments

Luca Barbato March 1, 2017, 6:23 p.m. | #1
On 28/02/2017 20:11, John Stebbins wrote:
> and fix fate-lavf-gif: sync_put_bits before writing each chunk.
> Cached bits from the previous chunk get written in the next chunk.
> This causes inconsistent output if put_bits caches 64 bits instead
> of 32.  sync_put_bits make sure < 8 bits are still cached.
> ---
>  libavcodec/put_bits.h | 89 +++++++++++++++++++++++++++++++++++++++------------
>  libavformat/gif.c     |  1 +
>  tests/ref/lavf/gif    |  2 +-
>  3 files changed, 70 insertions(+), 22 deletions(-)
> 

Maybe you could split the fix from the feature. Looks file, is fate
happy after it?

lu
John Stebbins March 1, 2017, 9:38 p.m. | #2
On 03/01/2017 11:23 AM, Luca Barbato wrote:
> On 28/02/2017 20:11, John Stebbins wrote:
>> and fix fate-lavf-gif: sync_put_bits before writing each chunk.
>> Cached bits from the previous chunk get written in the next chunk.
>> This causes inconsistent output if put_bits caches 64 bits instead
>> of 32.  sync_put_bits make sure < 8 bits are still cached.
>> ---
>>  libavcodec/put_bits.h | 89 +++++++++++++++++++++++++++++++++++++++------------
>>  libavformat/gif.c     |  1 +
>>  tests/ref/lavf/gif    |  2 +-
>>  3 files changed, 70 insertions(+), 22 deletions(-)
>>
> Maybe you could split the fix from the feature. Looks file, is fate
> happy after it?
>
>

sure, I can do sync_put_bits and gif change first in a separate patch. that will stand on it's own.  then do 64bit
put_bits after that.
Luca Barbato March 2, 2017, 8:18 a.m. | #3
On 01/03/2017 22:38, John Stebbins wrote:
> sure, I can do sync_put_bits and gif change first in a separate patch. that will stand on it's own.  then do 64bit
> put_bits after that.

Perfect :)

Patch

diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
index 677fecd..2e93872 100644
--- a/libavcodec/put_bits.h
+++ b/libavcodec/put_bits.h
@@ -33,8 +33,24 @@ 
 #include "libavutil/common.h"
 #include "libavutil/intreadwrite.h"
 
+#if defined(HAVE_FAST_64BIT)
+#define BIT_BUF_SZ      64
+#define BIT_BUF_SHIFT   6
+#define BIT_BUF_BYTE_SZ 8
+#define BIT_BUF_TYPE    uint64_t
+#define BIT_WL          AV_WL64
+#define BIT_WB          AV_WB64
+#else
+#define BIT_BUF_SZ      32
+#define BIT_BUF_SHIFT   5
+#define BIT_BUF_BYTE_SZ 4
+#define BIT_BUF_TYPE    uint32_t
+#define BIT_WL          AV_WL32
+#define BIT_WB          AV_WB32
+#endif
+
 typedef struct PutBitContext {
-    uint32_t bit_buf;
+    BIT_BUF_TYPE bit_buf;
     int bit_left;
     uint8_t *buf, *buf_ptr, *buf_end;
     int size_in_bits;
@@ -58,7 +74,7 @@  static inline void init_put_bits(PutBitContext *s, uint8_t *buffer,
     s->buf          = buffer;
     s->buf_end      = s->buf + buffer_size;
     s->buf_ptr      = s->buf;
-    s->bit_left     = 32;
+    s->bit_left     = BIT_BUF_SZ;
     s->bit_buf      = 0;
 }
 
@@ -67,7 +83,7 @@  static inline void init_put_bits(PutBitContext *s, uint8_t *buffer,
  */
 static inline int put_bits_attempted(PutBitContext *s)
 {
-    return (s->buf_ptr - s->buf) * 8 + 32 - s->bit_left;
+    return (s->buf_ptr - s->buf) * 8 + BIT_BUF_SZ - s->bit_left;
 }
 
 /**
@@ -83,7 +99,38 @@  static inline int put_bits_count(PutBitContext *s)
  */
 static inline int put_bits_left(PutBitContext* s)
 {
-    return (s->buf_end - s->buf_ptr) * 8 - 32 + s->bit_left;
+    return (s->buf_end - s->buf_ptr) * 8 - BIT_BUF_SZ + s->bit_left;
+}
+
+/**
+ * Sync bit_buf cache to buf.
+ * May leave up to 7 bits in bit_buf.
+ */
+static inline void sync_put_bits(PutBitContext *s)
+{
+#ifndef BITSTREAM_WRITER_LE
+    if (s->bit_left < BIT_BUF_SZ)
+        s->bit_buf <<= s->bit_left;
+#endif
+    while (s->bit_left < BIT_BUF_SZ - 8) {
+        /* XXX: should test end of buffer */
+#ifdef BITSTREAM_WRITER_LE
+        if (s->buf_ptr < s->buf_end)
+            *s->buf_ptr = s->bit_buf;
+        s->buf_ptr++;
+        s->bit_buf  >>= 8;
+#else
+        if (s->buf_ptr < s->buf_end)
+            *s->buf_ptr = s->bit_buf >> (BIT_BUF_SZ - 8);
+        s->buf_ptr++;
+        s->bit_buf  <<= 8;
+#endif
+        s->bit_left  += 8;
+    }
+#ifndef BITSTREAM_WRITER_LE
+    if (s->bit_left < BIT_BUF_SZ)
+        s->bit_buf >>= s->bit_left;
+#endif
 }
 
 /**
@@ -92,10 +139,10 @@  static inline int put_bits_left(PutBitContext* s)
 static inline void flush_put_bits(PutBitContext *s)
 {
 #ifndef BITSTREAM_WRITER_LE
-    if (s->bit_left < 32)
+    if (s->bit_left < BIT_BUF_SZ)
         s->bit_buf <<= s->bit_left;
 #endif
-    while (s->bit_left < 32) {
+    while (s->bit_left < BIT_BUF_SZ) {
         /* XXX: should test end of buffer */
 #ifdef BITSTREAM_WRITER_LE
         if (s->buf_ptr < s->buf_end)
@@ -104,13 +151,13 @@  static inline void flush_put_bits(PutBitContext *s)
         s->bit_buf  >>= 8;
 #else
         if (s->buf_ptr < s->buf_end)
-            *s->buf_ptr = s->bit_buf >> 24;
+            *s->buf_ptr = s->bit_buf >> (BIT_BUF_SZ - 8);
         s->buf_ptr++;
         s->bit_buf  <<= 8;
 #endif
         s->bit_left  += 8;
     }
-    s->bit_left = 32;
+    s->bit_left = BIT_BUF_SZ;
     s->bit_buf  = 0;
 }
 
@@ -146,23 +193,23 @@  void avpriv_copy_bits(PutBitContext *pb, const uint8_t *src, int length);
  */
 static inline void put_bits(PutBitContext *s, int n, unsigned int value)
 {
-    unsigned int bit_buf;
+    BIT_BUF_TYPE bit_buf;
     int bit_left;
 
-    assert(n <= 31 && value < (1U << n));
+    assert(n <= BIT_BUF_SZ - 1 && value < (1U << n));
 
     bit_buf  = s->bit_buf;
     bit_left = s->bit_left;
 
     /* XXX: optimize */
 #ifdef BITSTREAM_WRITER_LE
-    bit_buf |= value << (32 - bit_left);
+    bit_buf |= (BIT_BUF_TYPE)value << (BIT_BUF_SZ - bit_left);
     if (n >= bit_left) {
         if (s->buf_ptr < s->buf_end)
-            AV_WL32(s->buf_ptr, bit_buf);
-        s->buf_ptr += 4;
-        bit_buf     = (bit_left == 32) ? 0 : value >> bit_left;
-        bit_left   += 32;
+            BIT_WL(s->buf_ptr, bit_buf);
+        s->buf_ptr += BIT_BUF_BYTE_SZ;
+        bit_buf     = (bit_left == BIT_BUF_SZ) ? 0 : value >> bit_left;
+        bit_left   += BIT_BUF_SZ;
     }
     bit_left -= n;
 #else
@@ -173,9 +220,9 @@  static inline void put_bits(PutBitContext *s, int n, unsigned int value)
         bit_buf   <<= bit_left;
         bit_buf    |= value >> (n - bit_left);
         if (s->buf_ptr < s->buf_end)
-            AV_WB32(s->buf_ptr, bit_buf);
-        s->buf_ptr += 4;
-        bit_left   += 32 - n;
+            BIT_WB(s->buf_ptr, bit_buf);
+        s->buf_ptr += BIT_BUF_BYTE_SZ;
+        bit_left   += BIT_BUF_SZ - n;
         bit_buf     = value;
     }
 #endif
@@ -223,7 +270,7 @@  static inline uint8_t *put_bits_ptr(PutBitContext *s)
 static inline void skip_put_bytes(PutBitContext *s, int n)
 {
     assert((put_bits_count(s) & 7) == 0);
-    assert(s->bit_left == 32);
+    assert(s->bit_left == BIT_BUF_SZ);
     s->buf_ptr += n;
 }
 
@@ -235,8 +282,8 @@  static inline void skip_put_bytes(PutBitContext *s, int n)
 static inline void skip_put_bits(PutBitContext *s, int n)
 {
     s->bit_left -= n;
-    s->buf_ptr  -= 4 * (s->bit_left >> 5);
-    s->bit_left &= 31;
+    s->buf_ptr  -= BIT_BUF_BYTE_SZ * (s->bit_left >> BIT_BUF_SHIFT);
+    s->bit_left &= BIT_BUF_SZ - 1;
 }
 
 /**
diff --git a/libavformat/gif.c b/libavformat/gif.c
index e7fa37c..43d46f3 100644
--- a/libavformat/gif.c
+++ b/libavformat/gif.c
@@ -241,6 +241,7 @@  static int gif_image_write_image(AVIOContext *pb,
             put_bits(&p, 9, 0x101); /* end of stream */
             flush_put_bits(&p);
         }
+        sync_put_bits(&p);
         if (put_bits_ptr(&p) - p.buf > 0) {
             avio_w8(pb, put_bits_ptr(&p) - p.buf); /* byte count of the packet */
             avio_write(pb, p.buf, put_bits_ptr(&p) - p.buf); /* the actual buffer */
diff --git a/tests/ref/lavf/gif b/tests/ref/lavf/gif
index 4a4ebfb..4298e18 100644
--- a/tests/ref/lavf/gif
+++ b/tests/ref/lavf/gif
@@ -1,3 +1,3 @@ 
-e6089fd4ef3b9df44090ab3650bdd810 *./tests/data/lavf/lavf.gif
+894a6f1127919153b7077fdcc7e181a9 *./tests/data/lavf/lavf.gif
 2906401 ./tests/data/lavf/lavf.gif
 ./tests/data/lavf/lavf.gif CRC=0xe5605ff6