Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 9395057: Fix muted audio when playback rate != 1.0 or 0.0 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by vrk (LEFT CHROMIUM)
Modified:
5 years, 2 months ago
CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, scherkus (not reviewing), ihf+watch_chromium.org, Chris Rogers
Visibility:
Public.

Description

Fix muted audio when playback rate != 1.0 or 0.0 Rewrites the logic in AudioRendererAlgorithmBase to be able to output audio at any point of a sped-up/slowed down window, instead of only outputting audio in full multiples of windows. BUG=108239 TEST=media_unittests, manual testing on video test matrix Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125052

Patch Set 1 #

Patch Set 2 : . #

Total comments: 38

Patch Set 3 : Response to CR #

Patch Set 4 : . #

Patch Set 5 : Rebase to ToT and issue 9442005 #

Total comments: 8

Patch Set 6 : Adding unit tests and response to CR #

Patch Set 7 : . #

Total comments: 18

Patch Set 8 : Response to CR #

Total comments: 1

Patch Set 9 : . #

Patch Set 10 : . #

Total comments: 6

Patch Set 11 : Rebase ToT #

Patch Set 12 : Address comments, revert modifying buffer sizes in audio_hardware.cc #

Patch Set 13 : Rebase ToT, fix win compile failure #

Patch Set 14 : . #

