Enable implicit signalling for HE AAC v1 & v2 in ADTS.

Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872
Committed: https://crrev.com/8a400694209ce5d56e013673f8c9e8a2b0a1017c
Cr-Commit-Position: refs/heads/master@{#383478}

[chcunningham, 2016-03-16 03:35:33]

Patchset #1 (id:1) has been deleted

[chcunningham, 2016-03-17 21:48:19]

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:408: // WORRY: IF STREAM CHANGES, WHERE
DOES MIXER LEARN OF THE NEW INPUT CHANNEL COUNT?
This happens in AudioBufferConverter::AddBuffer. Each time we add a buffer the
buffer's configuration is evaluated against the existing config. If things
change, it makes a new internal AudioConverter with the new input parameters.

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:409: // WORRY: WHERE IS BROWSER LEARNING
OF NEW HW CHANNEL COUNT IF SOMETHING GETS PLUGGED IN? WHERE IS BROWSER-SIDE
MIXER?
The HW channel count is updated via audio_device_listener_{mac|win}.cc. We're
missing this feature on linux, so I filed http://crbug.com/595877

The browser-side mixing is done in the AudioConverter held by
OnMoreDataConverter. When the HW changes are detected,
AudioOutputController::OnDeviceChange() will be called, creating a new stream
with the updated configuration making its way to a new OnMoreDataConverter.

[DaleCurtis, 2016-03-18 00:16:22]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-03-18 00:16:23]

Needs some formatting and an actual commit message. Not sure if you meant to
send this out yet w/o any reviewers? :)

https://codereview.chromium.org/1805013003/diff/20001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1805013003/diff/20001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:305: channels !=
ChannelLayoutToChannelCount(config_.channel_layout()) ||  // IIUC, this should
go. We previously allowed channel layout changes as long as the number of
channels held fixed. Why?
Needed as discussed via e-mail.

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:418: if (hw_params.channel_layout() ==
CHANNEL_LAYOUT_DISCRETE) {
Why not just always use the discrete layout in this case?

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.h (right):

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.h:237: int32_t last_decoded_sample_rate_;
Just int

[chcunningham, 2016-03-25 01:41:16]

Ready for review :). Will update the description soon.

https://codereview.chromium.org/1805013003/diff/20001/media/filters/ffmpeg_au...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1805013003/diff/20001/media/filters/ffmpeg_au...
media/filters/ffmpeg_audio_decoder.cc:305: channels !=
ChannelLayoutToChannelCount(config_.channel_layout()) ||  // IIUC, this should
go. We previously allowed channel layout changes as long as the number of
channels held fixed. Why?
On 2016/03/18 00:16:23, DaleCurtis wrote:
> Needed as discussed via e-mail.

