Add HE AAC support to ISO BMFF.

Add HE AAC support to ISO BMFF.

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

The original cl also includes abstraction of BitReader class used by H264Parser. It includes bugs that break the H264Parser.

In this cl I removed the abstraction and make the BitReader used by H264Parser independent to the one used by esds parser because of:

1. The original cl introduce bugs.
2. Even if we fix the bugs, we may not be able to make the generalized class as fast as the old one while keeping the abstraction.
3. The H264 bit stream use very special syntax on escaping and trailing zero bits, which might not be a good candidate for abstraction.

BUG=134445
TEST=BitReaderTest, AACTest


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

[xiaomings, 2012-07-17 19:21:11]

Please check the changes to BitReader and its unittests.

[acolwell GONE FROM CHROMIUM, 2012-07-17 20:07:32]

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:22: BitReader();
Remove this constructor.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:28: void Initialize(const uint8* data, off_t size);
Remove this and put it's body into the two argument constructor.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:39: DCHECK(num_bits <= static_cast<int>(sizeof(T) *
CHAR_BIT));
Any reason not to use 8 instead of CHAR_BIT?

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:66: bool SkipBits(int num_bits);
Add comment documenting this method's behavior and explaining the return value.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:69: off_t Tell() const;
Do we really need this method now? Only tests are using it. Is NumBitsLeft()
sufficient?

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/aac.cc
File media/mp4/aac.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/aac.cc#ne...
media/mp4/aac.cc:64: reader.Initialize(&data[0], data.size());
Remove Initialize() method and just use the 2 parameter constructor here
instead.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/es_descri...
File media/mp4/es_descriptor.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/es_descri...
media/mp4/es_descriptor.cc:51: RCHECK(static_cast<off_t>(size * CHAR_BIT) <=
reader.NumBitsLeft());
Any reason not to use 8 instead of CHAR_BIT here and below?

[xiaomings, 2012-07-17 22:09:44]

Address CR comments.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:22: BitReader();
On 2012/07/17 20:07:32, acolwell wrote:
> Remove this constructor.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:28: void Initialize(const uint8* data, off_t size);
On 2012/07/17 20:07:32, acolwell wrote:
> Remove this and put it's body into the two argument constructor.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:39: DCHECK(num_bits <= static_cast<int>(sizeof(T) *
CHAR_BIT));
Had thought a predefined MACRO might be better. But to use 8 should be enough
here and more consistent. Fixed.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:66: bool SkipBits(int num_bits);
On 2012/07/17 20:07:32, acolwell wrote:
> Add comment documenting this method's behavior and explaining the return
value.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/base/bit_read...
media/base/bit_reader.h:69: off_t Tell() const;
Removed

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/aac.cc
File media/mp4/aac.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/aac.cc#ne...
media/mp4/aac.cc:64: reader.Initialize(&data[0], data.size());
On 2012/07/17 20:07:32, acolwell wrote:
> Remove Initialize() method and just use the 2 parameter constructor here
> instead.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/es_descri...
File media/mp4/es_descriptor.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/4001/media/mp4/es_descri...
media/mp4/es_descriptor.cc:51: RCHECK(static_cast<off_t>(size * CHAR_BIT) <=
reader.NumBitsLeft());
Use 8 instead.

[acolwell GONE FROM CHROMIUM, 2012-07-17 22:38:21]

