[cast] AAC encoder for OS X and iOS based on AudioConverter and AudioFile

[cast] AAC encoder for OS X and iOS based on AudioConverter and AudioFile

BUG=417859
Committed: https://crrev.com/d5acc6893a1af79444ed2bb55929a82ba6af7c28
Cr-Commit-Position: refs/heads/master@{#301026}

[jfroy, 2014-09-25 21:35:14]

jfroy@chromium.org changed reviewers:
+ hclam@google.com, miu@chromium.org

[miu, 2014-09-29 19:54:47]

Nice work!

Needs unit tests in audio_encoder_unittest.cc, which may be as simple as adding
this after line 181:

  TEST_P(AudioEncoderTest, EncodeAac) {
    RunTestForCodec(CODEC_AUDIO_AAC);
  }

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:315: 1024, /* AudioConverter produces 1024
samples blocks */
s/AudioConverter/AacImpl/

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:338: // |TransferSamplesIntoBuffer| method.
Just a sanity-check: This callback is called synchronously, right?  Meaning, the
call to AudioConverterFillComplexBuffer() won't call this at some future point
(on the same or different thread)?

Either way, it is worth mentioning this in the comment here.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:339: static OSStatus
ConverterFillDataCallback(
style nit: static methods need to be declared/defined after instance methods.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:414: in_asbd.mBytesPerPacket =
in_asbd.mBytesPerFrame = sizeof(float);
Somehow, this doesn't seem right.  Only 4 bytes per packet?

I can understand 4 bytes per frame, if the concept used by AudioToolbox is that
one sample is the same thing as a frame.  However, more often one audio frame is
defined as a batch of samples (e.g., 480 for a 10ms frame at 48 kHz).

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:499: scoped_ptr<int8[]> cookie_data(new
int8[cookie_size]);
Sanity-check: Did you mean uint8 here, or is int8 the type provided by their
API?

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:538: AudioBufferList out_abl;
nit: Can out_abl be initialized once in Initialize(), and then re-used over and
over in this method without having to re-assign to its members?  Then, you don't
need the |max_packet_size_| member since it would only be used inside
Initialize().

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:557:
out->reserve(packet_description.mDataByteSize + 7);
Where does 7 come from?  Can you specify this as sizeof(SomeStruct)?  Otherwise,
please define an integer constant for it.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:603: const bool can_resume_;
Is this used anywhere?

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.h (right):

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.h:47: class AacAudioConverterImpl;
nit: I would prefer this class be named either AacImpl or AudioToolboxImpl. 
There is a media::AudioConverter class that we wouldn't want mixed up with the
concepts at-play here.

[jfroy, 2014-10-14 01:06:34]

Replied to some of your questions. I'll submit an updated encoder soon. Tests
are forthcoming. Been working on the producer codebase recently.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:338: // |TransferSamplesIntoBuffer| method.
On 2014/09/29 19:54:47, miu wrote:
> Just a sanity-check: This callback is called synchronously, right?  Meaning,
the
> call to AudioConverterFillComplexBuffer() won't call this at some future point
> (on the same or different thread)?
> 
> Either way, it is worth mentioning this in the comment here.

Yep. I'll add the comment.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:414: in_asbd.mBytesPerPacket =
in_asbd.mBytesPerFrame = sizeof(float);
On 2014/09/29 19:54:47, miu wrote:
> Somehow, this doesn't seem right.  Only 4 bytes per packet?
> 
> I can understand 4 bytes per frame, if the concept used by AudioToolbox is
that
> one sample is the same thing as a frame.  However, more often one audio frame
is
> defined as a batch of samples (e.g., 480 for a 10ms frame at 48 kHz).

In Core Audio, a frame of audio is one sample across all channels at a
particular timestamp. For uncompressed audio, one packet contains one frame. For
de-interleaved audio, the per-frame fields are actually per-channel (or per
buffer), *except* mChannelsPerFrame which still represents the overall channel
count (i.e. the buffer count).

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:499: scoped_ptr<int8[]> cookie_data(new
int8[cookie_size]);
On 2014/09/29 19:54:47, miu wrote:
> Sanity-check: Did you mean uint8 here, or is int8 the type provided by their
> API?

Their API use void*, which doesn't play well in C++. I've traditionally used
char/int8 in these situations. I don't interpret the data, I just hand it over
to the next API (which also takes void*).

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:538: AudioBufferList out_abl;
On 2014/09/29 19:54:47, miu wrote:
> nit: Can out_abl be initialized once in Initialize(), and then re-used over
and
> over in this method without having to re-assign to its members?  Then, you
don't
> need the |max_packet_size_| member since it would only be used inside
> Initialize().

Good idea.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:557:
out->reserve(packet_description.mDataByteSize + 7);
On 2014/09/29 19:54:47, miu wrote:
> Where does 7 come from?  Can you specify this as sizeof(SomeStruct)? 
Otherwise,
> please define an integer constant for it.

It's the ADTS header size. I've switched to a constant for this.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:603: const bool can_resume_;
On 2014/09/29 19:54:47, miu wrote:
> Is this used anywhere?

Not yet but I'll be filing a bug to add support for encoder re-initialization
after an interruption. This will be needed on certain hardware configurations.

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.h (right):

https://codereview.chromium.org/601413003/diff/20001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.h:47: class AacAudioConverterImpl;
On 2014/09/29 19:54:47, miu wrote:
> nit: I would prefer this class be named either AacImpl or AudioToolboxImpl. 
> There is a media::AudioConverter class that we wouldn't want mixed up with the
> concepts at-play here.

I've picked AppleAacImpl to emphasize that this is an Apple platform specific
implementation without requiring the reader to know that AudioToolbox is an
Apple technology / name.

[jfroy, 2014-10-18 02:02:29]

The latest patch set should be pretty good. It includes a lot of edits to
comments to document the implementation and a couple of functional changes.

1) I had to re-add the maximum access unit size member because the encoder
modifies the buffer size field of the output ABL to return-by-ref the output
size. On input it needs to be the capacity.

2) Implemented a copy elision optimization when the client buffer is exactly the
size of one AAC access unit in samples. This is a really nice win because my
provider code configures the audio pipeline to return 1024 samples buffers.

