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() {
+}
+
+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, 2013/12/28 01:54:42]

nit: How about just using something like reg_ > kint64max instead of introducing
an extra type here just to check for MSb?

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

The comparison with 0 should be more efficient.

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

I think we should let the compiler worry about that. I'm more concerned about
readability and making it clear to the reader what is happening.

+  reg_ <<= 1;
+  nbits_--;
+  bit_count_++;
+  return true;
+}
+
+bool BitReaderCore::ReadUE(uint32* out) {
+  // Get the number of leading zeros.
+  int zero_count = -1;
+  bool is_one;
+  do {
+    if (nbits_ == 0 && !Refill(1))

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

How about this instead to avoid code duplication.

do {
  if (!ReadFlag(&is_one))
    return false;
  zero_count++;
} while (!is_one);

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

Might probably be best (even though there is a slight overhead: bit_count_ is
updated many times instead of once.

I might just go awith a different solution based on PeekBits (and might move
this method to H264 bit reader only).

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

Done the way you suggested but moved the code to the H264 bit reader only.

+      return false;
+    is_one = (static_cast<SignedRegType>(reg_) < 0);
+    reg_ <<= 1;
+    nbits_--;
+    zero_count++;
+  } while(!is_one);
+  bit_count_ += (zero_count + 1);
+
+  // If zero_count is greater than 31, the calculated value will overflow.
+  if (zero_count > 31)

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

nit: Shouldn't we put this inside the loop so we can fail as soon as we would
overflow?

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

I modified the behavior, it's now up to the caller to decide what to do in case
of an exp-golomb value overflow.

The function sticks to parsing an exp-golomb value whatever the length is.

+    return false;
+
+  // Read the actual value.
+  uint32 base = (1 << zero_count) - 1;
+  uint32 offset;
+  if (!ReadBits(zero_count, &offset))
+    return false;
+  *out = base + offset;
+
+  return true;
+}
+
+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 >= 32) {

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

nit: Why not skip 64 bits at a time since dummy is 64 bits?

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

Will do.

+    if (!ReadBitsInternal(32, &dummy))
+      return false;
+    num_bits -= 32;
+  }
+  return ReadBitsInternal(num_bits, &dummy);
+}
+
+int BitReaderCore::GetBitCount() const {
+  return bit_count_;
+}
+
+int BitReaderCore::bits_available() const {
+  return (byte_stream_provider_->GetBytesLeft() * CHAR_BIT +

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

nit: Is using CHAR_BIT here and elsewhere really buying us anything? Why not
just use 8 since we are making that assumption elsewhere in this file.

+          nbits_next_ + nbits_);
+}
+
+bool BitReaderCore::HasMoreRBSPData() {
+  // Try to have at least 9 bits (strictly more than 1 byte).
+  if (nbits_ < 9)
+    Refill(9);
+
+  // Not on last byte.
+  if (nbits_ > 8)
+    return true;
+
+  // Last byte, look for stop bit;
+  // We have more RBSP data if the last non-zero bit we find is not the
+  // first available bit.
+  return (reg_ << 1) != 0;
+}
+
+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 == sizeof(RegType) * CHAR_BIT) {
+    // 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_ >> (sizeof(RegType) * CHAR_BIT - num_bits);
+  reg_ <<= num_bits;
+  nbits_ -= num_bits;
+  return true;
+}
+
+bool BitReaderCore::Refill(int min_nbits) {
+  DCHECK_LE(min_nbits, static_cast<int>(sizeof(RegType) * CHAR_BIT));
+
+  // 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);
+
+  // Strict minimum number of bytes to refill.
+  int min_nbytes = (min_nbits - nbits_ + CHAR_BIT - 1) / CHAR_BIT;
+
+  // Max number of bytes to refill.
+  int max_nbytes = sizeof(RegType);
+
+  // Refill.
+  const uint8* byte_stream_window;
+  int window_size =
+      byte_stream_provider_->GetBytes(
+          min_nbytes, max_nbytes, &byte_stream_window);
+  if (window_size == 0)

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

Shouldn't this be window_size < min_nbytes? IIUC that would prevent this code
from doing work that will ultimately result in returning false anyways.

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

That's what I was doing originally, but Refill is currently used by
HasMoreRBSPData which requires 9 bits (but if there is less, those bits should
be *correctly* returned).

PeekBits / ViewBits will make a similar assumption.

+    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;
+
+  // Transfer from the next to the current register.
+  RefillCurrentRegister();
+
+  return (nbits_ >= min_nbits);

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

nit: if you adjust the condition above, I believe you can turn this condition
into a DCHECK_GE and always return true.

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

See explanation above.
Need the current behavior for PeekBits / HasMoreRBSPData.

+}
+
+void BitReaderCore::RefillCurrentRegister() {
+  // No refill possible if the destination register is full
+  // or the source register is empty.
+  if (nbits_ == sizeof(RegType) * CHAR_BIT || nbits_next_ == 0)
+    return;
+
+  reg_ |= (reg_next_ >> nbits_);
+
+  int free_nbits = static_cast<int>(sizeof(RegType) * CHAR_BIT) - nbits_;

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

nit: Please make a constant for static_cast<int>(sizeof(RegType) * CHAR_BIT) and
use it everywhere or just use 64.

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

Done.

+  if (free_nbits >= nbits_next_) {
+    nbits_ += nbits_next_;
+    reg_next_ = 0;
+    nbits_next_ = 0;
+  } else {

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

nit: early return and drop else.

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

Having an early return implicity involves that one case corresponds to the
exceptional behavior. Here, both cases are completely equivalent and both can
occur at roughly the same rate.

+    nbits_ += free_nbits;
+    reg_next_ <<= free_nbits;
+    nbits_next_ -= free_nbits;
+  }
+}
+
+}  // namespace media