Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(126)

Issue 757733003: Delay audio rendering until the clock reaches the first timestamp. (Closed)

Created:
6 years ago by DaleCurtis
Modified:
6 years ago
Reviewers:
xhwang
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Delay audio rendering until the clock reaches the first timestamp. Since AudioClock no longer looks at timestamps, indeed they've been completely removed from AudioRendererAlgorithm, ARI::Render() has no way to correct for media where the video starts before the audio. We don't want to add timestamp information back everywhere, so this change just tracks the first decoded timestamp received and uses that to delay rendering (by writing silence) until the appropriate time is reached. Since this may result in partial fills, AudioRendererAlgorithm has been extended to include an offset into the buffer for writing. BUG=435908 TEST=new unittest, sample media works. Committed: https://crrev.com/8d4f5ec4f26857fd8081f88bad83e99be7c4dab4 Cr-Commit-Position: refs/heads/master@{#305727}

Patch Set 1 : Typo. #

Total comments: 17

Patch Set 2 : Comments. #

Patch Set 3 : Fix cast. #

Patch Set 4 : Fix state assumption. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -26 lines) Patch
M media/cast/test/fake_media_source.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M media/filters/audio_renderer_algorithm.h View 1 1 chunk +6 lines, -1 line 0 comments Download
M media/filters/audio_renderer_algorithm.cc View 3 chunks +7 lines, -4 lines 0 comments Download
M media/filters/audio_renderer_algorithm_unittest.cc View 1 5 chunks +47 lines, -7 lines 0 comments Download
M media/filters/audio_renderer_impl.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M media/filters/audio_renderer_impl.cc View 1 2 3 4 chunks +31 lines, -5 lines 0 comments Download
M media/filters/audio_renderer_impl_unittest.cc View 1 2 3 4 chunks +51 lines, -8 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
DaleCurtis
6 years ago (2014-11-24 22:43:30 UTC) #3
xhwang
Nice fix! LGTM % a few comments + nits. https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_algorithm.h File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_algorithm.h#newcode48 media/filters/audio_renderer_algorithm.h:48: ...
6 years ago (2014-11-25 18:12:36 UTC) #4
DaleCurtis
Thanks for review! https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_algorithm.h File media/filters/audio_renderer_algorithm.h (right): https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_algorithm.h#newcode48 media/filters/audio_renderer_algorithm.h:48: // |dest_offset| is the offset into ...
6 years ago (2014-11-25 20:37:53 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/757733003/40001
6 years ago (2014-11-25 20:44:19 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/96597) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/2052)
6 years ago (2014-11-25 21:02:23 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/757733003/60001
6 years ago (2014-11-25 21:06:06 UTC) #11
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_triggered_tests/builds/84394)
6 years ago (2014-11-25 21:51:22 UTC) #13
DaleCurtis
https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_impl.cc File media/filters/audio_renderer_impl.cc (right): https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_impl.cc#newcode598 media/filters/audio_renderer_impl.cc:598: first_packet_ts_ - audio_clock_->back_timestamp(); On 2014/11/25 20:37:53, DaleCurtis wrote: > ...
6 years ago (2014-11-25 22:11:28 UTC) #14
xhwang
On 2014/11/25 22:11:28, DaleCurtis wrote: > https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_impl.cc > File media/filters/audio_renderer_impl.cc (right): > > https://codereview.chromium.org/757733003/diff/20001/media/filters/audio_renderer_impl.cc#newcode598 > ...
6 years ago (2014-11-25 22:28:39 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/757733003/80001
6 years ago (2014-11-25 22:35:06 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:80001)
6 years ago (2014-11-25 23:32:48 UTC) #18
commit-bot: I haz the power
6 years ago (2014-11-25 23:33:32 UTC) #19
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/8d4f5ec4f26857fd8081f88bad83e99be7c4dab4
Cr-Commit-Position: refs/heads/master@{#305727}

Powered by Google App Engine
This is Rietveld 408576698