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

media/base/bit_reader_unittest.cc

Index: media/base/bit_reader_unittest.cc
diff --git a/media/base/bit_reader_unittest.cc b/media/base/bit_reader_unittest.cc
index b6edd491a63733f1785ebd947cd9caf86d5d5783..af091a3b79a2d1152362012abdf963f9162a2f70 100644
--- a/media/base/bit_reader_unittest.cc
+++ b/media/base/bit_reader_unittest.cc
@@ -8,6 +8,13 @@
 
 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;
@@ -64,6 +71,41 @@ TEST(BitReaderTest, SkipBitsTest) {
   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;