Make H264BitReader ignore trailing zero bytes and stop bit.

content/common/gpu/media/h264_bit_reader.cc

Index: content/common/gpu/media/h264_bit_reader.cc
diff --git a/content/common/gpu/media/h264_bit_reader.cc b/content/common/gpu/media/h264_bit_reader.cc
index f2d2f356c49bab1ddde9e866f1fbf9c14ef7e3ca..15890fc5d8b8037706f24790a44d0becf6d76908 100644
--- a/content/common/gpu/media/h264_bit_reader.cc
+++ b/content/common/gpu/media/h264_bit_reader.cc
@@ -9,19 +9,22 @@ namespace content {
 
 H264BitReader::H264BitReader()
     : data_(NULL), bytes_left_(0), curr_byte_(0),
-      num_remaining_bits_in_curr_byte_(0), prev_two_bytes_(0) {
+      num_remaining_bits_in_curr_byte_(0), prev_two_bytes_(0),
+      trailing_zero_bytes_(0), data_bits_in_last_byte_(0) {

[Ami GONE FROM CHROMIUM, 2012/08/01 23:04:00]

data_bits_in_last_byte_(0) violates the member comment in the .h.  
More generally, none of these initializers is actually necessary, b/c they are
all unconditionally overwritten in Initialize().

[xiaomings, 2012/08/01 23:39:54]

All removed.

On 2012/08/01 23:04:00, Ami Fischman wrote:

 }
 
 H264BitReader::~H264BitReader() {}
 
 bool H264BitReader::Initialize(const uint8* data, off_t size) {
   DCHECK(data);
-
-  if (size < 1)
-    return false;
+  DCHECK_GE(size, 0);
 
   data_ = data;
   bytes_left_ = size;
+
+  if (!RemoveTrailingZeroAndStopBit())
+    return false;
+
   num_remaining_bits_in_curr_byte_ = 0;
   // Initially set to 0xffff to accept all initial two-byte sequences.
   prev_two_bytes_ = 0xffff;
@@ -49,24 +52,27 @@ bool H264BitReader::UpdateCurrByte() {
   // Load a new byte and advance pointers.
   curr_byte_ = *data_++ & 0xff;
   --bytes_left_;
-  num_remaining_bits_in_curr_byte_ = 8;
+
+  if (bytes_left_ == 0) {
+    curr_byte_ >>= 8 - data_bits_in_last_byte_;
+    num_remaining_bits_in_curr_byte_ = data_bits_in_last_byte_;
+  } else {
+    num_remaining_bits_in_curr_byte_ = 8;
+  }
 
   prev_two_bytes_ = (prev_two_bytes_ << 8) | curr_byte_;
 
   return true;
 }
 
-// Read |num_bits| (1 to 31 inclusive) from the stream and return them
-// in |out|, with first bit in the stream as MSB in |out| at position
-// (|num_bits| - 1).
 bool H264BitReader::ReadBits(int num_bits, int *out) {
   int bits_left = num_bits;
   *out = 0;
-  DCHECK(num_bits <= 31);
+  DCHECK_LE(num_bits, 31);
 
   while (num_remaining_bits_in_curr_byte_ < bits_left) {
     // Take all that's left in current byte, shift to make space for the rest.
-    *out = (curr_byte_ << (bits_left - num_remaining_bits_in_curr_byte_));
+    *out |= (curr_byte_ << (bits_left - num_remaining_bits_in_curr_byte_));

[Ami GONE FROM CHROMIUM, 2012/08/01 23:04:00]

Is this a serious bug fix?
(where's the test, why isn't it mentioned in the CL description, etc)

[xiaomings, 2012/08/01 23:39:54]

It fixed a bug that H264BitReader fails to read anything longer than 16 bits.
Modified comment for that.

On 2012/08/01 23:04:00, Ami Fischman wrote:

     bits_left -= num_remaining_bits_in_curr_byte_;
 
     if (!UpdateCurrByte())
@@ -80,25 +86,60 @@ bool H264BitReader::ReadBits(int num_bits, int *out) {
   return true;
 }
 
-off_t H264BitReader::NumBitsLeft() {
-  return (num_remaining_bits_in_curr_byte_ + bytes_left_ * 8);
+off_t H264BitReader::NumBitsLeft() const {
+  off_t bits_left = num_remaining_bits_in_curr_byte_ +
+      (bytes_left_ + trailing_zero_bytes_) * 8;
+  if (bytes_left_ == 0)
+    bits_left += 8 - data_bits_in_last_byte_;
+  return bits_left;
 }
 
-bool H264BitReader::HasMoreRBSPData() {
-  // Make sure we have more bits, if we are at 0 bits in current byte
-  // and updating current byte fails, we don't have more data anyway.
-  if (num_remaining_bits_in_curr_byte_ == 0 && !UpdateCurrByte())
-      return false;
+bool H264BitReader::HasMoreRBSPData() const {
+  return num_remaining_bits_in_curr_byte_ != 0 || bytes_left_ != 0;
+}
+
+bool H264BitReader::RemoveTrailingZeroAndStopBit() {
+  // First remove all trailing zero bytes.
+  trailing_zero_bytes_ = 0;
+
+  while (bytes_left_ != 0) {
+    if (data_[bytes_left_ - 1] == 0) {
+      --bytes_left_;
+      ++trailing_zero_bytes_;
+      continue;
+    }
+    if (bytes_left_ >= 3 && data_[bytes_left_ - 1] == 0x03 &&
+        data_[bytes_left_ - 2] == 0x00 && data_[bytes_left_ - 3] == 0x00) {
+      bytes_left_ -= 3;
+      trailing_zero_bytes_ += 3;
+      continue;
+    }
+
+    break;
+  }
+
+  if (bytes_left_ == 0)
+    return false;
+
+  // Then remove the trailing zero bits of the last byte and the stop bit.
+  uint8 last_byte = data_[bytes_left_ - 1];
+
+  // The whole last byte will be ignored if it is 0x80.

[Ami GONE FROM CHROMIUM, 2012/08/01 23:04:00]

l.127-140 smell to me.  Why is 0x80 handled differently from the rest?  Why are
l.139 & 140 copies of each other?

Can you make this more straightforward?

[xiaomings, 2012/08/01 23:39:54]

0x80 is special here, as if the last bytes is 0x80, the whole bytes should be
ignored. I will address this condition in a different way.

For the copy of the same code, the first "--data_bits_in_last_byte" in the loop
is to remove all zero bits. The second one is used to remove the stop bit.
I can modify the loop condition (i.e. start from 7) to remove the duplication,
but it might be confusing. I have added comment for the stop bit.
On 2012/08/01 23:04:00, Ami Fischman wrote:

+  if (last_byte == 0x80) {
+    --bytes_left_;
+    data_bits_in_last_byte_ = 8;
+    ++trailing_zero_bytes_;
+
+    return bytes_left_ != 0;
+  }
 
-  // On last byte?
-  if (bytes_left_)
-    return true;
+  data_bits_in_last_byte_ = 8;
 
-  // 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 (curr_byte_ &
-          ((1 << (num_remaining_bits_in_curr_byte_ - 1)) - 1)) != 0;
+  while ((last_byte & (1 << (8 - data_bits_in_last_byte_))) == 0)
+    --data_bits_in_last_byte_;
+  --data_bits_in_last_byte_;
+
+  return true;
 }
 
 }  // namespace content