Add HE AAC support to ISO BMFF.

Add HE AAC support to ISO BMFF.

Also abstract common code in H264BitReader into BitReader for reusing.

Was: https://chromiumcodereview.appspot.com/10710002/

In the submitted patch of the last issue, the media.gyp included a non-existing file. This error hadn't been caught by try but later got caught by the build process and reverted the commit. So I create this issue to submit the fixed patch.

BUG=134445
TEST=BitReaderTest, AACTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145769

[xiaomings, 2012-07-09 19:08:21]

Removed non-existing file from media.gyp. Need LGTM to re-submit.

[acolwell GONE FROM CHROMIUM, 2012-07-09 19:42:20]

LGTM

[jam, 2012-07-09 20:47:27]

lgtm

[commit-bot: I haz the power, 2012-07-09 21:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiaomings@google.com/10753005/1

[commit-bot: I haz the power, 2012-07-09 22:57:27]

Change committed as 145769

[Ami GONE FROM CHROMIUM, 2012-07-16 20:02:05]

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
File content/common/gpu/media/h264_parser.cc (left):

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
content/common/gpu/media/h264_parser.cc:815: if (br_.HasMoreRBSPData()) {
This broke the default test-25fps.h264 stream in
video_decode_accelerator_unittest.  I'm restoring it in
https://chromiumcodereview.appspot.com/10782019/

[Pawel Osciak, 2012-07-16 21:27:08]

Please address my comments, looks like this introduced two bugs (one mentioned
by Ami above and the other in UpdateCurrByte()) and I believe you made the bit
parsing function slower.

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
content/common/gpu/media/h264_bit_reader.cc:41: }
This is not true. There can be more than one zero-byte padding in the stream, so
this will not always work.
Also, we need to use HasMoreRBSPData() only at specific situations and doing
this every byte is suboptimal (and also incorrect).

https://chromiumcodereview.appspot.com/10753005/diff/1/media/base/bit_reader.h
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10753005/diff/1/media/base/bit_reader....
media/base/bit_reader.h:53: curr_byte_ &= (1 <<
num_remaining_bits_in_curr_byte_) - 1;
Could you please explain why this more complicated version than the original
one? This version looks like it's doing more operations than the original,
perhaps I'm missing something?

= Old version:
(2 comps + 2 subs + 1 shift) * num_bytes + 3 subs + 2 shifts + 1 AND + 1 OR
for 1 byte: 2 comps, 5 subs, 3 shifts, 1 AND, 1 OR = 12 ops
for 2 bytes: 4 comps, 7 subs, 4 shifts, 1 AND, 1 OR = 17 ops
etc.

= New version:
(4 comps + 4 subs + 3 shifts + 1 add + 1 AND) * num_bytes + 1 comp
for 1 byte: 4 comps, 4 subs, 3 shifts, 1 add, 1 AND, 1 comp = 14 ops
for 2 bytes: 8 comps, 8 subs, 6 shifts, 2 adds, 2 AND, 1 comp = 27 ops
etc.

(hopefully didn't make a mistake counting, also not counting number of registers
needed)

Old version:
bool H264Parser::H264BitReader::ReadBits(int num_bits, int *out) {
  int bits_left = num_bits;
  *out = 0;
  DCHECK(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_));
    bits_left -= num_remaining_bits_in_curr_byte_;

    if (!UpdateCurrByte())
      return false;
  }

  *out |= (curr_byte_ >> (num_remaining_bits_in_curr_byte_ - bits_left));
  *out &= ((1 << num_bits) - 1);
  num_remaining_bits_in_curr_byte_ -= bits_left;

  return true;
}

[xiaomings, 2012-07-16 21:30:38]

Hi Ami and posciak,

I am checking this issue. I am thinking a way to abstract this or maybe just
revert H264BitReader and have a simplified BitReader in /media.

[Pawel Osciak, 2012-07-16 21:30:56]

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
File content/common/gpu/media/h264_bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10753005/diff/1/content/common/gpu/med...
content/common/gpu/media/h264_bit_reader.cc:41: }
On 2012/07/16 21:27:09, posciak wrote:
> This is not true. There can be more than one zero-byte padding in the stream,
so
> this will not always work.
> Also, we need to use HasMoreRBSPData() only at specific situations and doing
> this every byte is suboptimal (and also incorrect).

Sorry, meant every last byte.