Use passthrough decoder for (E)AC3 formats

Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2572573007
Cr-Commit-Position: refs/heads/master@{#471893}
Committed: https://chromium.googlesource.com/chromium/src/+/4131b4cb048807c5651f917bc77669545128ff5b

[AndyWu, 2016-12-13 23:53:47]

Description was changed from

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
==========

to

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[AndyWu, 2016-12-13 23:58:51]

Description was changed from

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[AndyWu, 2016-12-13 23:58:51]

tsunghung@chromium.org changed reviewers:
+ chcunningham@chromium.org, dalecurtis@chromium.org, halliwell@chromium.org,
sanfin@chromium.org, timav@chromium.org, xhwang@chromium.org

[DaleCurtis, 2016-12-14 00:47:35]

Won't have time to get to this this week. Have a couple days next week I'll use
to look over this.

[DaleCurtis, 2016-12-14 20:00:04]

Had some time for comments, AudioBuffer and Ac3Util both need unittests written.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc
File media/base/ac3_util.cc (right):

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
Probably want to use a BitReader here instead. SyncFrame instead of syncframe.
Also some of the constant names are weird case and abbreviation, don't mix the
spec names in with the constants.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:45: while (current < end - 1) {
You can scan two bytes at a time by check current[1] if it's 0x0B and skipping
two ahead otherwise.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:109: int bitrate =
kBitrateByHalfFrmsizecod[half_frmsizecod];
Don't use else blocks with return.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.h
File media/base/ac3_util.h (right):

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.h#n...
media/base/ac3_util.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
Should go in media/formats/ probably.

[AndyWu, 2016-12-15 22:20:03]

I will check BitReader and update again.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc
File media/base/ac3_util.cc (right):

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/12/14 20:00:03, DaleCurtis_OOO_Dec14_Jan6 wrote:
> Probably want to use a BitReader here instead. SyncFrame instead of syncframe.
> Also some of the constant names are weird case and abbreviation, don't mix the
> spec names in with the constants.

Done, thanks. I still keep the weird abbreviations in local variables to match
the spec. Let me know if you think I should rename them, too.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:45: while (current < end - 1) {
On 2016/12/14 20:00:03, DaleCurtis_OOO_Dec14_Jan6 wrote:
> You can scan two bytes at a time by check current[1] if it's 0x0B and skipping
> two ahead otherwise.

Done.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.cc#...
media/base/ac3_util.cc:109: int bitrate =
kBitrateByHalfFrmsizecod[half_frmsizecod];
On 2016/12/14 20:00:03, DaleCurtis_OOO_Dec14_Jan6 wrote:
> Don't use else blocks with return.

Done.

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.h
File media/base/ac3_util.h (right):

https://codereview.chromium.org/2572573007/diff/40001/media/base/ac3_util.h#n...
media/base/ac3_util.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/12/14 20:00:03, DaleCurtis_OOO_Dec14_Jan6 wrote:
> Should go in media/formats/ probably.

Done.

[chcunningham, 2016-12-16 22:03:14]

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer...
media/base/audio_buffer.cc:83: if (!IsBitstream(sample_format))
I feel like the interleaved code path is the only one that makes sense for the
compressed bitstream. I'm not sure what sample formats are available in AC3, but
lets say the format is planar instead of interleaved. I expect that the attempt
to copy specific channel data based on standard PCM planar layout will go
horribly wrong for encoded AC3? If thats right, we want to re-work this to
always skip to  interleaved setup, even if the layout is actually planar once
decoded.

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer...
media/base/audio_buffer.cc:124: CHECK(data[0]);
Are we confident that data[0] would never return 0 for a bitstream format? Never
any leading padding? For all bistream formats that we might encounter now/later?

https://codereview.chromium.org/2572573007/diff/80001/media/filters/android/m...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2572573007/diff/80001/media/filters/android/m...
media/filters/android/media_codec_audio_decoder.cc:71: if (config.codec() ==
kCodecAC3) {
You make the is_passthrough_ decision in two places. 1st is here, second is in
sdk_media_codec_bridge when you convert the (E)AC3 mimetypes to "raw". You might
try centralize that decision to keep everything in sync.

[AndyWu, 2017-05-02 23:41:03]

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer...
media/base/audio_buffer.cc:83: if (!IsBitstream(sample_format))
I do make (E)AC3 as interleaved formats:
https://cs.chromium.org/chromium/src/media/base/sample_format.cc?l=91


On 2016/12/16 22:03:14, chcunningham wrote:
> I feel like the interleaved code path is the only one that makes sense for the
> compressed bitstream. I'm not sure what sample formats are available in AC3,
but
> lets say the format is planar instead of interleaved. I expect that the
attempt
> to copy specific channel data based on standard PCM planar layout will go
> horribly wrong for encoded AC3? If thats right, we want to re-work this to
> always skip to  interleaved setup, even if the layout is actually planar once
> decoded.

https://codereview.chromium.org/2572573007/diff/80001/media/base/audio_buffer...
media/base/audio_buffer.cc:124: CHECK(data[0]);
On 2016/12/16 22:03:14, chcunningham wrote:
> Are we confident that data[0] would never return 0 for a bitstream format?
Never
> any leading padding? For all bistream formats that we might encounter
now/later?

data[0] is a pointer; null pointer is unexpected.

https://codereview.chromium.org/2572573007/diff/80001/media/filters/android/m...
File media/filters/android/media_codec_audio_decoder.cc (right):

https://codereview.chromium.org/2572573007/diff/80001/media/filters/android/m...
media/filters/android/media_codec_audio_decoder.cc:71: if (config.codec() ==
kCodecAC3) {
Done, thanks for your comment.

On 2016/12/16 22:03:14, chcunningham wrote:
> You make the is_passthrough_ decision in two places. 1st is here, second is in
> sdk_media_codec_bridge when you convert the (E)AC3 mimetypes to "raw". You
might
> try centralize that decision to keep everything in sync.

[chcunningham, 2017-05-04 18:59:05]

Sorry for the delay, will take a look at this today.

[DaleCurtis, 2017-05-04 19:05:44]

Mostly lg2m needs unittests for new AudioBuffer methods. Is this enabled in this
CL? I think we're still missing the implementation in the AudioManagerAndroid,
right?

https://codereview.chromium.org/2572573007/diff/120001/media/base/android/med...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/android/med...
media/base/android/media_codec_util.cc:369: return (codec == kCodecAC3 || codec
== kCodecEAC3);
Drop unnecessary parens.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.h:156: // Return the sample format.
Can we drop this? And instead return IsBitstreamFormat()?  I don't want clients
thinking they can know the sample format for AudioBuffers, they are expected to
use Read() irrespective of the format.

[chcunningham, 2017-05-05 01:14:12]

Generally looks good. I need to take more time with the ac3 utils.


> I think we're still missing the implementation in the AudioManagerAndroid,
right?

+1 - I'm not clear on what happens browser-side.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:82: // Planar data, so need to allocate buffer for
each channel.
DCHECK(!IsBitstreamFormat(...))

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:113: data_size_ = data_size_per_channel *
channel_count_;
else DCHECK(data_size_ > 0)?

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:246:
dest->set_is_compressed_format(is_compressed_format);
is_compressed_format is slightly different name from IsBitstream. Can you use
one or the other everywhere?

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:323: DCHECK(!IsBitstream(sample_format_));
These methods are called in frame_processor as part of append window trimming.
Remind me, what is the plan for that API? It obviously won't work, but AFIACT
people aren't prevented from trying to use it (and doing so will just mangle the
data). Perhaps we should turn this DCHECK into a LOG(ERROR) and make it a no-op?

[chcunningham, 2017-05-05 18:57:19]

https://codereview.chromium.org/2572573007/diff/120001/media/formats/ac3/ac3_...
File media/formats/ac3/ac3_util.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/formats/ac3/ac3_...
media/formats/ac3/ac3_util.cc:135: return 2 *
kSyncFrameSizeInWordsFor44kHz[Ac3Header.ac3_frame_size_code()];
Can you document where the 2, 6, and 4 (LHS of return multiplications) come
from? I assume this is in some table defined in the spec, so just reference that
spec version and table number

[AndyWu, 2017-05-06 01:46:48]

Thanks a lot for the review. :)
Yes, this CL is only part of the whole change. The complete change can be found
here: https://codereview.chromium.org/2466463005/#ps240001. I am trying to
upstream piece by piece to make code review easier.

https://codereview.chromium.org/2572573007/diff/120001/media/base/android/med...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/android/med...
media/base/android/media_codec_util.cc:369: return (codec == kCodecAC3 || codec
== kCodecEAC3);
On 2017/05/04 19:05:44, DaleCurtis_OOO_May_5_To_May23 wrote:
> Drop unnecessary parens.

Done.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:82: // Planar data, so need to allocate buffer for
each channel.
On 2017/05/05 01:14:12, chcunningham wrote:
> DCHECK(!IsBitstreamFormat(...))

Done.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:113: data_size_ = data_size_per_channel *
channel_count_;
On 2017/05/05 01:14:12, chcunningham wrote:
> else DCHECK(data_size_ > 0)?

Done.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:246:
dest->set_is_compressed_format(is_compressed_format);
On 2017/05/05 01:14:11, chcunningham wrote:
> is_compressed_format is slightly different name from IsBitstream. Can you use
> one or the other everywhere? 

Sorry, this change should not be included in this CL. Will implement it along
with AudioBus changes.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:323: DCHECK(!IsBitstream(sample_format_));
On 2017/05/05 01:14:11, chcunningham wrote:
> These methods are called in frame_processor as part of append window trimming.
> Remind me, what is the plan for that API? It obviously won't work, but AFIACT
> people aren't prevented from trying to use it (and doing so will just mangle
the
> data). Perhaps we should turn this DCHECK into a LOG(ERROR) and make it a
no-op?

Done. However, we really should prevent trimming calls on bitstream format.

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.h:156: // Return the sample format.
On 2017/05/04 19:05:44, DaleCurtis_OOO_May_5_To_May23 wrote:
> Can we drop this? And instead return IsBitstreamFormat()?  I don't want
clients
> thinking they can know the sample format for AudioBuffers, they are expected
to
> use Read() irrespective of the format.

Done, you are right, IsBitstreamFormat() is enough.

https://codereview.chromium.org/2572573007/diff/120001/media/formats/ac3/ac3_...
File media/formats/ac3/ac3_util.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/formats/ac3/ac3_...
media/formats/ac3/ac3_util.cc:135: return 2 *
kSyncFrameSizeInWordsFor44kHz[Ac3Header.ac3_frame_size_code()];
On 2017/05/05 18:57:18, chcunningham wrote:
> Can you document where the 2, 6, and 4 (LHS of return multiplications) come
> from? I assume this is in some table defined in the spec, so just reference
that
> spec version and table number 

Done.

[chcunningham, 2017-05-08 20:01:02]

chcunningham@chromium.org changed reviewers:
+ wolenetz@chromium.org

[chcunningham, 2017-05-08 20:01:02]

+wolenetz - FYI on no-op appendWindow trimming for AC3 (see comments in
AudioBuffer).

LGTM

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/120001/media/base/audio_buffe...
media/base/audio_buffer.cc:323: DCHECK(!IsBitstream(sample_format_));
They current MediaSource API doesn't really afford us a way to disable that
feature (other than internally making it a no-op). 

Perhaps (E)AC3 should be registered among the other MSE codecs and in its
registry documentation we can include a disclaimer that appendWindowStart/End
may not be supported. 

None of this blocks landing.

[AndyWu, 2017-05-08 20:18:52]

The CQ bit was checked by tsunghung@chromium.org

[commit-bot: I haz the power, 2017-05-08 20:20:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 20:23:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 20:23:45]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[AndyWu, 2017-05-08 21:11:29]

The CQ bit was checked by tsunghung@chromium.org

[AndyWu, 2017-05-08 21:11:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2572573007/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-08 21:12:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 21:33:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 21:33:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[AndyWu, 2017-05-08 21:38:22]

tsunghung@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2017-05-08 21:50:18]

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:166: CHECK_GT(frame_count, 0);  // Otherwise looks
like an EOF buffer.
Which process can this run in?  If it's the browser, then we don't want to
crash.

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:168: return make_scoped_refptr(new AudioBuffer(
How trusting is the audio_buffer class about having enough data to satisify the
constraints imposed by the other parameters?  Since these are now coming across
IPC, we'd probably need to validate them.

[AndyWu, 2017-05-08 22:51:02]

chcunningham@, any suggestions?

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:166: CHECK_GT(frame_count, 0);  // Otherwise looks
like an EOF buffer.
On 2017/05/08 21:50:18, Tom Sepez wrote:
> Which process can this run in?  If it's the browser, then we don't want to
> crash.

Yes, it can run in the browser process. The logic is the same as existing code,
line #147. If we skip the check here, we may still get assertion failure
somewhere later.

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:168: return make_scoped_refptr(new AudioBuffer(
On 2017/05/08 21:50:18, Tom Sepez wrote:
> How trusting is the audio_buffer class about having enough data to satisify
the
> constraints imposed by the other parameters?  Since these are now coming
across
> IPC, we'd probably need to validate them.

Since it's compressed data, the data_size could be arbitrary. The best I can do
is adding something like CHECK_LE(channel_count * frame_count * sizeof(short),
data_size).

[Tom Sepez, 2017-05-08 22:56:56]

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:166: CHECK_GT(frame_count, 0);  // Otherwise looks
like an EOF buffer.
On 2017/05/08 22:51:02, AndyWu wrote:
> On 2017/05/08 21:50:18, Tom Sepez wrote:
> > Which process can this run in?  If it's the browser, then we don't want to
> > crash.
> 
> Yes, it can run in the browser process. The logic is the same as existing
code,
> line #147. If we skip the check here, we may still get assertion failure
> somewhere later.

Ok, then in both places we'll want to change to DCHECK, and return null buffers
if this is the case for the release builds.

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:168: return make_scoped_refptr(new AudioBuffer(
On 2017/05/08 22:51:02, AndyWu wrote:
> On 2017/05/08 21:50:18, Tom Sepez wrote:
> > How trusting is the audio_buffer class about having enough data to satisify
> the
> > constraints imposed by the other parameters?  Since these are now coming
> across
> > IPC, we'd probably need to validate them.
> 
> Since it's compressed data, the data_size could be arbitrary. The best I can
do
> is adding something like CHECK_LE(channel_count * frame_count * sizeof(short),
> data_size).

Ok, that doesn't help much, we may be OK however if you can point out the places
where the audio class bounds checks accesses to this compressed stream before
blindly using the values .

[AndyWu, 2017-05-09 01:25:46]

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
File media/base/audio_buffer.cc (right):

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:166: CHECK_GT(frame_count, 0);  // Otherwise looks
like an EOF buffer.
On 2017/05/08 22:56:56, Tom Sepez wrote:
> On 2017/05/08 22:51:02, AndyWu wrote:
> > On 2017/05/08 21:50:18, Tom Sepez wrote:
> > > Which process can this run in?  If it's the browser, then we don't want to
> > > crash.
> > 
> > Yes, it can run in the browser process. The logic is the same as existing
> code,
> > line #147. If we skip the check here, we may still get assertion failure
> > somewhere later.
> 
> Ok, then in both places we'll want to change to DCHECK, and return null
buffers
> if this is the case for the release builds.

Done, I add more sanity checks in media/mojo/common/media_type_converters.cc. It
will return EOS if check failed.

https://codereview.chromium.org/2572573007/diff/160001/media/base/audio_buffe...
media/base/audio_buffer.cc:168: return make_scoped_refptr(new AudioBuffer(
On 2017/05/08 22:56:56, Tom Sepez wrote:
> On 2017/05/08 22:51:02, AndyWu wrote:
> > On 2017/05/08 21:50:18, Tom Sepez wrote:
> > > How trusting is the audio_buffer class about having enough data to
satisify
> > the
> > > constraints imposed by the other parameters?  Since these are now coming
> > across
> > > IPC, we'd probably need to validate them.
> > 
> > Since it's compressed data, the data_size could be arbitrary. The best I can
> do
> > is adding something like CHECK_LE(channel_count * frame_count *
sizeof(short),
> > data_size).
> 
> Ok, that doesn't help much, we may be OK however if you can point out the
places
> where the audio class bounds checks accesses to this compressed stream before
> blindly using the values .

The compressed data will be passed to audio sink, which is AudioTrack on
Android, and then transferred to meadia_server process on Android. The bottom
line is that we won't parse the data in browser process.

[AndyWu, 2017-05-09 01:26:14]

The CQ bit was checked by tsunghung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 01:27:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 04:06:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 04:06:09]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-05-10 17:10:02]

lgtm

[AndyWu, 2017-05-10 17:14:25]

The CQ bit was checked by tsunghung@chromium.org

[AndyWu, 2017-05-10 17:14:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2572573007/#ps200001
(title: "Sanity checks")

[commit-bot: I haz the power, 2017-05-10 17:15:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 20:19:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:19:10]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[AndyWu, 2017-05-11 15:58:20]

The CQ bit was checked by tsunghung@chromium.org

[commit-bot: I haz the power, 2017-05-11 15:59:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 17:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:59:02]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[AndyWu, 2017-05-15 16:51:48]

The CQ bit was checked by tsunghung@chromium.org

[commit-bot: I haz the power, 2017-05-15 16:52:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 17:51:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:51:44]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[AndyWu, 2017-05-15 19:49:21]

The CQ bit was checked by tsunghung@chromium.org

[commit-bot: I haz the power, 2017-05-15 19:50:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 20:53:46]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494877761418380,
"parent_rev": "7aabcec49978c5a588d9818f8a0d7bdc7c337aff", "commit_rev":
"4131b4cb048807c5651f917bc77669545128ff5b"}

[commit-bot: I haz the power, 2017-05-15 20:53:57]

Description was changed from

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use passthrough decoder for (E)AC3 formats

This CL is part of enabling (E)AC3 passthrough feature. Use "Android
raw decoder" for (E)AC3 audio formats. The decoder would only decrypt
but not decode the audio bitstream, i.e. the output would be clean but
compressed audio bitstream, not PCM.

BUG=613385
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2572573007
Cr-Commit-Position: refs/heads/master@{#471893}
Committed:
https://chromium.googlesource.com/chromium/src/+/4131b4cb048807c5651f917bc776...
==========

[commit-bot: I haz the power, 2017-05-15 20:53:59]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/4131b4cb048807c5651f917bc776...