Add HE AAC support to ISO BMFF.

Add HE AAC support to ISO BMFF.

1. Parse esds box to get HE AAC config.
2. Send audio config from AAC header to decoder instead of the one embedded in SampleDescription.
3. Convert raw audio data into ADTS before sending to decoder.
4. Abstract general bit stream code from H264BitReader into media::BitReader.

BUG=134445
TEST=BitReaderTest, AACTest


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

[strobe_, 2012-06-28 14:58:22]

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

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.h:50: *out = *out * (1 << bits_to_take) +
Windows try bot: "..\media/base/bit_reader.h(50) : warning C4334: '<<' : result
of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)"

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
File media/base/bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader_unittest.cc:111: EXPECT_EQ(value64, 0x5599559955995599u);
Android try bot: this should be 0x55..99ul

[acolwell GONE FROM CHROMIUM, 2012-06-28 17:31:25]

Here are my initial comments.

In general, please prefer using << instead of * and  & with a hex mask instead
of % when doing bit operations. I think it makes it clearer what is going on.

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

https://chromiumcodereview.appspot.com/10710002/diff/26001/content/common/gpu...
content/common/gpu/media/h264_parser.cc:82: while
(num_remaining_bits_in_curr_byte_ != 0 && curr_byte_ % 2 == 0) {
nit: Since we are dealing with bits I'd prefer !(curr_byte_ & 0x1) here and
curr_byte_ >>= 1 below.

https://chromiumcodereview.appspot.com/10710002/diff/26001/content/common/gpu...
File content/common/gpu/media/h264_parser.h (left):

https://chromiumcodereview.appspot.com/10710002/diff/26001/content/common/gpu...
content/common/gpu/media/h264_parser.h:356:
DISALLOW_COPY_AND_ASSIGN(H264BitReader);
nit: You should keep the DISALLOW_COPY_AND_ASSIGN()

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

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.cc:23: void BitReader::Initialize(const void* data, off_t
size) {
nit: Use uint8* here and above so you don't need the cast.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.cc:48: uint64 dummy;
How about just using int here and adding a dummySize = sizeof(dummy) * 8; and
use that instead of 64?

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

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.h:31: // Return false on insufficient size of stream..
Remove this line since you've removed the return value.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.h:50: *out = *out * (1 << bits_to_take) +
nit: Any reason not to use "(*out << bits_to_take) |"  instead?

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.h:54: curr_byte_ %= (1 <<
num_remaining_bits_in_curr_byte_);
nit: You're dealing with bits here so I'd expect you to mask like the original
code
curr_byte_ &= (1 << num_remaining_bits_in_curr_byte_) - 1;

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader.h:62: *this = save;
Whoa.. I'm not a fan of this. Why don't you just compute whether you have enough
bits and early return before the loop? Or why can't you just use the original
implementation instead?

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
File media/base/bit_reader_unittest.cc (right):

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader_unittest.cc:15: class H264BitReader : public
media::BitReader {
Please remove this. It appears to be duplicate code copied from
content/common/gpu/media/h264_parser.h

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/base/bit_rea...
media/base/bit_reader_unittest.cc:120: TEST(BitReaderTest, H264StreamTest) {
Move this to a file in content/common/gpu/media/

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

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:11: namespace {
Remove. The media code does not use anonymous name spaces. Just make everything
in here static.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:15: static const uint32_t kFrequencyMap[] = {
Change to uint32

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:32: *size = *size * 128 + (byte & 0x7f);
Use << here

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:104: frequency_(0), channel_layout_(CHANNEL_LAYOUT_UNSUPPORTED)
{
Add 2 indent spaces.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:183: sizeof(kFrequencyMap) / sizeof(*kFrequencyMap));
used arraysize(kFrequencyMap) here instead of sizeof/sizeof

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:212: if (size <= 8192) {
reverse condition and return early so that you don't have to indent this block
of code.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:218: adts[2] = (profile_ - 1) * 64 + frequency_index_ * 4 +
channel_config_ / 4;
Please use << instead of * for all shifting and use explicit bit masks instead
of % here and below.

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/aac.cc#n...
media/mp4/aac.cc:254: bool AAC::SkipErrorSpecificConfig(BitReader* bit_reader)
const {
Remove bit_reader parameter. It isn't used by this method

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/box_defi...
File media/mp4/box_definitions.cc (right):

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/box_defi...
media/mp4/box_definitions.cc:405: RCHECK(aac.Initialize(data));
What do you think about renaming this to Parse() to be consistent with the rest
of this code?

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/mp4_stre...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10710002/diff/26001/media/mp4/mp4_stre...
media/mp4/mp4_stream_parser.cc:166: aac_ = entry.esds.aac;
What protects this code from other audio codecs being in the file like AMR-NB or
MP3?

[acolwell GONE FROM CHROMIUM, 2012-07-03 01:00:12]

LGTM % nits

http://codereview.chromium.org/10710002/diff/15010/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser.h (right):

http://codereview.chromium.org/10710002/diff/15010/content/common/gpu/media/h...
content/common/gpu/media/h264_parser.h:15: #include "base/compiler_specific.h"
Why is this needed now?

http://codereview.chromium.org/10710002/diff/15010/content/common/gpu/media/h...
File content/common/gpu/media/h264_parser_unittest.cc (left):

http://codereview.chromium.org/10710002/diff/15010/content/common/gpu/media/h...
content/common/gpu/media/h264_parser_unittest.cc:94: 
nit: remove unnecessary change.

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.cc
File media/base/bit_reader.cc (right):

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.cc#n...
media/base/bit_reader.cc:37: if (bytes_left_ >= 1) {
nit: reverse check and early return

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.h
File media/base/bit_reader.h (right):

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.h#ne...
media/base/bit_reader.h:23: // TODO(posciak): need separate unittests for this
class.
nit: Can we remove this TODO since you have created separate unit tests?

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.h#ne...
media/base/bit_reader.h:39: // enter a status that further ReadBits operations
will always return false
nit: status that -> state where

http://codereview.chromium.org/10710002/diff/15010/media/base/bit_reader.h#ne...
media/base/bit_reader.h:60: if (num_bits != 0) {
nit: Reverse check and return early

http://codereview.chromium.org/10710002/diff/15010/media/mp4/aac.cc
File media/mp4/aac.cc (right):

http://codereview.chromium.org/10710002/diff/15010/media/mp4/aac.cc#newcode152
media/mp4/aac.cc:152: if (size > 8192)
nit: Add a comment indicating what the significance of 8192. Is this a max ADTS
frame size?

http://codereview.chromium.org/10710002/diff/15010/media/mp4/aac.h
File media/mp4/aac.h (right):

http://codereview.chromium.org/10710002/diff/15010/media/mp4/aac.h#newcode39
media/mp4/aac.h:39: bool ConvertEsdsToADTS(std::vector<uint8>* buffer) const;
nit:Document return value and buffer. Is buffer an in/out parameter? What is the
state of buffer if this method returns false?

http://codereview.chromium.org/10710002/diff/15010/media/mp4/aac.h#newcode53
media/mp4/aac.h:53: // can be used by Chromium. They ara based on the AAC
specific
ara -> are

[acolwell GONE FROM CHROMIUM, 2012-07-03 16:35:08]

Looks good. Just one minor nit. Once fixed & the trybots pass, go ahead and
click the commit checkbox.

http://codereview.chromium.org/10710002/diff/29031/media/mp4/aac.cc
File media/mp4/aac.cc (right):

http://codereview.chromium.org/10710002/diff/29031/media/mp4/aac.cc#newcode153
media/mp4/aac.cc:153: if (size > (1 << 13) - 1)
nit: The comment says 11 bits, but it looks like you are actually using 12 bits
here. How come?

[commit-bot: I haz the power, 2012-07-03 21:22:45]

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

[commit-bot: I haz the power, 2012-07-03 21:22:52]

Presubmit check for 10710002-6039 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[xiaomings, 2012-07-03 22:37:31]

Adding Jam for an owners LGTM on content.

[jam, 2012-07-04 04:37:48]

lgtm for the gypi files. i don't understand this change, so i defer to acolwell
who's an owner for the rest of the content files

[commit-bot: I haz the power, 2012-07-06 20:14:06]

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

[commit-bot: I haz the power, 2012-07-06 21:59:26]

Change committed as 145617

[commit-bot: I haz the power, 2012-07-09 18:45:06]

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

[acolwell GONE FROM CHROMIUM, 2012-07-09 18:48:35]

On 2012/07/09 18:45:06, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/xiaomings%40google.com/10710002/11024

I just cancelled this because it looks like the patch was already committed? Is
that not the case?