Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader_core.cc

+// Copyright (c) 2013 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_core.h"
+
+#include <base/sys_byteorder.h>
+
+namespace media {
+
+BitReaderCore::ByteStreamProvider::ByteStreamProvider() {
+}
+
+BitReaderCore::ByteStreamProvider::~ByteStreamProvider() {
+}
+
+// static
+const int BitReaderCore::kRegWidthInBits =
+  sizeof(BitReaderCore::RegType) * CHAR_BIT;
+
+BitReaderCore::BitReaderCore(ByteStreamProvider* byte_stream_provider)
+    : byte_stream_provider_(byte_stream_provider),
+      bit_count_(0),
+      nbits_(0),
+      reg_(0),
+      nbits_next_(0),
+      reg_next_(0) {
+}
+
+BitReaderCore::~BitReaderCore() {
+}
+
+bool BitReaderCore::ReadFlag(bool* flag) {
+  if (nbits_ == 0 && !Refill(1))
+    return false;
+
+  *flag = (static_cast<SignedRegType>(reg_) < 0);

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

Please don't do this. It obscures what the actual code is doing. Just use (reg_
& (1 << (kRegWidthInBits - 1))) != 0 or some similar construct that makes it
clear that you are just checking the MSb. Any reasonable compiler will properly
recognize this as an MSb check and generate the best instructions for it.

[damienv1, 2014/01/07 00:19:40]

Will do.

+  reg_ <<= 1;
+  nbits_--;
+  bit_count_++;
+  return true;
+}
+
+int BitReaderCore::PeekBitsMsbAligned(int num_bits, uint64* out) {
+  // Try to have at least |num_bits| in the bit register.
+  if (nbits_ < num_bits)
+    Refill(num_bits);
+
+  *out = reg_;
+  return nbits_;
+}
+
+bool BitReaderCore::SkipBits(int num_bits) {
+  DCHECK_GE(num_bits, 0);
+  DVLOG_IF(0, num_bits > 100)
+      << "BitReader::SkipBits inefficient for large skips";
+
+  uint64 dummy;
+  while (num_bits >= 64) {
+    if (!ReadBitsInternal(64, &dummy))
+      return false;
+    num_bits -= 64;
+  }
+  return ReadBitsInternal(num_bits, &dummy);
+}
+
+int BitReaderCore::GetBitCount() const {
+  return bit_count_;
+}
+
+int BitReaderCore::bits_available() const {
+  return (byte_stream_provider_->GetBytesLeft() * CHAR_BIT +
+          nbits_next_ + nbits_);
+}
+
+bool BitReaderCore::ReadBitsInternal(int num_bits, uint64* out) {
+  DCHECK_GE(num_bits, 0);
+
+  if (num_bits == 0) {
+    *out = 0;
+    return true;
+  }
+
+  if (num_bits > nbits_ && !Refill(num_bits)) {
+    // Any subsequent ReadBits should fail:
+    // empty the current bit register for that purpose.
+    nbits_ = 0;
+    reg_ = 0;
+    return false;
+  }
+
+  bit_count_ += num_bits;
+
+  if (num_bits == kRegWidthInBits) {
+    // Special case needed since for example for a 64 bit integer "a"
+    // "a << 64" is not defined by the C/C++ standard.
+    *out = reg_;
+    reg_ = 0;
+    nbits_ = 0;
+    return true;
+  }
+
+  *out = reg_ >> (kRegWidthInBits - num_bits);
+  reg_ <<= num_bits;
+  nbits_ -= num_bits;
+  return true;
+}
+
+bool BitReaderCore::Refill(int min_nbits) {
+  DCHECK_LE(min_nbits, kRegWidthInBits);
+
+  // Transfer from the next to the current register.
+  RefillCurrentRegister();
+  if (min_nbits <= nbits_)
+    return true;
+  DCHECK_EQ(nbits_next_, 0);
+  DCHECK_EQ(reg_next_, 0u);
+
+  // Max number of bytes to refill.
+  int max_nbytes = sizeof(RegType);

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

nit: How about just using sizeof(reg_next_) here since that's that you are
moving the bits into anyways?

[damienv1, 2014/01/07 00:19:40]

Will do.

+
+  // Refill.
+  const uint8* byte_stream_window;
+  int window_size =
+      byte_stream_provider_->GetBytes(max_nbytes, &byte_stream_window);
+  if (window_size == 0)
+    return false;
+
+  reg_next_ = 0;
+  memcpy(&reg_next_, byte_stream_window, window_size);
+  reg_next_ = base::NetToHost64(reg_next_);
+  nbits_next_ = window_size * CHAR_BIT;

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

Please just use 8 instead of CHAR_BIT everywhere in this file. It is highly
unlikely we'll build this code on a platform where CHAR_BIT is not 8 and I don't
think this extra level of indirection helps make the code more readable.

[damienv1, 2014/01/07 00:19:40]

I can do it although I think it's good programming practice to use this type of
constant when available. I don't think this makes the code less readable in this
specific case.

+
+  // Transfer from the next to the current register.
+  RefillCurrentRegister();
+
+  return (nbits_ >= min_nbits);
+}
+
+void BitReaderCore::RefillCurrentRegister() {
+  // No refill possible if the destination register is full
+  // or the source register is empty.
+  if (nbits_ == kRegWidthInBits || nbits_next_ == 0)
+    return;
+
+  reg_ |= (reg_next_ >> nbits_);
+
+  int free_nbits = kRegWidthInBits - nbits_;
+  if (free_nbits >= nbits_next_) {
+    nbits_ += nbits_next_;
+    reg_next_ = 0;
+    nbits_next_ = 0;
+  } else {
+    nbits_ += free_nbits;
+    reg_next_ <<= free_nbits;
+    nbits_next_ -= free_nbits;
+  }
+}
+
+}  // namespace media