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

Issue 9416085: Increase the buffer size in AudioRendererImpl to fix muted playback rate (Closed)

Created:
8 years, 10 months ago by vrk (LEFT CHROMIUM)
Modified:
8 years, 10 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
Visibility:
Public.

Description

Increase the buffer size in AudioRendererImpl to fix muted playback rate This CL temporarily removes GetBufferSizeForSampleRate() and uses SelectSamplesPerPacket() to determine the size of samples_per_packet in AudioDevice. This restores the buffer size that AudioRendererImpl used prior to r113821. SelectSamplesPerPacket() calculates a much larger samples_per_packet value than GetBufferSizeForSampleRate(), so it fixes the issue described in 108239. This CL also does some mild refactoring to move audio_common.cc into audio_util.cc. BUG=108239 TEST=media_unittests,content_unittests, manual testing of different playback rates Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123292

Patch Set 1 #

Total comments: 2

Patch Set 2 : rebase ToT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -77 lines) Patch
D content/browser/renderer_host/media/audio_common.h View 1 chunk +0 lines, -17 lines 0 comments Download
D content/browser/renderer_host/media/audio_common.cc View 1 chunk +0 lines, -33 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_renderer_host.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.cc View 1 2 chunks +2 lines, -2 lines 0 comments Download
M content/content_browser.gypi View 1 1 chunk +0 lines, -2 lines 0 comments Download
M content/renderer/media/audio_renderer_impl.cc View 2 chunks +1 line, -21 lines 0 comments Download
M media/audio/audio_util.h View 2 chunks +5 lines, -0 lines 0 comments Download
M media/audio/audio_util.cc View 2 chunks +27 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
vrk (LEFT CHROMIUM)
crogers/scherkus: Please review. henrika: FYI.
8 years, 10 months ago (2012-02-22 02:12:47 UTC) #1
vrk (LEFT CHROMIUM)
This is a simpler fix for issue 108239 that should be safe to merge into ...
8 years, 10 months ago (2012-02-22 02:19:19 UTC) #2
henrika (OOO until Aug 14)
LGTM. No conflicts with WebRTC.
8 years, 10 months ago (2012-02-22 10:31:54 UTC) #3
Chris Rogers
https://chromiumcodereview.appspot.com/9416085/diff/1/content/browser/renderer_host/media/audio_renderer_host.cc File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://chromiumcodereview.appspot.com/9416085/diff/1/content/browser/renderer_host/media/audio_renderer_host.cc#newcode204 content/browser/renderer_host/media/audio_renderer_host.cc:204: } Do we need to handle the case where ...
8 years, 10 months ago (2012-02-22 23:08:53 UTC) #4
Chris Rogers
LGTM based on offline discussion with Victoria. We need to get this into M18 as ...
8 years, 10 months ago (2012-02-23 00:32:44 UTC) #5
vrk (LEFT CHROMIUM)
8 years, 10 months ago (2012-02-23 00:53:27 UTC) #6
On 2012/02/23 00:32:44, Chris Rogers wrote:
> LGTM based on offline discussion with Victoria.  We need to get this into M18
as
> safely as possible, then my other suggestions can be worked on...

Thanks everyone!

Forking this CL to a follow-up CL here:
https://chromiumcodereview.appspot.com/9442005/

Powered by Google App Engine
This is Rietveld 408576698