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.

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

s/2013/2014

[damienv1, 2014/01/13 22:41:06]

Done.

+// 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 {
+const int kRegWidthInBits = sizeof(uint64) * 8;
+}
+
+namespace media {
+
+BitReaderCore::ByteStreamProvider::ByteStreamProvider() {
+}
+
+BitReaderCore::ByteStreamProvider::~ByteStreamProvider() {
+}
+
+BitReaderCore::BitReaderCore(ByteStreamProvider* byte_stream_provider)
+    : byte_stream_provider_(byte_stream_provider),
+      bits_read_(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 = (reg_ & (UINT64_C(1) << (kRegWidthInBits - 1))) != 0;

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

nit: I believe this should be GG_UINT64_C based on other example I've seen in
the codebase.

[damienv1, 2014/01/13 22:41:06]

Thanks, that's correct (I forgot Chromium did port these macros).

+  reg_ <<= 1;
+  nbits_--;
+  bits_read_++;
+  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 >= kRegWidthInBits) {
+    if (!ReadBitsInternal(kRegWidthInBits, &dummy))

[Pawel Osciak, 2014/01/11 02:30:49]

What do you think of instead:
if (num_bits >= kRegWidthInBits * 2)
  zero out both regs
  skip ((num_bits - existing bits in regs) % 8) bytes in provider
  read reminder

and eliminate the loop and the inefficiency altogether?

A SkipBytes/Seek method in provider could be useful.

[damienv1, 2014/01/13 22:41:06]

I agree this is more efficient.
However, I would rather leave it as an optimization that can be done in a
different CL if there are some real use cases of large skips.

+      return false;
+    num_bits -= kRegWidthInBits;
+  }
+  return ReadBitsInternal(num_bits, &dummy);
+}
+
+int BitReaderCore::bits_read() const {
+  return bits_read_;
+}
+
+int BitReaderCore::bits_buffered() const {
+  return 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;
+  }
+
+  bits_read_ += 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(reg_next_);
+
+  // Refill.
+  const uint8* byte_stream_window;
+  int window_size =
+      byte_stream_provider_->GetBytes(max_nbytes, &byte_stream_window);
+  if (window_size == 0)

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

Add the following before this line just to allow us to detect rogue providers.

DCHECK_GE(windows_size, 0);
DCHECK_LE(windows_size, max_nbytes);

[damienv1, 2014/01/13 22:41:06]

Done.

+    return false;
+
+  reg_next_ = 0;
+  memcpy(&reg_next_, byte_stream_window, window_size);
+  reg_next_ = base::NetToHost64(reg_next_);
+  nbits_next_ = window_size * 8;
+
+  // 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 {

[acolwell GONE FROM CHROMIUM, 2014/01/11 00:24:37]

nit: early return and drop else.

[damienv1, 2014/01/13 22:41:06]

Done.

+    nbits_ += free_nbits;
+    reg_next_ <<= free_nbits;

[Pawel Osciak, 2014/01/11 02:30:49]

Would this work instead of l:145-156?

free_nbits = kRegWidthInBits - nbits_;
bits_to_refill = min(free_nbits : nbits_next_);

reg_ |= (reg_next_ >> bits_to_refill);
reg_next_ <<= bits_to_refill;

nbits_ += bits_to_refill;
nbits_next_ -= bits_to_refill;

[damienv1, 2014/01/13 22:41:06]

If |bits_to_refill| is equal to 64, then it does not work:
if a is a 64 bit integer, (a << 64) or (a >> 64) is not defined by the C/C++
standard and does not return 0 as it could have been expected.

+    nbits_next_ -= free_nbits;
+  }
+}
+
+}  // namespace media