Split the bit reader functionalities from the byte stream provider.

media/base/bit_reader_h264.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_h264.h"
+
+namespace media {
+
+// static
+const int BitReaderH264::kMaxExpGolombCodeSize = 63;
+
+BitReaderH264::BitReaderH264(const uint8* data, int size)
+    : data_(data),
+      bytes_left_(size),
+      prev_two_bytes_(0xffff),
+      bit_reader_core_(this) {
+  DCHECK(data_ != NULL);
+  DCHECK_GE(bytes_left_, 0);
+}
+
+BitReaderH264::~BitReaderH264() {}
+
+int BitReaderH264::ReadUE(uint32* out) {
+  // Get the number of leading zeros.
+  int zero_count = -1;
+  bool is_one;
+  do {
+    if (!bit_reader_core_.ReadFlag(&is_one))
+      return -1;
+    zero_count++;
+  } while(!is_one);
+
+  int code_size = 2 * zero_count + 1;
+
+  // If zero_count is greater than 31, the calculated value will overflow,
+  // just skip the bits corresponding to the exp-golomb value.
+  if (zero_count > 31) {

[damienv1, 2014/01/02 22:56:23]

Equivalent but better would be:

if (code_size > kMaxExpGolombCodeSize)

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

Done.

+    if (!bit_reader_core_.SkipBits(zero_count))
+      return -1;
+    return code_size;

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

Why don't we just return -1 here? It seems like the caller is assuming that this
should be a valid 32 bit ExpGolomb code. Anything bigger than this is likely a
bitstream error. Why allow the code to progress here?

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

My assumption here is that the bit reader should not be the one to decide
whether this is a wrong code. Likely to be a bitstream error does not mean this
is a actually a bitstream error.

+  }
+
+  // Read the actual value.
+  uint32 base = (1 << zero_count) - 1;
+  uint32 offset;
+  if (!ReadBits(zero_count, &offset))
+    return -1;
+  *out = base + offset;
+
+  return code_size;
+}
+
+bool BitReaderH264::HasMoreRBSPData() {
+  uint64 bit_reg;
+  int nbits = bit_reader_core_.PeekBitsMsbAligned(9, &bit_reg);
+
+  // 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 (bit_reg << 1) != 0;

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

Is this correct? This doesn't check that the MSb is set. This appears to check
whether there are any non-zero bits after the next available, but it does not
verify that the next bit is the last non-zero bit.

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

* Case 1, "(bit_reg << 1) != 0": in this case, this is correct, we didn't reach
the end of the RBSP data since at least the MSB is part of the RBSP data.
* Case 2, "(bit_reg << 1) == 0":
  - implicitly, the MSB will be one: in this case, this is really the end of the
RBSP data.
  - If the MSB was zero, either the application would have read too many bits
(and read the stop bit) or the stream would be ill-formed (having the last 8
bits all set to 0). In either case, this can be considered as the end of the
rbsp data.

+}
+
+int BitReaderH264::GetBytes(int max_nbytes, const uint8** out) {
+  DCHECK_LE(max_nbytes, static_cast<int>(sizeof(data_window_)));
+  DCHECK_GE(max_nbytes, 0);
+  DCHECK(out);
+
+  const uint8* start = data_;
+  bool emulation_prevention_byte_detected = false;
+
+  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 (!emulation_prevention_byte_detected && nbytes > 0)
+        memcpy(data_window_, start, nbytes);
+      emulation_prevention_byte_detected = true;
+      *out = data_window_;
+      // Need another full three bytes before we can detect the sequence again.
+      prev_two_bytes_ = 0xffff;
+    } else {
+      if (emulation_prevention_byte_detected)
+        data_window_[nbytes] = *data_;
+      prev_two_bytes_ = (prev_two_bytes_ << 8) | *data_;
+      ++nbytes;
+    }
+
+    ++data_;
+    --bytes_left_;
+  }
+
+  return nbytes;
+}
+
+int BitReaderH264::GetBytesLeft() const {
+  // Getting the actual number of bytes left would be really inefficient
+  // as this would mean parsing the rest of data
+  // for possible start code emulation prevention bytes.
+  // So this function should rather not be used in the framework of H264.
+  NOTREACHED();

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

This is a code smell. If both providers can't properly implement this method
then it should be removed from the provider interface. Perhaps bits_available()
should be moved to BitReader if you don't want to support it for H.264

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

Yes, I can do that. This is now possible since the BitReader is the one
implementing the byte stream provider.

+  return 0;
+}
+
+}  // namespace media