More efficient BitReader::SkipBits for large numbers of bits.

media/base/bit_reader_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/bit_reader.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
 
+static void SetBit(uint8* buf, size_t size, size_t bit_pos) {
+  size_t byte_pos = bit_pos / 8;
+  bit_pos -= byte_pos * 8;
+  DCHECK_LT(byte_pos, size);
+  buf[byte_pos] |= (1 << (7 - bit_pos));
+}
+
 TEST(BitReaderTest, NormalOperationTest) {
   uint8 value8;
   uint64 value64;
   // 0101 0101 1001 1001 repeats 4 times
   uint8 buffer[] = {0x55, 0x99, 0x55, 0x99, 0x55, 0x99, 0x55, 0x99};
   BitReader reader1(buffer, 6);  // Initialize with 6 bytes only
 
   EXPECT_TRUE(reader1.ReadBits(1, &value8));
   EXPECT_EQ(value8, 0);
   EXPECT_TRUE(reader1.ReadBits(8, &value8));
@@ skipping 36 matching lines @@
   EXPECT_TRUE(reader1.ReadBits(8, &value8));
   EXPECT_EQ(value8, 3);
   EXPECT_TRUE(reader1.SkipBits(76));
   EXPECT_TRUE(reader1.ReadBits(4, &value8));
   EXPECT_EQ(value8, 13);
   EXPECT_FALSE(reader1.SkipBits(100));
   EXPECT_TRUE(reader1.SkipBits(0));
   EXPECT_FALSE(reader1.SkipBits(1));
 }
 
+TEST(BitReaderTest, VariableSkipBitsTest) {
+  uint8 buffer[256];

[DaleCurtis, 2014/09/25 01:25:53]

uint8 buffer[256](); will zero initialize IIRC. Might be = {0} instead.

[damienv1, 2014/09/25 01:45:51]

Done.

+  for (size_t k = 0; k < arraysize(buffer); k++)

[DaleCurtis, 2014/09/25 01:25:53]

I think you can just use sizeof() here, no? Otherwise you need ARRAYSIZE_UNSAFE
since this is a local variable.

[damienv1, 2014/09/25 01:45:51]

I don't think I have to use ARRAYSIZE_UNSAFE. Here the type uint8 is not an
anonymous type.
Here is a copy of the doc:
// ARRAYSIZE_UNSAFE performs essentially the same calculation as arraysize,
// but can be used on anonymous types or types defined inside
// functions.

Did I miss sth ?

[DaleCurtis, 2014/09/25 01:50:15]

Ah, you're right.  arraysize() is fine, my mistake. 

+    buffer[k] = 0;
+
+  // The test alternates between ReadBits and SkipBits.

[DaleCurtis, 2014/09/25 01:25:53]

Add a comment about how the values were chosen (arbitrarily is fine).

[damienv1, 2014/09/25 01:45:51]

Done.

+  // The first number is the number of bits to read, the second one is the
+  // number of bits to skip.
+  size_t pattern_read_skip[][2] = {

[DaleCurtis, 2014/09/25 01:25:53]

const

[damienv1, 2014/09/25 01:45:51]

Done.

+    {  5,  17 },

[wolenetz, 2014/09/25 01:33:56]

nit: suggest adding (tiny) read+skip within sequences of < 8 bits, aligned
within and across byte boundaries.

[damienv1, 2014/09/25 01:57:47]

Done.

+    {  4,  34 },
+    { 17,  68 },
+    {  7, 102 },
+    {  9, 204 },
+    {  3, 408 } };
+
+  // Set bits to one only for the first and last bit of each read
+  // in the pattern.
+  size_t pos = 0;
+  for (size_t k = 0; k < arraysize(pattern_read_skip); k++) {

[DaleCurtis, 2014/09/25 01:25:53]

++k instead all over this test.

[DaleCurtis, 2014/09/25 01:25:53]

Ditto on arraysize.

[damienv1, 2014/09/25 01:45:51]

Done.

[damienv1, 2014/09/25 01:45:51]

I am not sure there is a convention on primitive types (I think both are
allowed). I see either case in Chrome media code. I am fine with either one.

[DaleCurtis, 2014/09/25 01:50:15]

We definitely prefer ++ for all for().  It is in the style guide too:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preincrement_...

[damienv1, 2014/09/25 01:59:29]

From this style guide:
"For simple scalar (non-object) values there is no reason to prefer one form and
we allow either. For iterators and other template types, use pre-increment."

+    SetBit(buffer, arraysize(buffer), pos);

[wolenetz, 2014/09/25 01:33:56]

nit: skip the setbits if the read is 0?

[damienv1, 2014/09/25 01:57:47]

Done.

+    SetBit(buffer, arraysize(buffer), pos + pattern_read_skip[k][0] - 1);
+    pos += pattern_read_skip[k][0] + pattern_read_skip[k][1];
+  }
+
+  // Run the test.
+  BitReader bit_reader(buffer, sizeof(buffer));

[wolenetz, 2014/09/25 01:33:56]

nit: confirm how many bits should be available?

[damienv1, 2014/09/25 01:57:47]

Done.

+  for (size_t k = 0; k < arraysize(pattern_read_skip); k++) {
+    int value;
+    EXPECT_TRUE(bit_reader.ReadBits(pattern_read_skip[k][0], &value));
+    EXPECT_EQ(value, 1 + (1 << (pattern_read_skip[k][0] - 1)));

[wolenetz, 2014/09/25 01:33:56]

nit: this will fail for read_skip pattern with read size 1. Suggest using |

[damienv1, 2014/09/25 01:57:47]

Good catch. Thanks.
Done.

+    EXPECT_TRUE(bit_reader.SkipBits(pattern_read_skip[k][1]));
+  }
+}
+
 TEST(BitReaderTest, BitsReadTest) {
   int value;
   bool flag;
   uint8 buffer[] = { 0x0a, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
   BitReader reader1(buffer, sizeof(buffer));
   EXPECT_EQ(reader1.bits_available(), 120);
 
   EXPECT_TRUE(reader1.SkipBits(2));
   EXPECT_EQ(reader1.bits_read(), 2);
   EXPECT_EQ(reader1.bits_available(), 118);
   EXPECT_TRUE(reader1.ReadBits(3, &value));
   EXPECT_EQ(reader1.bits_read(), 5);
   EXPECT_EQ(reader1.bits_available(), 115);
   EXPECT_TRUE(reader1.ReadFlag(&flag));
   EXPECT_EQ(reader1.bits_read(), 6);
   EXPECT_EQ(reader1.bits_available(), 114);
   EXPECT_TRUE(reader1.SkipBits(76));
   EXPECT_EQ(reader1.bits_read(), 82);
   EXPECT_EQ(reader1.bits_available(), 38);
 }
 
 }  // namespace media