Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader_h264.cc

Index: media/base/bit_reader_h264.cc
diff --git a/media/base/bit_reader_h264.cc b/media/base/bit_reader_h264.cc
new file mode 100644
index 0000000000000000000000000000000000000000..84292afd5d18dd094f23b788538c27aa7b54916b
--- /dev/null
+++ b/media/base/bit_reader_h264.cc
@@ -0,0 +1,97 @@
+// 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_h264.h"
+
+namespace media {
+
+namespace {
+
+class H264ByteStreamProvider : public BitReaderCore::ByteStreamProvider {
+ public:
+  H264ByteStreamProvider(const uint8* data, off_t size);
+  virtual ~H264ByteStreamProvider();
+
+  // BitReaderCore::ByteStreamProvider implementation.
+  virtual int GetBytes(int min_n,
+                       int max_n,
+                      const uint8** out) OVERRIDE;
+  virtual int GetBytesLeft() const OVERRIDE;
+
+ private:
+  // Pointer to the next unread byte in the stream.
+  // Does not include bits held in the bit registers.

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

nit: Remove this comment since this code shouldn't know about the bit registers
in the core?

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

Will do.

+  const uint8* data_;
+
+  // Bytes left in the stream.
+  // Does not include bits held in the bit registers.

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

ditto?

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

Will do.

+  int bytes_left_;
+
+  // Array used to return some bytes when emulation prevention bytes

[damienv1, 2013/12/28 01:32:25]

when *some* ...

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

Done.

+  // are detected.
+  uint8 data_window_[8];
+
+  // Last two bytes read from the stream.
+  int prev_two_bytes_;
+};
+
+H264ByteStreamProvider::H264ByteStreamProvider(const uint8* data, off_t size)
+  : data_(data),
+    bytes_left_(size),
+    prev_two_bytes_(0xffff) {
+  DCHECK(data_ != NULL && bytes_left_ > 0);
+}
+
+H264ByteStreamProvider::~H264ByteStreamProvider() {
+}
+
+int H264ByteStreamProvider::GetBytes(
+    int min_nbytes, int max_nbytes, const uint8** out) {

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

nit: This implementation and the one used by BitReader don't appear to care
about min_nbytes. Consider removing it.

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

Should be able to remove it. Didn't do it right away since I didn't know if I
would use it in a near future.

+  DCHECK_LE(max_nbytes, static_cast<int>(sizeof(data_window_)));
+
+  const uint8* start = data_;
+  bool epb = false;

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

nit:s/epb/copy_to_data_window/ or something similar so the variable name is more
descriptive.

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

Done.

+
+  int nbytes = 0;
+  *out = data_;
+
+  while (nbytes < max_nbytes && bytes_left_ > 0) {
+    // Emulation prevention three-byte detection.
+    // If a sequence of 0x000003 is found, skip (ignore) the last byte (0x03)
+    // and starts copying data to |data_window_| which is used as the output.
+    if (*data_ == 0x3 && (prev_two_bytes_ & 0xffff) == 0) {
+      if (!epb && nbytes > 0)
+        memcpy(data_window_, start, nbytes);
+      epb = true;
+      *out = data_window_;
+      // Need another full three bytes before we can detect the sequence again.
+      prev_two_bytes_ = 0xffff;
+    } else {
+      if (epb)
+        data_window_[nbytes] = *data_;
+      prev_two_bytes_ = (prev_two_bytes_ << 8) | static_cast<int>(*data_);

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

If you make prev_two_bytes_ unsigned does it remove the need for a cast? If so,
please make the change to simplify the code here.

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

Will check.

+      ++nbytes;
+    }
+
+    ++data_;
+    --bytes_left_;
+  }
+
+  return nbytes;
+}
+
+int H264ByteStreamProvider::GetBytesLeft() const {
+  return bytes_left_;

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

This doesn't seem right. Shouldn't this be returning the number of unescaped
bytes. I think you might have to do a prescan of the buffer to find the number
of escape sequences and subtract that off.

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

I implemented this one to be inline with the current H264 bit reader.
However, I agree.
If we want to correctly implement it, we should remove start code emulation
prevention bytes.
Since this is costly, I would prefer having a LOG(FATAL) or sth similar since
this method should not be needed when dealing with H264 streams.

+}
+
+}  // namespace
+
+BitReaderH264::BitReaderH264(const uint8* data, off_t size)
+    : byte_stream_provider_(new H264ByteStreamProvider(data, size)),
+      bit_reader_core_(byte_stream_provider_.get()) {
+}
+
+BitReaderH264::~BitReaderH264() {}
+
+}  // namespace media