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

Issue 9442005: Clean up audio-related utility functions to compute buffer sizes (Closed)

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

Clean up audio-related utility functions to compute buffer sizes Renames SelectSamplesPerPacket() to GetHighLatencyOutputBufferSize() and moves it into audio_hardware.h, where its low-latency counterpart lies. BUG=NONE TEST=NONE Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124981

Patch Set 1 #

Patch Set 2 : . #

Total comments: 12

Patch Set 3 : Rebase ToT + fix nit #

Patch Set 4 : Fix content unittest #

Patch Set 5 : . #

Patch Set 6 : Rebase ToT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -18 lines) Patch
M content/browser/renderer_host/media/audio_input_renderer_host.cc View 1 2 2 chunks +1 line, -6 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.cc View 1 2 3 4 5 1 chunk +1 line, -5 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/audio_hardware.h View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/media/audio_hardware.cc View 1 chunk +25 lines, -0 lines 0 comments Download
M content/renderer/media/audio_renderer_impl.cc View 1 2 chunks +2 lines, -1 line 0 comments Download
M media/audio/audio_parameters.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M media/audio/audio_util.h View 1 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
vrk (LEFT CHROMIUM)
This CL is based off of https://chromiumcodereview.appspot.com/9416085/
8 years, 10 months ago (2012-02-23 00:54:07 UTC) #1
vrk (LEFT CHROMIUM)
crogers: I implemented your suggestions from issue 9416085, with one minor modification in AudioRendererHost/AudioRendererImplHost: > ...
8 years, 10 months ago (2012-02-23 01:06:42 UTC) #2
henrika (OOO until Aug 14)
LGTM. https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_input_renderer_host.cc File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_input_renderer_host.cc#newcode209 content/browser/renderer_host/media/audio_input_renderer_host.cc:209: DCHECK_GT(audio_params.samples_per_packet, 0); Nice. Thanks.
8 years, 10 months ago (2012-02-23 08:45:35 UTC) #3
scherkus (not reviewing)
LGTM w/ nits https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc#newcode199 content/browser/renderer_host/media/audio_renderer_host.cc:199: DCHECK_GT(audio_params.samples_per_packet, 0); does it make sense ...
8 years, 10 months ago (2012-02-23 18:27:43 UTC) #4
Chris Rogers
LGTM with some comments https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc#newcode199 content/browser/renderer_host/media/audio_renderer_host.cc:199: DCHECK_GT(audio_params.samples_per_packet, 0); Is there any ...
8 years, 10 months ago (2012-02-23 19:00:21 UTC) #5
vrk (LEFT CHROMIUM)
Thanks everyone! crogers/scherkus: PTAL, I have a few questions for you in the comments! https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc ...
8 years, 10 months ago (2012-02-23 22:33:17 UTC) #6
scherkus (not reviewing)
https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://chromiumcodereview.appspot.com/9442005/diff/2001/content/browser/renderer_host/media/audio_renderer_host.cc#newcode199 content/browser/renderer_host/media/audio_renderer_host.cc:199: DCHECK_GT(audio_params.samples_per_packet, 0); On 2012/02/23 22:33:18, Victoria Kirst wrote: > ...
8 years, 10 months ago (2012-02-23 23:49:09 UTC) #7
vrk (LEFT CHROMIUM)
On 2012/02/23 23:49:09, scherkus wrote: > Sounds like something worth investigating post-commit! > > For ...
8 years, 10 months ago (2012-02-27 18:42:27 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vrk@chromium.org/9442005/21001
8 years, 9 months ago (2012-03-05 18:20:42 UTC) #9
commit-bot: I haz the power
8 years, 9 months ago (2012-03-05 20:08:17 UTC) #10
Change committed as 124981

Powered by Google App Engine
This is Rietveld 408576698