LGTM % nit

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:9: BitReader::BitReader(const uint8* data, off_t size)
{
nit: off_t -> int here and on bytes_left_

[Ami GONE FROM CHROMIUM, 2012-07-17 23:22:18]

I only reviewed the BitReader class (and a few of its callsites to understand
what's going on).

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:10: DCHECK(data != NULL || size == 0);  // Data cannot
be NULL if size is not 0.
Comment isn't adding value.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:10: DCHECK(data != NULL || size == 0);  // Data cannot
be NULL if size is not 0.
What's the value of supporting empty buffers?
I suspect all your callers could more profitably simply never create the
bitreader if the buffer is empty.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:14: num_remaining_bits_in_curr_byte_ = 0;
These three assignments belong in an initializer list

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:24: if (bytes_left_ < 1)
Can this ever be negative?  Why not ==0 for clarity?

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:36: bool ReadBits(int num_bits, T *out) {
This inlines the entire method definition at every call-site, which slows down
compilation, linking, and runtime.

Instead you could have:
template<typename T>
bool ReadBits(int num_bits, T *out) {
  DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));
  uint64 val;
  ReadBitsInternal(num_bits, &val);
  *out = val;
}
where ReadBitsInternal is *not* a templated function, and defined in the .cc
file.

Or, you could overload and have 4 explicit members for uint8/16/32/64.

Or, best of all IMO, just hard-code that T==uint64 and let the call-sites deal
with it.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:37: DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));
Replace <= with _LE

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:43: *out = (*out << bits_to_take) +
This can left-shift by the width of *out, which is undefined behavior in C++.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:57: num_remaining_bits_in_curr_byte_ = 0;
how can we get here and this isn't already the case?

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:58: bytes_left_ = 0;
ditto

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:68: bool SkipBits(int num_bits);
I question the value of this API.  Was is this more useful than using a
throw-away temporary at the callsite?

