Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader.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 <algorithm>
-
 namespace media {
 
+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.

+}
+
+LinearByteStreamProvider::~LinearByteStreamProvider() {
+}
+
+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_;
+
+  *out = data_;
+  data_ += nbytes;
+  bytes_left_ -= nbytes;
+  return nbytes;
+}
+
+int LinearByteStreamProvider::GetBytesLeft() const {
+  return bytes_left_;
+}
+
+}  // namespace
+
 BitReader::BitReader(const uint8* data, off_t size)
-    : data_(data), bytes_left_(size), num_remaining_bits_in_curr_byte_(0) {
-  DCHECK(data_ != NULL && bytes_left_ > 0);
-
-  UpdateCurrByte();
+    : 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()) {
 }
 
 BitReader::~BitReader() {}
 
-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);
-}
-
-int BitReader::bits_available() const {
-  return 8 * bytes_left_ + num_remaining_bits_in_curr_byte_;
-}
-
-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;
-
-    if (num_remaining_bits_in_curr_byte_ == 0)
-      UpdateCurrByte();
-  }
-
-  return num_bits == 0;
-}
-
-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;
-}
-
 }  // namespace media