Acknowledged.

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.cc:418: if (hw_params.channel_layout() ==
CHANNEL_LAYOUT_DISCRETE) {
On 2016/03/18 00:16:23, DaleCurtis wrote:
> Why not just always use the discrete layout in this case?

We chatted - thing is we aren't confident that we can properly do the mixing for
discrete. Hoping the OS can umpix us from 5.1 to whatever the fancy layout
happens to be. 

NOTE TO SELF: ping the old bug and see that folks aren't regressed.

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
File media/renderers/audio_renderer_impl.h (right):

https://codereview.chromium.org/1805013003/diff/20001/media/renderers/audio_r...
media/renderers/audio_renderer_impl.h:237: int32_t last_decoded_sample_rate_;
On 2016/03/18 00:16:23, DaleCurtis wrote:
> Just int

Done.

[chcunningham, 2016-03-25 19:19:17]

Description was changed from

==========
Enable midstream profile changes for AAC.

BUG=534301
==========

to

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301
==========

[chcunningham, 2016-03-25 19:20:55]

FYI, this does not cover enabling explicit signalling of HE AAC in the MP4
container (http://crbug.com/526314). This will come in a follow up CL.

[wolenetz, 2016-03-25 19:50:20]

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

[wolenetz, 2016-03-25 19:50:20]

drive-by:
Would this CL also improve implicit signalling support for src= (ffmpegdemuxer),
especially where ffmpeg may not have access to the full stream during processing
prior to OnFindStreamInfoDone()?

[wolenetz, 2016-03-25 19:50:37]

wolenetz@chromium.org changed reviewers:
- wolenetz@chromium.org

[chcunningham, 2016-03-25 19:53:58]

On 2016/03/25 19:50:20, wolenetz wrote:
> drive-by:
> Would this CL also improve implicit signalling support for src=
(ffmpegdemuxer),
> especially where ffmpeg may not have access to the full stream during
processing
> prior to OnFindStreamInfoDone()?

Yes.

[chcunningham, 2016-03-25 21:38:59]

On 2016/03/25 19:53:58, chcunningham wrote:
> On 2016/03/25 19:50:20, wolenetz wrote:
> > drive-by:
> > Would this CL also improve implicit signalling support for src=
> (ffmpegdemuxer),
> > especially where ffmpeg may not have access to the full stream during
> processing
> > prior to OnFindStreamInfoDone()?
> 
> Yes.

Let me clarify a little. 

This mostly changes nothing for src=, other than to add tests. For typical
implicitly signalled AAC, src= already worked and still works. This changes
nothing there. 

This may theoretically add support for the narrow case where someone is
attempting to stream aac content that truly has midstream config changes to
src=. I don't think anyone is actually doing this though and we aren't going out
of our way to support it in the renderer like we are for MSE - meaning this sort
of content may face re-sampling/mixing to whatever the original config was. I
don't think we need to bother doing anything more for this use case though.

[DaleCurtis, 2016-03-25 21:57:00]

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

https://codereview.chromium.org/1805013003/diff/100001/media/base/audio_buffe...
media/base/audio_buffer.cc:213: if (!end_of_stream_)
Seems like something that could be DCHECK'd?

https://codereview.chromium.org/1805013003/diff/100001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/100001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:381: (hw_params.channel_layout() ==
CHANNEL_LAYOUT_DISCRETE)
Remove unnecessary parens. Upon further consideration I think we want to choose
STEREO here since it's the most common format by a huge margin:

https://uma.googleplex.com/histograms/?histograms=Media.AudioChannelLayout

I also think this check should include
CommandLine::HasSwitch(kTrySupportedChannelLayouts) and use STEREO in this case
as well since these are folk that want stereo expansion to all channels on
Windows. You'll need to add that flag to the RendererProcessHostImpl list to use
it here.

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:814: const uint32_tstart_time_ms;
missing space?

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:815: const uint32_tduration_ms;
Ditto.

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:818: struct MSEPlaybackTestData {
Seems we only need one data structure which we could use for both mse and src=?

[chcunningham, 2016-03-25 23:51:30]

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

https://codereview.chromium.org/1805013003/diff/100001/media/base/audio_buffe...
media/base/audio_buffer.cc:213: if (!end_of_stream_)
On 2016/03/25 21:57:00, DaleCurtis wrote:
> Seems like something that could be DCHECK'd?

Done.

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:814: const uint32_tstart_time_ms;
On 2016/03/25 21:57:00, DaleCurtis wrote:
> missing space?

Done.

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:815: const uint32_tduration_ms;
On 2016/03/25 21:57:00, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1805013003/diff/100001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:818: struct MSEPlaybackTestData {
On 2016/03/25 21:57:00, DaleCurtis wrote:
> Seems we only need one data structure which we could use for both mse and
src=?

Eh, my vote is for separate. Combining them makes the brace intializers longer
with values that only have meaning for one of the test types. Also, I'm
realizing now that the constructors aren't needed/used - will delete.

[chcunningham, 2016-03-25 23:52:16]

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

[chcunningham, 2016-03-25 23:52:17]

+ccameron for render_process_host_impl.cc

https://codereview.chromium.org/1805013003/diff/100001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/100001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:381: (hw_params.channel_layout() ==
CHANNEL_LAYOUT_DISCRETE)
On 2016/03/25 21:57:00, DaleCurtis wrote:
> Remove unnecessary parens. Upon further consideration I think we want to
choose
> STEREO here since it's the most common format by a huge margin:
> 
> https://uma.googleplex.com/histograms/?histograms=Media.AudioChannelLayout
> 
> I also think this check should include
> CommandLine::HasSwitch(kTrySupportedChannelLayouts) and use STEREO in this
case
> as well since these are folk that want stereo expansion to all channels on
> Windows. You'll need to add that flag to the RendererProcessHostImpl list to
use
> it here.

Done.

[DaleCurtis, 2016-03-25 23:57:51]

lgtm % nits and a couple bugs.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:378: bool
try_supported_channel_layouts_win = false;
Remove win suffix.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:382:
cmd_line->HasSwitch(switches::kTrySupportedChannelLayouts);
inline cmd_line?

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:417: (hw_channel_count >
stream_channel_count)
Remove extra parens.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:421:
audio_parameters_.Reset(hw_params.format(), renderer_channel_layout,
Do you want to initialize last_decoded_sample_rate_ here or check that you have
one when starting? Otherwise you'll always trigger a config change on first
decode.

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:806: // PlaybackTestData(const char*
filename,
Delete?

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:819: // MSEPlaybackTestData(const char*
filename,
Delete?

[chcunningham, 2016-03-26 00:31:36]

Thanks Dale

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:378: bool
try_supported_channel_layouts_win = false;
On 2016/03/25 23:57:51, DaleCurtis wrote:
> Remove win suffix.

Done.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:382:
cmd_line->HasSwitch(switches::kTrySupportedChannelLayouts);
On 2016/03/25 23:57:51, DaleCurtis wrote:
> inline cmd_line?

Done.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:417: (hw_channel_count >
stream_channel_count)
On 2016/03/25 23:57:51, DaleCurtis wrote:
> Remove extra parens.

Done.

https://codereview.chromium.org/1805013003/diff/120001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:421:
audio_parameters_.Reset(hw_params.format(), renderer_channel_layout,
On 2016/03/25 23:57:51, DaleCurtis wrote:
> Do you want to initialize last_decoded_sample_rate_ here or check that you
have
> one when starting? Otherwise you'll always trigger a config change on first
> decode.

Done. I added a check that its been set below before doing the OnConfigChange

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:806: // PlaybackTestData(const char*
filename,
On 2016/03/25 23:57:51, DaleCurtis wrote:
> Delete?

Done.

https://codereview.chromium.org/1805013003/diff/120001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:819: // MSEPlaybackTestData(const char*
filename,
On 2016/03/25 23:57:51, DaleCurtis wrote:
> Delete?

Done.

[chcunningham, 2016-03-26 01:27:36]

Description was changed from

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301
==========

to

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872
==========

[ccameron, 2016-03-26 10:32:57]

rphi lgtm

[chcunningham1, 2016-03-28 03:00:27]

The CQ bit was checked by chcunningham@google.com

[chcunningham1, 2016-03-28 03:00:28]

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

[commit-bot: I haz the power, 2016-03-28 03:00:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1805013003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1805013003/160001

[commit-bot: I haz the power, 2016-03-28 04:13:32]

Description was changed from

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872
==========

to

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872
==========

[commit-bot: I haz the power, 2016-03-28 04:13:33]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-03-28 04:14:43]

Description was changed from

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's 
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See 

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872
==========

to

==========
Enable implicit signalling for HE AAC v1 & v2 in ADTS.

This allows MSE to playback HE AAC v1 (Spectral Band Replication) and
v2 (Parametric Stereo) in the ADTS (.acc) container. ADTS is not
capable of explicitly signaling these profiles, so the frame's
metadata will claim the stream is an earlier profile (e.g AAC LC) and
the decoder will later tell us its actually HE AAC. See

For more background, see these sections of 2009 ISO-14496-3
1.6.5.2 Implicit and explicit signaling of SBR
1.6.6.2 Implicit and Explicit Signaling of Parametric Stereo

TEST=new media unit tests and all manual tests in bug report
BUG=534301, 177872

Committed: https://crrev.com/8a400694209ce5d56e013673f8c9e8a2b0a1017c
Cr-Commit-Position: refs/heads/master@{#383478}
==========

[commit-bot: I haz the power, 2016-03-28 04:14:44]

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