(I don't like it b/c it has such an interesting interface comment)

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:71: off_t NumBitsLeft() const;
Like SkipBits, I suspect this not the API you want.
This is only better than ReadBits() if you have client logic that needs to
branch between two different subsequent ReadBits() calls depending on the answer
to this question.  But I don't think you have such client logic.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:73: protected:
Why protected?
(esp. given no virtuals in this class, and esp. no virtual dtor)

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/mp4/aac.cc
File media/mp4/aac.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/mp4/aac.cc#n...
media/mp4/aac.cc:90: if (extension_type != 5 && reader.NumBitsLeft() >= 16) {
What does it mean if extension_type != 5 but NumBitsLeft()==15?  Isn't that a
malformed stream that should return false from this method?

[xiaomings, 2012-07-19 00:16:32]

Addressed CR comments.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:10: DCHECK(data != NULL || size == 0);  // Data cannot
be NULL if size is not 0.
On 2012/07/17 23:22:18, Ami Fischman wrote:
> Comment isn't adding value.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:10: DCHECK(data != NULL || size == 0);  // Data cannot
be NULL if size is not 0.
On 2012/07/17 23:22:18, Ami Fischman wrote:
> What's the value of supporting empty buffers?
> I suspect all your callers could more profitably simply never create the
> bitreader if the buffer is empty.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:14: num_remaining_bits_in_curr_byte_ = 0;
On 2012/07/17 23:22:18, Ami Fischman wrote:
> These three assignments belong in an initializer list

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.cc:24: if (bytes_left_ < 1)
On 2012/07/17 23:22:18, Ami Fischman wrote:
> Can this ever be negative?  Why not ==0 for clarity?

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:36: bool ReadBits(int num_bits, T *out) {
On 2012/07/17 23:22:18, Ami Fischman wrote:
> This inlines the entire method definition at every call-site, which slows down
> compilation, linking, and runtime.
> 
> Instead you could have:
> template<typename T>
> bool ReadBits(int num_bits, T *out) {
>   DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));
>   uint64 val;
>   ReadBitsInternal(num_bits, &val);
>   *out = val;
> }
> where ReadBitsInternal is *not* a templated function, and defined in the .cc
> file.
> 
> Or, you could overload and have 4 explicit members for uint8/16/32/64.
> 
> Or, best of all IMO, just hard-code that T==uint64 and let the call-sites deal
> with it.
Use ReadInternal to avoid duplicate code.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:37: DCHECK(num_bits <= static_cast<int>(sizeof(T) * 8));
On 2012/07/17 23:22:18, Ami Fischman wrote:
> Replace <= with _LE

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:43: *out = (*out << bits_to_take) +
On 2012/07/17 23:22:18, Ami Fischman wrote:
> This can left-shift by the width of *out, which is undefined behavior in C++.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:57: num_remaining_bits_in_curr_byte_ = 0;
On 2012/07/17 23:22:18, Ami Fischman wrote:
> how can we get here and this isn't already the case?

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:58: bytes_left_ = 0;
On 2012/07/17 23:22:18, Ami Fischman wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:68: bool SkipBits(int num_bits);
On 2012/07/17 23:22:18, Ami Fischman wrote:
> I question the value of this API.  Was is this more useful than using a
> throw-away temporary at the callsite?
> 
> (I don't like it b/c it has such an interesting interface comment)

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:71: off_t NumBitsLeft() const;
On 2012/07/17 23:22:18, Ami Fischman wrote:
> Like SkipBits, I suspect this not the API you want.
> This is only better than ReadBits() if you have client logic that needs to
> branch between two different subsequent ReadBits() calls depending on the
answer
> to this question.  But I don't think you have such client logic.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/base/bit_rea...
media/base/bit_reader.h:73: protected:
On 2012/07/17 23:22:18, Ami Fischman wrote:
> Why protected?
> (esp. given no virtuals in this class, and esp. no virtual dtor)

Done.

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/mp4/aac.cc
File media/mp4/aac.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/10003/media/mp4/aac.cc#n...
media/mp4/aac.cc:90: if (extension_type != 5 && reader.NumBitsLeft() >= 16) {
On 2012/07/17 23:22:18, Ami Fischman wrote:
> What does it mean if extension_type != 5 but NumBitsLeft()==15?  Isn't that a
> malformed stream that should return false from this method?
Remove the check and let the underlying code to check if it is valid.

[Ami GONE FROM CHROMIUM, 2012-07-19 00:28:58]

LGTM for BitReader.  Thanks for making these changes!

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:26: if (*out != 0)
This conditional is unnecessary.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:40: *out = 0;
I'm not sure why you want this.  It smells like housekeeping, but I worry that
it becomes part of the interface.  Given callers must inspect the bool ret
value, I'd be fine with leaving garbage in *out, too.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:41: 
extra newline

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.h:34: DCHECK_LE(num_bits, static_cast<int>(sizeof(T) *
8));
For being in the header there's a lot of space taken for what can be just:
DCHECK_LE(num_bits, static_cast<int>(sizeof(T) * 8));
uint64 temp;
bool ret = ReadBitsInternal(num_bits, &temp);
*out = static_cast<T>(temp);
return ret;

[xiaomings, 2012-07-19 01:11:01]

Address CR comments.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:26: if (*out != 0)
On 2012/07/19 00:28:59, Ami Fischman wrote:
> This conditional is unnecessary.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:40: *out = 0;
On 2012/07/19 00:28:59, Ami Fischman wrote:
> I'm not sure why you want this.  It smells like housekeeping, but I worry that
> it becomes part of the interface.  Given callers must inspect the bool ret
> value, I'd be fine with leaving garbage in *out, too.

Done.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.cc:41: 
On 2012/07/19 00:28:59, Ami Fischman wrote:
> extra newline
Removed the if.

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
File media/base/bit_reader.h (right):

https://chromiumcodereview.appspot.com/10780026/diff/24003/media/base/bit_rea...
media/base/bit_reader.h:34: DCHECK_LE(num_bits, static_cast<int>(sizeof(T) *
8));
On 2012/07/19 00:28:59, Ami Fischman wrote:
> For being in the header there's a lot of space taken for what can be just:
> DCHECK_LE(num_bits, static_cast<int>(sizeof(T) * 8));
> uint64 temp;
> bool ret = ReadBitsInternal(num_bits, &temp);
> *out = static_cast<T>(temp);
> return ret;

Done.

[Ami GONE FROM CHROMIUM, 2012-07-19 01:24:06]

BitReader still LGTM

[commit-bot: I haz the power, 2012-07-19 18:22:43]

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

[commit-bot: I haz the power, 2012-07-19 19:43:50]

Change committed as 147511