3) Added AAC to the unit tests as you suggested. It depends on issue 605803004
to work (like the rest of this issue). Every test passes.

[miu, 2014-10-22 05:05:28]

lgtm, but please address these last few little things before committing:

https://codereview.chromium.org/601413003/diff/60001/media/cast/sender/audio_...
File media/cast/sender/audio_encoder.cc (right):

https://codereview.chromium.org/601413003/diff/60001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:320: static const int AccessUnitSamples =
1024;
Chromium style: Constants start with k, so this should be kAccessUnitSamples.

https://codereview.chromium.org/601413003/diff/60001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:324: static const int AdtsHeaderSize = 7;
kAdtsHeaderSize

https://codereview.chromium.org/601413003/diff/60001/media/cast/sender/audio_...
media/cast/sender/audio_encoder.cc:612: DCHECK(*io_num_packets ==
AccessUnitSamples);
nit: DCHECK_EQ(*io_num_packets, static_cast<unsigned>(kAccessUnitSamples));

[jfroy, 2014-10-24 00:16:36]

The CQ bit was checked by jfroy@chromium.org

[jfroy, 2014-10-24 00:16:44]

The CQ bit was unchecked by jfroy@chromium.org

[jfroy, 2014-10-24 00:17:46]

Waiting for issue 605803004 to land before sending this to the CQ.

[jfroy, 2014-10-24 01:03:26]

The CQ bit was checked by jfroy@chromium.org

[commit-bot: I haz the power, 2014-10-24 01:05:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/601413003/100001

[commit-bot: I haz the power, 2014-10-24 01:54:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-24 01:54:37]

Patchset 6 (id:??) landed as
https://crrev.com/d5acc6893a1af79444ed2bb55929a82ba6af7c28
Cr-Commit-Position: refs/heads/master@{#301026}