Guard against midstream audio configuration changes.

Guard against midstream audio configuration changes.

In WebAudio this can lead to oob reads.  In HTML5 this will cause
garbled audio or oob reads depending on the sample format.  The
guards in place just ensure channel count nor sample format change
between demux and decode.

Video already has similar guards in place via the checks done in
VideoFrame::IsValidConfig during buffer allocation.

BUG=166565
TEST=unit tests pass under ASAN.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182027

[DaleCurtis, 2013-02-12 00:54:07]

Ronald: Let me know if this seems silly.

[rbultje1, 2013-02-12 00:58:40]

On 2013/02/12 00:54:07, DaleCurtis wrote:
> Ronald: Let me know if this seems silly.

Uhm... I'm a little impartial here, upstream has done a lot of effort to make
adaptive changes (e.g. samplerate, channelcount, frame size) work correctly, it
seems silly to not support it if upstream does support it. Is there a reason we
want to disallow it? Isn't this critical for adaptive support?

[DaleCurtis, 2013-02-12 01:12:05]

At present we don't support "decode" driven configuration changes, only
"demuxer" driven changes; and even then only via MediaSource.

If we want to support this we'll need to convert the decoded audio data to
whatever was initially configured.  The biggest hurdle, converting
AudioRendererAlgorithm to use float, is already planned for other reasons. 
Afterward, adding an AudioConverter to FFmpegAudioDecoder shouldn't be too
difficult.

For now we'll at least need to prevent this from occurring to avoid hitting oob
reads.

cc: acolwell to get his opinion on non-media-source based configuration changes.

[rbultje1, 2013-02-12 01:13:17]

ok, if this is by design I guess it's OK.

Eventually, I'd love if we _did_ support decoder-driven config changes, but
obviously that's very low-priority.

[scherkus (not reviewing), 2013-02-12 02:11:55]

Responding to questions -- still reviewing code.

On 2013/02/12 01:12:05, DaleCurtis wrote:
> At present we don't support "decode" driven configuration changes, only
> "demuxer" driven changes; and even then only via MediaSource.

That's not quite true ... we support mid-stream resolution changes for VP8 and
the downstream consumers are expected to handle it.

> If we want to support this we'll need to convert the decoded audio data to
> whatever was initially configured.  The biggest hurdle, converting
> AudioRendererAlgorithm to use float, is already planned for other reasons. 
> Afterward, adding an AudioConverter to FFmpegAudioDecoder shouldn't be too
> difficult.

I imagine the AudioConverter would be a part of the AudioRendererImpl as to
avoid per-AudioDecoder-code, but that's something to discuss down the road.

> For now we'll at least need to prevent this from occurring to avoid hitting
oob
> reads.
> 
> cc: acolwell to get his opinion on non-media-source based configuration
changes.

[scherkus (not reviewing), 2013-02-12 02:16:56]

lgtm w/ nits

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
media/filters/audio_file_reader.cc:185: DLOG(ERROR) << "Unsupported mid-frame
configuration change!"
consistent terminology nit: s/mid-frame/mid-stream/

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
File media/filters/audio_file_reader_unittest.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
media/filters/audio_file_reader_unittest.cc:157:
RunTestFailingDecode("midstream_config_change.mp3");
mp3s will fail for chromium

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:309: // Store initial values to guard
against mid-frame configuration changes.
consistent terminology nit: s/mid-frame/mid-stream/

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:400: DLOG(ERROR) << "Unsupported mid-frame
configuration change!"
consistent terminology nit: s/mid-frame/mid-stream/

https://codereview.chromium.org/12224114/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:664: // Verify files which change
configuration mid-stream are failed gracefully.
s/are failed gracefully/fail gracefully/

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:159: // Store initial
values to guard against mid-frame configuration changes.
consistent terminology nit: s/mid-frame/mid-stream/

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: DLOG(ERROR) <<
"Unsupported mid-frame configuration change!"
consistent terminology nit: s/mid-frame/mid-stream/

[DaleCurtis, 2013-02-12 02:41:24]

Done % s/mid-stream/midstream/:

https://www.google.com/search?q=define+midstream

Will CQ tomorrow after test file lands.

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
media/filters/audio_file_reader.cc:185: DLOG(ERROR) << "Unsupported mid-frame
configuration change!"
On 2013/02/12 02:16:56, scherkus wrote:
> consistent terminology nit: s/mid-frame/mid-stream/

Done.

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
File media/filters/audio_file_reader_unittest.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/audio_file_read...
media/filters/audio_file_reader_unittest.cc:157:
RunTestFailingDecode("midstream_config_change.mp3");
On 2013/02/12 02:16:56, scherkus wrote:
> mp3s will fail for chromium

Whoops, supposed to be in the #if block.

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:309: // Store initial values to guard
against mid-frame configuration changes.
On 2013/02/12 02:16:56, scherkus wrote:
> consistent terminology nit: s/mid-frame/mid-stream/

Done.

https://codereview.chromium.org/12224114/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:400: DLOG(ERROR) << "Unsupported mid-frame
configuration change!"
On 2013/02/12 02:16:56, scherkus wrote:
> consistent terminology nit: s/mid-frame/mid-stream/

Done.

https://codereview.chromium.org/12224114/diff/1/media/filters/pipeline_integr...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/12224114/diff/1/media/filters/pipeline_integr...
media/filters/pipeline_integration_test.cc:664: // Verify files which change
configuration mid-stream are failed gracefully.
On 2013/02/12 02:16:56, scherkus wrote:
> s/are failed gracefully/fail gracefully/

Done.

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
File webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:159: // Store initial
values to guard against mid-frame configuration changes.
On 2013/02/12 02:16:56, scherkus wrote:
> consistent terminology nit: s/mid-frame/mid-stream/

Done.

https://codereview.chromium.org/12224114/diff/1/webkit/media/crypto/ppapi/ffm...
webkit/media/crypto/ppapi/ffmpeg_cdm_audio_decoder.cc:281: DLOG(ERROR) <<
"Unsupported mid-frame configuration change!"
On 2013/02/12 02:16:56, scherkus wrote:
> consistent terminology nit: s/mid-frame/mid-stream/

Done.

[acolwell GONE FROM CHROMIUM, 2013-02-12 17:37:42]

On 2013/02/12 01:12:05, DaleCurtis wrote:
> cc: acolwell to get his opinion on non-media-source based configuration
changes.

The Media Source spec allows sample rate and channel count to change during
playback so we'll need this functionality eventually. It's currently disabled
since we don't have the proper resampling and up/down mixing code in place yet.

[DaleCurtis, 2013-02-12 19:07:26]

I've filed http://crbug.com/175780 and blocked http://crbug.com/122916 on it to
track support for configuration changes.

[commit-bot: I haz the power, 2013-02-12 19:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/12224114/9001

[commit-bot: I haz the power, 2013-02-12 21:56:26]

Change committed as 182027