Make H264BitReader ignore trailing zero bytes and stop bit.

Make H264BitReader ignore trailing zero bytes and stop bit.

Also make H264BitReader be able to read more than 16 bits in one go. This can be verified by H264BitReaderTest::ReadStreamWithoutEscapeAndTrailingZeroBytes.

TEST=H264BitReaderTest

[xiaomings, 2012-08-01 22:40:02]

Make H264BitReader ignore trailing zero bytes/bits and the stop bit.

For example, for a h264 NALU like 11001100 01010000 00000000. The last 12 zero
bits will be ignored. Then the last 1 bit will be treated as a stop bit that is
not included in the NALU. So the effective data of the NALU will be 11001100
010.

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

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:13: trailing_zero_bytes_(0),
data_bits_in_last_byte_(0) {
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().

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:75: *out |= (curr_byte_ <<
(bits_left - num_remaining_bits_in_curr_byte_));
Is this a serious bug fix?
(where's the test, why isn't it mentioned in the CL description, etc)

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:127: // The whole last byte will be
ignored if it is 0x80.
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?

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:46: // stop bit and any trailing zero
byte.
Why is this API reasonable, given that these bits can never be returned in
ReadBits?

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:50: bool HasMoreRBSPData() const;
This name no longer makes as much sense since you're making ReadBits really be
ReadRBSPBits.

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:84: // trailing zero bits into
account. It can never be 0.
"this doesn't take ... into account"
is pretty confusing.  I think you just want "excluding stop bit & trailing zero
bits".

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

Addressed Ami's comments. See reply inline for details.

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:13: trailing_zero_bytes_(0),
data_bits_in_last_byte_(0) {
All removed.

On 2012/08/01 23:04:00, Ami Fischman wrote:
> 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().

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:75: *out |= (curr_byte_ <<
(bits_left - num_remaining_bits_in_curr_byte_));
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:
> Is this a serious bug fix?
> (where's the test, why isn't it mentioned in the CL description, etc)

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:127: // The whole last byte will be
ignored if it is 0x80.
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:
> 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?

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:46: // stop bit and any trailing zero
byte.
This is actually used in h264_parser.cc:1081 for calculating the size of the
head. It can be replaced by a Tell() implementation. Let me know if we should
add Tell().
On 2012/08/01 23:04:00, Ami Fischman wrote:
> Why is this API reasonable, given that these bits can never be returned in
> ReadBits?

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:50: bool HasMoreRBSPData() const;
Removed.

On 2012/08/01 23:04:00, Ami Fischman wrote:
> This name no longer makes as much sense since you're making ReadBits really be
> ReadRBSPBits.

https://chromiumcodereview.appspot.com/10837058/diff/4001/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.h:84: // trailing zero bits into
account. It can never be 0.
Modified.

On 2012/08/01 23:04:00, Ami Fischman wrote:
> "this doesn't take ... into account"
> is pretty confusing.  I think you just want "excluding stop bit & trailing
zero
> bits".

[Ami GONE FROM CHROMIUM, 2012-08-02 19:49:48]

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:71: *out |= (curr_byte_ <<
(bits_left - num_remaining_bits_in_curr_byte_));
I think this bugfix belongs in its own CL.
How did the ReadBits(31) test pass before this CL?

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:119: data_bits_in_last_byte_ = 8;
l.118-125 can be replaced with:
data_bits_in_last_byte_ = 8 - ffs(last_byte);

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:126: if (data_bits_in_last_byte_ ==
0) {
I see now what you were doing before, and I think that was better than this
(esp. if you take my ffs suggestion above).  
You probably want to restore l.128-134 of patchset 2 and delete this.

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
File content/common/gpu/media/h264_parser.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_parser.cc:729: pps->transform_8x8_mode_flag = temp
!= 0;
!=0 unnecessary

[xiaomings, 2012-08-03 00:37:58]

Address CR comments.

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:71: *out |= (curr_byte_ <<
(bits_left - num_remaining_bits_in_curr_byte_));
On 2012/08/02 19:49:48, Ami Fischman wrote:
> I think this bugfix belongs in its own CL.
> How did the ReadBits(31) test pass before this CL?

Done.

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:119: data_bits_in_last_byte_ = 8;
On 2012/08/02 19:49:48, Ami Fischman wrote:
> l.118-125 can be replaced with:
> data_bits_in_last_byte_ = 8 - ffs(last_byte);

Done.

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:126: if (data_bits_in_last_byte_ ==
0) {
On 2012/08/02 19:49:48, Ami Fischman wrote:
> I see now what you were doing before, and I think that was better than this
> (esp. if you take my ffs suggestion above).  
> You probably want to restore l.128-134 of patchset 2 and delete this.

Done.

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
File content/common/gpu/media/h264_parser.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4002/content/common/gpu/...
content/common/gpu/media/h264_parser.cc:729: pps->transform_8x8_mode_flag = temp
!= 0;
On 2012/08/02 19:49:48, Ami Fischman wrote:
> !=0 unnecessary

Done.

[Ami GONE FROM CHROMIUM, 2012-08-03 05:01:32]

LGTM % posciak review

https://chromiumcodereview.appspot.com/10837058/diff/11002/content/common/gpu...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10837058/diff/11002/content/common/gpu...
content/common/gpu/media/h264_bit_reader.h:46: // stop bit and any trailing zero
byte.
s/byte/bytes/

[xiaomings, 2012-08-03 18:25:55]

Address CR comment.

https://chromiumcodereview.appspot.com/10837058/diff/11002/content/common/gpu...
File content/common/gpu/media/h264_bit_reader.h (right):

https://chromiumcodereview.appspot.com/10837058/diff/11002/content/common/gpu...
content/common/gpu/media/h264_bit_reader.h:46: // stop bit and any trailing zero
byte.
On 2012/08/03 05:01:32, Ami Fischman wrote:
> s/byte/bytes/

Done.

[Pawel Osciak, 2012-08-10 17:25:12]

lgtm % comments that don't have to be addressed atm.

https://chromiumcodereview.appspot.com/10837058/diff/4004/content/common/gpu/...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10837058/diff/4004/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:88: off_t
H264BitReader::NumBitsLeft() const {
This was assuming that there were no trailing zeros and you don't change the
behavior, but I'm starting to think that what we'd actually want is the size
without trailing zeros in case there are any.

https://chromiumcodereview.appspot.com/10837058/diff/4004/content/common/gpu/...
content/common/gpu/media/h264_bit_reader.cc:107: data_[bytes_left_ - 2] == 0x00
&& data_[bytes_left_ - 3] == 0x00) {
Is this scenario possible? Why would anyone put emulation prevention if the
zeros after it were to be treated as trailing bytes? And moreover, have more
trailing zero bytes afterwards? I don't think this is allowed by spec.
But I guess it doesn't hurt to do this.

[Ami GONE FROM CHROMIUM, 2013-10-31 21:33:13]

Time to close this CL?

[xiaomings, 2013-10-31 21:41:29]

On 2013/10/31 21:33:13, Ami Fischman wrote:
> Time to close this CL?

Yes, if we are not going to commit this.