Patch Set 15 : Rebase ToT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+736 lines, -391 lines) Patch
M content/renderer/media/audio_hardware.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/media/audio_renderer_impl.cc View 1 2 3 4 8 13 2 chunks +9 lines, -10 lines 0 comments Download
M media/audio/audio_util.h View 1 2 3 4 8 13 1 chunk +0 lines, -5 lines 0 comments Download
M media/audio/audio_util.cc View 1 2 3 4 2 chunks +0 lines, -72 lines 0 comments Download
M media/base/seekable_buffer.h View 1 2 2 chunks +6 lines, -3 lines 0 comments Download
M media/base/seekable_buffer.cc View 1 2 4 chunks +32 lines, -21 lines 0 comments Download
M media/filters/audio_renderer_algorithm_base.h View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +84 lines, -26 lines 0 comments Download
M media/filters/audio_renderer_algorithm_base.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +286 lines, -105 lines 0 comments Download
M media/filters/audio_renderer_algorithm_base_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +259 lines, -115 lines 0 comments Download
M media/filters/audio_renderer_base.h View 1 4 chunks +7 lines, -4 lines 0 comments Download
M media/filters/audio_renderer_base.cc View 1 2 3 4 5 11 chunks +23 lines, -21 lines 0 comments Download
M media/filters/audio_renderer_base_unittest.cc View 1 chunk +8 lines, -4 lines 0 comments Download
M media/filters/null_audio_renderer.h View 1 chunk +2 lines, -0 lines 0 comments Download
M media/filters/null_audio_renderer.cc View 3 chunks +15 lines, -5 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 16 (0 generated)
vrk (LEFT CHROMIUM)
5 years, 3 months ago (2012-02-18 00:42:36 UTC) #1
vrk (LEFT CHROMIUM)
I haven't rewritten the AudioRendererAlgorithmBase unit tests yet, which I will need to do as ...
5 years, 3 months ago (2012-02-18 00:44:37 UTC) #2
acolwell GONE FROM CHROMIUM
Looks pretty good. Thanks for all the helpful comments in the code. http://codereview.chromium.org/9395057/diff/2001/content/renderer/media/audio_renderer_impl.cc File content/renderer/media/audio_renderer_impl.cc ...
5 years, 3 months ago (2012-02-22 07:51:40 UTC) #3
vrk (LEFT CHROMIUM)
Thanks for all the helpful comments, PTAL! https://chromiumcodereview.appspot.com/9395057/diff/2001/content/renderer/media/audio_renderer_impl.cc File content/renderer/media/audio_renderer_impl.cc (right): https://chromiumcodereview.appspot.com/9395057/diff/2001/content/renderer/media/audio_renderer_impl.cc#newcode221 content/renderer/media/audio_renderer_impl.cc:221: uint32 filled_frames ...
5 years, 3 months ago (2012-02-23 20:33:06 UTC) #4
vrk (LEFT CHROMIUM)
cc: crogers I just rebased the CL to issue 9442005 (https://chromiumcodereview.appspot.com/9442005/). Since my fix in ...
5 years, 3 months ago (2012-02-23 22:45:52 UTC) #5
Chris Rogers
Thanks Victoria, I haven't looked through the whole CL, but added one comment nit: https://chromiumcodereview.appspot.com/9395057/diff/14003/content/renderer/media/audio_hardware.cc ...
5 years, 3 months ago (2012-02-23 23:39:54 UTC) #6
acolwell GONE FROM CHROMIUM
LGTM w/ minor nits https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio_renderer_algorithm_base.cc File media/filters/audio_renderer_algorithm_base.cc (right): https://chromiumcodereview.appspot.com/9395057/diff/14003/media/filters/audio_renderer_algorithm_base.cc#newcode116 media/filters/audio_renderer_algorithm_base.cc:116: while(total_frames_rendered < requested_frames) { nit: ...
5 years, 3 months ago (2012-02-24 00:48:32 UTC) #7
vrk (LEFT CHROMIUM)
acolwell: PTAL! Specifically, please take a look at the CrossfadeFrame() changes in AudioRendererAlgorithmBase and the ...
5 years, 3 months ago (2012-02-27 18:19:13 UTC) #8
acolwell GONE FROM CHROMIUM
Looks good. Just a few nits https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio_renderer_algorithm_base.cc File media/filters/audio_renderer_algorithm_base.cc (right): https://chromiumcodereview.appspot.com/9395057/diff/18015/media/filters/audio_renderer_algorithm_base.cc#newcode372 media/filters/audio_renderer_algorithm_base.cc:372: CrossfadeFrame<int8>(outtro, intro); I ...
5 years, 3 months ago (2012-02-27 21:00:07 UTC) #9
vrk (LEFT CHROMIUM)
Did a lot of mathing today to try to answer acolwell's questions about errors and ...
5 years, 3 months ago (2012-02-28 02:18:36 UTC) #10
acolwell GONE FROM CHROMIUM
Thanks for looking into this. I figured this might be the ultimate outcome, but at ...
5 years, 3 months ago (2012-02-28 15:55:28 UTC) #11
vrk (LEFT CHROMIUM)
acolwell: PT-yet-another-L :) I tweaked some of the unit test constants so that they would ...
5 years, 3 months ago (2012-02-28 23:40:31 UTC) #12
acolwell GONE FROM CHROMIUM
Overall LGTM. Just a few tiny comments/observations. https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio_renderer_algorithm_base.cc File media/filters/audio_renderer_algorithm_base.cc (right): https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio_renderer_algorithm_base.cc#newcode391 media/filters/audio_renderer_algorithm_base.cc:391: playback_rate_ = ...
5 years, 3 months ago (2012-02-29 00:31:38 UTC) #13
vrk (LEFT CHROMIUM)
Thanks for the reviews! crogers: FYI the TODO in audio_hardware.cc. https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio_renderer_algorithm_base.cc File media/filters/audio_renderer_algorithm_base.cc (right): https://chromiumcodereview.appspot.com/9395057/diff/35004/media/filters/audio_renderer_algorithm_base.cc#newcode391 ...
5 years, 3 months ago (2012-02-29 20:41:39 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vrk@chromium.org/9395057/51001
5 years, 2 months ago (2012-03-05 22:45:57 UTC) #15
commit-bot: I haz the power
5 years, 2 months ago (2012-03-06 00:48:38 UTC) #16
Change committed as 125052
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06