Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1868983004:
Fixing AudioBuffer params and channel layout bugs (Closed)
DescriptionFixing AudioBuffer params and channel layout bugs
Work for AAC implicit signaling introduced the possiblity for audio rendering
configs to change to change midstream. Checking for channel layout changes
caused a regression and caused 6 channel audio playback to fail.
Clusterfuzz also detected some heap buffer overflow issues, when the decoded
audio buffers we receive have a different channel layout or sample rate than
the ones we expect.
This change allows FfmpegAudioDecoder to ignore channel layout changes on non
AAC codec. It also checks that decoded AudioBuffer sample rate and channel
count match the expected parameters before adding them to the splicer queue, in
AudioRendererImpl.
BUG=599625, 599846, 600538
TEST=manual tests on an ASAN build, with ffmpeg_regression_test to follow
REVIEW=dalecurtis
Committed: https://crrev.com/43eed5f900264ff88be25806c883dab2011260fa
Cr-Commit-Position: refs/heads/master@{#385922}
Patch Set 1 #Patch Set 2 : Fixing typos #
Total comments: 16
Patch Set 3 : Addressing comments #Patch Set 4 : Addressing last comment #Patch Set 5 : cl format #Patch Set 6 : Returning after pipeline error #
Total comments: 6
Messages
Total messages: 25 (11 generated)
tguilbert@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:303: bool config_is_stale = Naming style in chrome is typically "is_config_stale" https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:308: // Only consider channel layout changes for AAC. Add a comment why, something like: // TODO(tguilbert, dalecurtis): Due to http://crbug.com/600538 we need to // allow channel layout changes for the time being. See if ffmpeg is fixable. https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:309: if(config_.codec() == kCodecAAC){ run git cl format to fix formatting errors. notably spaces around () and no need for {} https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:332: ResetTimestampState(); This only needs to be called when sample_rate changes. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:538: // Check if the buffer we received matches the expected configuration. Add something like: // TODO(chcunningham, tguilbert): Figure out if we want to support implicit // config changes during src=. Doing so requires resampling each individual // stream which is inefficient when there are many tags in a page. // // Note: We explicitly do not check channel layout here to avoid breaking // weird behavior with multichannel wave files: http://crbug.com/600538. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:539: No extra line. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:543: ChannelLayoutToChannelCount(audio_parameters_.channel_layout()))) { Just use audio_parameters_.channels() ? https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:544: HandleAbortedReadOrDecodeError(PIPELINE_ERROR_DECODE); Add a MEDIA_LOG in here, something like: MEDIA_LOG(ERROR, media_log_) << "Unsupported midstream configuration change!" << " Sample Rate: " << buffer->sample_rate() << " vs " << audio_parameters_.sample_rate() << ", Channels: " << buffer->channel_count() << " vs " << audio_parameters_.channels();
https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:303: bool config_is_stale = On 2016/04/07 20:05:04, DaleCurtis wrote: > Naming style in chrome is typically "is_config_stale" Done. https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:308: // Only consider channel layout changes for AAC. On 2016/04/07 20:05:04, DaleCurtis wrote: > Add a comment why, something like: > > // TODO(tguilbert, dalecurtis): Due to http://crbug.com/600538 we need to > // allow channel layout changes for the time being. See if ffmpeg is fixable. Done. https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:309: if(config_.codec() == kCodecAAC){ On 2016/04/07 20:05:04, DaleCurtis wrote: > run git cl format to fix formatting errors. notably spaces around () and no need > for {} Done. https://codereview.chromium.org/1868983004/diff/20001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:332: ResetTimestampState(); On 2016/04/07 20:05:04, DaleCurtis wrote: > This only needs to be called when sample_rate changes. Does this need to be called before discard_helper_->ProcessBuffers(_,_) on line 365? https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:538: // Check if the buffer we received matches the expected configuration. On 2016/04/07 20:05:04, DaleCurtis wrote: > Add something like: > > // TODO(chcunningham, tguilbert): Figure out if we want to support implicit > // config changes during src=. Doing so requires resampling each individual > // stream which is inefficient when there are many tags in a page. > // > // Note: We explicitly do not check channel layout here to avoid breaking > // weird behavior with multichannel wave files: http://crbug.com/600538. Done. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:539: On 2016/04/07 20:05:04, DaleCurtis wrote: > No extra line. Done. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:543: ChannelLayoutToChannelCount(audio_parameters_.channel_layout()))) { On 2016/04/07 20:05:04, DaleCurtis wrote: > Just use audio_parameters_.channels() ? Oops! You are right. Done. https://codereview.chromium.org/1868983004/diff/20001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:544: HandleAbortedReadOrDecodeError(PIPELINE_ERROR_DECODE); On 2016/04/07 20:05:04, DaleCurtis wrote: > Add a MEDIA_LOG in here, something like: > > MEDIA_LOG(ERROR, media_log_) > << "Unsupported midstream configuration change!" > << " Sample Rate: " << buffer->sample_rate() << " vs " > << audio_parameters_.sample_rate() > << ", Channels: " << buffer->channel_count() << " vs " > << audio_parameters_.channels(); Done.
lgtm % offline discussion to only call resettimestampstate on sample rate changes.
The CQ bit was checked by tguilbert@chromium.org
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/1868983004/#ps60001 (title: "Addressing last comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868983004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868983004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patch failed since you skipped git cl format.
The CQ bit was checked by tguilbert@chromium.org
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/1868983004/#ps80001 (title: "cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868983004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868983004/80001
The CQ bit was unchecked by dalecurtis@chromium.org
The CQ bit was checked by tguilbert@chromium.org
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/1868983004/#ps100001 (title: "Returning after pipeline error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868983004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868983004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fixing AudioBuffer params and channel layout bugs Work for AAC implicit signaling introduced the possiblity for audio rendering configs to change to change midstream. Checking for channel layout changes caused a regression and caused 6 channel audio playback to fail. Clusterfuzz also detected some heap buffer overflow issues, when the decoded audio buffers we receive have a different channel layout or sample rate than the ones we expect. This change allows FfmpegAudioDecoder to ignore channel layout changes on non AAC codec. It also checks that decoded AudioBuffer sample rate and channel count match the expected parameters before adding them to the splicer queue, in AudioRendererImpl. BUG=599625, 599846, 600538 TEST=manual tests on an ASAN build, with ffmpeg_regression_test to follow REVIEW=dalecurtis ========== to ========== Fixing AudioBuffer params and channel layout bugs Work for AAC implicit signaling introduced the possiblity for audio rendering configs to change to change midstream. Checking for channel layout changes caused a regression and caused 6 channel audio playback to fail. Clusterfuzz also detected some heap buffer overflow issues, when the decoded audio buffers we receive have a different channel layout or sample rate than the ones we expect. This change allows FfmpegAudioDecoder to ignore channel layout changes on non AAC codec. It also checks that decoded AudioBuffer sample rate and channel count match the expected parameters before adding them to the splicer queue, in AudioRendererImpl. BUG=599625, 599846, 600538 TEST=manual tests on an ASAN build, with ffmpeg_regression_test to follow REVIEW=dalecurtis Committed: https://crrev.com/43eed5f900264ff88be25806c883dab2011260fa Cr-Commit-Position: refs/heads/master@{#385922} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/43eed5f900264ff88be25806c883dab2011260fa Cr-Commit-Position: refs/heads/master@{#385922}
Message was sent while issue was closed.
chcunningham@chromium.org changed reviewers: + chcunningham@chromium.org
Message was sent while issue was closed.
Thanks for saving implicit signalling from revert! Some questions for you below https://codereview.chromium.org/1868983004/diff/100001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/1868983004/diff/100001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:310: // allow channel layout changes for the moment. See if ffmpeg is fixable. Just to confirm my understanding, we initially have CHANNEL_LAYOUT_5_1, but then a buffer comes in with CHANNEL_LAYOUT_5_1_BACK, and we believe this is a bug in FFmpeg? I don't know the .wav format (MPEG PS?) well... I can't say whether a channel layout should be possible. Can you share the .wav files from the bug with me? I'm not able to download from pantheon for some reason. I'm wondering which of these layouts is expected (if not both). Also wondering how far reaching this bug is. https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:538: // TODO(chcunningham, tguilbert): Figure out if we want to support implicit Implicit config changes for AAC actually work with today's src= code because ffmpeg figures figures out the implicit AAC profile when it first opens it up. We have src= tests for the various aac files here [0]. I'm not aware of other containers that do implicit signalling nor am I aware of anyone asking for additional src= support. Decode error LGTM for now :) 0: https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin... https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:558: if (!splicer_->AddInput(buffer)) { IIUC, prior to adding the decode error above, we were doing a buffer overflow in AudioBuffer::ReadFrames. I notice that method has a few DCHECKS [0] for this scenario... What do you guys think about making them CHECKS instead? 0: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...
Message was sent while issue was closed.
https://codereview.chromium.org/1868983004/diff/100001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/1868983004/diff/100001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:310: // allow channel layout changes for the moment. See if ffmpeg is fixable. On 2016/04/19 00:28:56, chcunningham wrote: > Just to confirm my understanding, we initially have CHANNEL_LAYOUT_5_1, but then > a buffer comes in with CHANNEL_LAYOUT_5_1_BACK, and we believe this is a bug in > FFmpeg? I don't know the .wav format (MPEG PS?) well... I can't say whether a > channel layout should be possible. > > Can you share the .wav files from the bug with me? I'm not able to download from > pantheon for some reason. I'm wondering which of these layouts is expected (if > not both). Also wondering how far reaching this bug is. I do not know if this is a bug per-say in Ffmpeg. We seemed to have never taken this as a guarantee before. We don't set the layout [0] and it is potentially overwritten from what I understand [1]. This failure did not happen mid stream, but failed as soon as the file started playing. 0: https://code.google.com/p/chromium/codesearch#chromium/src/media/ffmpeg/ffmpe... 1: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg... https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:538: // TODO(chcunningham, tguilbert): Figure out if we want to support implicit On 2016/04/19 00:28:56, chcunningham wrote: > Implicit config changes for AAC actually work with today's src= code because > ffmpeg figures figures out the implicit AAC profile when it first opens it up. > We have src= tests for the various aac files here [0]. I'm not aware of other > containers that do implicit signalling nor am I aware of anyone asking for > additional src= support. Decode error LGTM for now :) > > > 0: > https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin... Discussed offline. Currently, the media used to test signaling doesn't work properly on src= playback. https://codereview.chromium.org/1868983004/diff/100001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:558: if (!splicer_->AddInput(buffer)) { On 2016/04/19 00:28:56, chcunningham wrote: > IIUC, prior to adding the decode error above, we were doing a buffer overflow in > AudioBuffer::ReadFrames. I notice that method has a few DCHECKS [0] for this > scenario... What do you guys think about making them CHECKS instead? > > > 0: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... SGTM, but please get a second opinion :) |
