Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader.h

Index: media/base/bit_reader.h
diff --git a/media/base/bit_reader.h b/media/base/bit_reader.h
index 8c15891c91576a6ce5e250088c71aae06e353a8d..d618604a584831d5e7243f79449075d2388f4ec5 100644
--- a/media/base/bit_reader.h
+++ b/media/base/bit_reader.h
@@ -8,67 +8,49 @@
 #include <sys/types.h>
 
 #include "base/basictypes.h"
-#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "media/base/bit_reader_core.h"
 #include "media/base/media_export.h"
 
 namespace media {
 
-// A class to read bit streams.
 class MEDIA_EXPORT BitReader {
  public:
   // Initialize the reader to start reading at |data|, |size| being size
   // of |data| in bytes.
-  BitReader(const uint8* data, off_t size);
+  BitReader(const uint8* data, off_t size,
+            bool use_h264_byte_provider = false);

[acolwell GONE FROM CHROMIUM, 2013/12/27 20:35:21]

I don't think the generic BitReader should have any knowledge of H.264. Having
this here basically defeats the purpose of factoring out the byte provider. It
seems like there should be a constructor that takes a provider and one that
doesn't and uses the linear provider by default.

[damienv1, 2013/12/27 21:23:20]

You're right, it would be better to give an instance of the
ByteStreamProvider to the BitReader constructor (instead of having this h264
flag). However, internal classes BitReaderCore::ByteStreamProvider cannot be
forward declared, and I didn't really want to expose this interface which in
theory could just be hidden from the perspective of the user of BitReader.

   ~BitReader();
 
-  // 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.
   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;
+    return bit_reader_core_.ReadBits(num_bits, out);
   }
 
-  // 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/
-  // SkipBits operations will always return false unless |num_bits| is 0.
-  bool SkipBits(int num_bits);
+  bool ReadFlag(bool* flag) {
+    return bit_reader_core_.ReadFlag(flag);
+  }
+
+  bool SkipBits(int num_bits) {
+    return bit_reader_core_.SkipBits(num_bits);
+  }
 
   // Returns the number of bits available for reading.
   int bits_available() const;
 
- private:
-  // Help function used by ReadBits to avoid inlining the bit reading logic.
-  bool ReadBitsInternal(int num_bits, uint64* out);
-
-  // Advance to the next byte, loading it into curr_byte_.
-  // If the num_remaining_bits_in_curr_byte_ is 0 after this function returns,
-  // the stream has reached the end.
-  void UpdateCurrByte();
-
-  // Pointer to the next unread (not in curr_byte_) byte in the stream.
-  const uint8* data_;
+  int GetBitCount() {
+    return bit_reader_core_.GetBitCount();
+  }
 
-  // Bytes left in the stream (without the curr_byte_).
-  off_t bytes_left_;
+  // See the definition of more_rbsp_data() in spec.
+  bool HasMoreRBSPData() {

[acolwell GONE FROM CHROMIUM, 2013/12/27 20:35:21]

This seems like a H.264 specific concept and shouldn't be in this generic
reader.

[damienv1, 2013/12/27 22:27:03]

Done.

+    return bit_reader_core_.HasMoreRBSPData();
+  }
 
-  // Contents of the current byte; first unread bit starting at position
-  // 8 - num_remaining_bits_in_curr_byte_ from MSB.
-  uint8 curr_byte_;
+ private:
+  scoped_ptr<BitReaderCore::ByteStreamProvider> byte_stream_provider_;

[acolwell GONE FROM CHROMIUM, 2013/12/27 20:35:21]

Why does this object own the provider instead of the BitReaderCore?

[damienv1, 2013/12/27 21:23:20]

The BitReader is the owner of the pipe "ByteStreamProvider --(bytes)-->
BitReaderCore --(Bits)-->"
I don't see any reason why the BitReaderCore would be the owner of the
ByteStreamProvider.

 
-  // Number of bits remaining in curr_byte_
-  int num_remaining_bits_in_curr_byte_;
+  BitReaderCore bit_reader_core_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(BitReader);
 };