Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader.cc

Index: media/base/bit_reader.cc
diff --git a/media/base/bit_reader.cc b/media/base/bit_reader.cc
index e4d83af74107a09a2f745bd363ff1615af6fcd36..2620c5aa7b637f2254ad56049fe95c46bfd23c45 100644
--- a/media/base/bit_reader.cc
+++ b/media/base/bit_reader.cc
@@ -4,80 +4,64 @@
 
 #include "media/base/bit_reader.h"
 
-#include <algorithm>
-
 namespace media {
 
-BitReader::BitReader(const uint8* data, off_t size)
-    : data_(data), bytes_left_(size), num_remaining_bits_in_curr_byte_(0) {
+namespace {
+
+class LinearByteStreamProvider : public BitReaderCore::ByteStreamProvider {

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

Since nothing else can access this class, is there any reason not to just make
this part of BitReader and have BitReader privately inherit from
BitReaderCore::ByteStreamProvider?

[damienv1, 2013/12/28 04:02:23]

Done.

+ public:
+  LinearByteStreamProvider(const uint8* data, off_t size);

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

nit: s/off_t/size_t/ since it is a size?

[damienv1, 2013/12/28 02:26:17]

Need to check what is best (the original code was using off_t which does not
look the best choice to me).

[damienv1, 2013/12/28 04:02:23]

Now use "int" everywhere.

+  virtual ~LinearByteStreamProvider();
+
+  // BitReaderCore::ByteStreamProvider implementation.
+  virtual int GetBytes(int min_n,
+                       int max_n,
+                      const uint8** out) OVERRIDE;
+  virtual int GetBytesLeft() const OVERRIDE;
+
+ private:
+  // Pointer to the next unread byte in the stream.
+  // Does not include bits held in the bit registers.
+  const uint8* data_;
+
+  // Bytes left in the stream.
+  // Does not include bits held in the bit registers.
+  int bytes_left_;

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

nit: This should match the type passed into the constructor. It doesn't matter
to me which one you use, but you should be consistent and not rely on implicit
casting. I'm slightly leaning towards int everywhere since your other methods
appear to be using that.

[damienv1, 2013/12/28 02:26:17]

Will do.

+};
+
+LinearByteStreamProvider::LinearByteStreamProvider(
+    const uint8* data, off_t size)
+  : data_(data),
+    bytes_left_(size) {
   DCHECK(data_ != NULL && bytes_left_ > 0);

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

nit: You should probably change this to
DCHECK(data_);
CHECK_GE(bytes_left_, 0);

since a negative bytes_left_ could result in reading invalid memory in the code
below.

[damienv1, 2013/12/28 02:26:17]

Will do.

+}
 
-  UpdateCurrByte();
+LinearByteStreamProvider::~LinearByteStreamProvider() {
 }
 
-BitReader::~BitReader() {}
+int LinearByteStreamProvider::GetBytes(
+    int min_nbytes, int max_nbytes, const uint8** out) {
+  int nbytes = max_nbytes;

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

nit: Add the following DCHECKS to clarify expectations.
DCHECK_GT(min_nbytes, 0);
DCHECK_GE(max_nbytes, min_nbytes);
DCHECK(out);

[damienv1, 2013/12/28 02:26:17]

Will do.

+  if (nbytes > bytes_left_)
+    nbytes = bytes_left_;
 
-bool BitReader::SkipBits(int num_bits) {
-  DCHECK_GE(num_bits, 0);
-  DVLOG_IF(0, num_bits > 100)
-      << "BitReader::SkipBits inefficient for large skips";
-
-  // Skip any bits in the current byte waiting to be processed, then
-  // process full bytes until less than 8 bits remaining.
-  while (num_bits > 0 && num_bits > num_remaining_bits_in_curr_byte_) {
-    num_bits -= num_remaining_bits_in_curr_byte_;
-    num_remaining_bits_in_curr_byte_ = 0;
-    UpdateCurrByte();
-
-    // If there is no more data remaining, only return true if we
-    // skipped all that were requested.
-    if (num_remaining_bits_in_curr_byte_ == 0)
-      return (num_bits == 0);
-  }
-
-  // Less than 8 bits remaining to skip. Use ReadBitsInternal to verify
-  // that the remaining bits we need exist, and adjust them as necessary
-  // for subsequent operations.
-  uint64 not_needed;
-  return ReadBitsInternal(num_bits, &not_needed);
+  *out = data_;
+  data_ += nbytes;
+  bytes_left_ -= nbytes;
+  return nbytes;
 }
 
-int BitReader::bits_available() const {
-  return 8 * bytes_left_ + num_remaining_bits_in_curr_byte_;
+int LinearByteStreamProvider::GetBytesLeft() const {
+  return bytes_left_;
 }
 
-bool BitReader::ReadBitsInternal(int num_bits, uint64* out) {
-  DCHECK_LE(num_bits, 64);
-
-  *out = 0;
-
-  while (num_remaining_bits_in_curr_byte_ != 0 && num_bits != 0) {
-    int bits_to_take = std::min(num_remaining_bits_in_curr_byte_, num_bits);
-
-    *out <<= bits_to_take;
-    *out += curr_byte_ >> (num_remaining_bits_in_curr_byte_ - bits_to_take);
-    num_bits -= bits_to_take;
-    num_remaining_bits_in_curr_byte_ -= bits_to_take;
-    curr_byte_ &= (1 << num_remaining_bits_in_curr_byte_) - 1;
+}  // namespace
 
-    if (num_remaining_bits_in_curr_byte_ == 0)
-      UpdateCurrByte();
-  }
-
-  return num_bits == 0;
+BitReader::BitReader(const uint8* data, off_t size)
+    : byte_stream_provider_(new LinearByteStreamProvider(data, size)),

[acolwell GONE FROM CHROMIUM, 2013/12/28 01:54:42]

If you don't go with my suggestion above, you should consider just making
byte_stream_provider_ a LinearByteStreamProvider instead a pointer to the
abstract interface since there is no way to actually change
byte_stream_provider_ to any other type and the lifetime is always equal to the
BitReader itself.

[damienv1, 2013/12/28 02:26:17]

But in this case, I cannot have LinearByteStreamProvider in an anonymous
namespace (need forward declaration in the header file).

[damienv1, 2013/12/28 04:02:23]

Did the private inheritance.

[acolwell GONE FROM CHROMIUM, 2014/01/06 22:44:06]

That's ok. You can just make it a private inner class instead.

+      bit_reader_core_(byte_stream_provider_.get()) {
 }
 
-void BitReader::UpdateCurrByte() {
-  DCHECK_EQ(num_remaining_bits_in_curr_byte_, 0);
-
-  if (bytes_left_ == 0)
-    return;
-
-  // Load a new byte and advance pointers.
-  curr_byte_ = *data_;
-  ++data_;
-  --bytes_left_;
-  num_remaining_bits_in_curr_byte_ = 8;
-}
+BitReader::~BitReader() {}
 
 }  // namespace media