Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader_core.h

+// 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.
+
+#ifndef MEDIA_BASE_BIT_READER_CORE_H_
+#define MEDIA_BASE_BIT_READER_CORE_H_
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+class MEDIA_EXPORT BitReaderCore {
+ public:
+  class ByteStreamProvider {
+   public:
+    ByteStreamProvider();
+    virtual ~ByteStreamProvider();
+
+    // Request n number of bytes where:
+    // - n must be less than or equal to |max_n|.
+    // Return the actual number of bytes available in |*array|.
+    // Note: |*array| is ensured to be valid until the next call to GetBytes.
+    virtual int GetBytes(int max_n, const uint8** array) = 0;
+
+    // Return the number of bytes left in the stream.
+    virtual int GetBytesLeft() const = 0;
+  };
+
+  // Lifetime of |byte_stream_provider| should be longer than BitReaderCore.
+  explicit BitReaderCore(ByteStreamProvider* byte_stream_provider);
+  ~BitReaderCore();
+
+  // Read |num_bits| next bits from stream and return in |*out|, first bit
+  // from the stream starting at |num_bits| position in |*out|.
+  // |num_bits| cannot be larger than the bits the type can hold.
+  // Return false if the given number of bits cannot be read (not enough
+  // bits in the stream), true otherwise. When return false, the stream will
+  // enter a state where further ReadBits/SkipBits operations will always
+  // return false unless |num_bits| is 0. The type |T| has to be a primitive
+  // integer type.
+  // T can especially not be a boolean since it generates

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

Perhaps we should just define the following so this comment isn't necessary.

bool ReadBits(int num_bits, bool* out) {
  DCHECK_EQ(num_bits, 1);
  return ReadFlag(out);
}

Either that or declare the bool specialization, but don't define it anywhere so
that a link error leads people to ReadFlag().

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

The first solution sounds good to me.

+  // some optimization warnings during compilation on windows platforms,
+  // use ReadFlag instead.
+  template<typename T> bool ReadBits(int num_bits, T* out) {
+    DCHECK_LE(num_bits, static_cast<int>(sizeof(T) * 8));
+    uint64 temp;
+    bool ret = ReadBitsInternal(num_bits, &temp);
+    *out = static_cast<T>(temp);
+    return ret;
+  }
+
+  // Read a boolean.
+  bool ReadFlag(bool* flag);
+
+  // Retrieve some bits without actually consuming them.
+  // Bits returned in |*out| are written from MSB to LSB.
+  // Return the number of bits written in |out|.
+  // Note: if the number of remaining bits is less than |num_bits|,
+  // only the remaining bits are returned and in this case, the number
+  // of bits returned is less than |num_bits|.
+  int PeekBitsMsbAligned(int num_bits, uint64* out);

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

PeekBits should expose bits the same way that ReadBits() does. Having them
shifted towards the MSb seem odd and feels out of place here.

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

Most of the time, you use PeekBits to read multiple chunks of bits. And it's
much more convenient to work with bits aligned on MSB.
=> I think it simplify code using PeekBits to get bits MSB aligned.

What I can do is provide both PeekBits and PeekBitsMsbAligned in this case. Does
it sound good to you ?

+
+  // Skip |num_bits| next bits from stream. Return false if the given number of
+  // bits cannot be skipped (not enough bits in the stream), true otherwise.
+  // When return false, the stream will enter a state where further
+  // ReadBits/ReadFlag/SkipBits operations
+  // will always return false unless |num_bits| is 0.
+  bool SkipBits(int num_bits);
+
+  // Returns the number of bits read so far.
+  int GetBitCount() const;

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

s/GetBitCount/bits_read/ since it is a simple accessor and make the name reflect
what the comment says.

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

Will do.

+
+  // Returns the number of bits available.
+  int bits_available() const;
+
+ private:
+  typedef uint64 RegType;

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

Please remove these types and just use uint64 below. The aren't really buying us
anything since there are already 64-bit assumptions sprinkled around the code.

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

OK, will do it in the end.

+  typedef int64 SignedRegType;
+  static const int kRegWidthInBits;
+
+  // Help function used by ReadBits to avoid inlining the bit reading logic.
+  bool ReadBitsInternal(int num_bits, uint64* out);
+
+  // Refill bit registers to have at least |min_nbits| bits available.
+  // Return true if the mininimum bit count condition is met after the refill.
+  bool Refill(int min_nbits);
+
+  // Refill the current bit register from the next bit register.
+  void RefillCurrentRegister();
+
+  ByteStreamProvider* const byte_stream_provider_;
+
+  // Number of bits read so far.
+  int bit_count_;
+
+  // Number of valid bits left in |reg|.
+  // Note: bits are consumed from MSB to LSB.
+  int nbits_;
+  RegType reg_;
+
+  // Number of valid bits left in |reg_next_|.
+  // Note: bits are consumed from MSB to LSB.
+  int nbits_next_;
+  RegType reg_next_;
+
+  DISALLOW_COPY_AND_ASSIGN(BitReaderCore);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_BIT_READER_CORE_H_