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

Issue 164031: Mute audio under OLA when given rate would cause poor quality. (Closed)

Created:
11 years, 4 months ago by kylep
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com, scherkus (not reviewing), awong, Alpha Left Google
Visibility:
Public.

Description

Mute audio under OLA when given rate would cause poor quality. BUG=18362 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22974

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 3

Patch Set 5 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -3 lines) Patch
M media/filters/audio_renderer_algorithm_ola.h View 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M media/filters/audio_renderer_algorithm_ola.cc View 1 2 3 4 3 chunks +18 lines, -2 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
kylep
I didn't mute for audio @ 0.5x speed even though it sounds bad because of ...
11 years, 4 months ago (2009-08-06 00:21:41 UTC) #1
fbarchard
LGTM I would expand on the comment about issues with higher and lower rates. 1. ...
11 years, 4 months ago (2009-08-06 00:45:13 UTC) #2
fbarchard
nit on the comment http://codereview.chromium.org/164031/diff/1/2 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/164031/diff/1/2#newcode45 Line 45: // Limited rate to ...
11 years, 4 months ago (2009-08-06 01:00:27 UTC) #3
scherkus (not reviewing)
drive by comments: - change description does not match code change - magic numbers! - ...
11 years, 4 months ago (2009-08-06 03:39:13 UTC) #4
kylep
Extracted constants, clarified comments, renamed some stuff to make it more readable.
11 years, 4 months ago (2009-08-06 17:51:49 UTC) #5
fbarchard
LGTM nit uses more electricity on 8 bit than 16 bit. :) http://codereview.chromium.org/164031/diff/1008/1009 File media/filters/audio_renderer_algorithm_ola.cc ...
11 years, 4 months ago (2009-08-06 18:33:52 UTC) #6
scherkus (not reviewing)
wait would it be possible to have a unit test for this change? asserting that ...
11 years, 4 months ago (2009-08-06 19:23:35 UTC) #7
fbarchard1
don't be evil :-)yes it would be possible. At EA we had a saying - ...
11 years, 4 months ago (2009-08-06 21:55:37 UTC) #8
kylep
How exactly should I write said test... pass in buffers full of random data, set ...
11 years, 4 months ago (2009-08-07 00:16:06 UTC) #9
fbarchard1
kyle and I looked at this... I was thinking it would be like my audio_util ...
11 years, 4 months ago (2009-08-07 01:00:00 UTC) #10
scherkus (not reviewing)
without getting into testing philosophies here, we can forgo the tests I think you wanted ...
11 years, 4 months ago (2009-08-10 22:06:22 UTC) #11
kylep
So... commit when this turns green? http://codereview.chromium.org/164031/diff/1008/1009 File media/filters/audio_renderer_algorithm_ola.cc (right): http://codereview.chromium.org/164031/diff/1008/1009#newcode20 Line 20: // TODO(kylep): ...
11 years, 4 months ago (2009-08-10 22:44:37 UTC) #12
scherkus (not reviewing)
11 years, 4 months ago (2009-08-10 22:50:53 UTC) #13
LGTM!

Powered by Google App Engine
This is Rietveld 408576698