SetVolume and GetVolume take one volume instead of separate left and right vo...

SetVolume and GetVolume take one volume instead of separate left and right volumes.
BUG=26660
TEST=no visible difference. Make sure volume still works.  Code size should go down marginally.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31018

[fbarchard, 2009-11-04 03:52:55]

Cleaning up volume.  Mostly a learning exercise.  I may need to add a parameter
to specify audio layouts, so I wanted to start with something simple.

[Alpha Left Google, 2009-11-04 04:31:39]

Try bots complain about compilation, can you fix that?

[fbarchard, 2009-11-04 04:36:22]

reduced audio unittest time from 20 seconds to 8 seconds.

[scherkus (not reviewing), 2009-11-04 04:40:41]

few tiny nits

http://codereview.chromium.org/357004/diff/10001/11017
File chrome/renderer/audio_message_filter.cc (right):

http://codereview.chromium.org/357004/diff/10001/11017#newcode98
Line 98: double volume) {
nit: this can now fit on a single line

http://codereview.chromium.org/357004/diff/10001/11016
File chrome/renderer/audio_message_filter_unittest.cc (right):

http://codereview.chromium.org/357004/diff/10001/11016#newcode142
Line 142: kVolume));
nit: this can now fit on a single line

http://codereview.chromium.org/357004/diff/10001/11005
File media/audio/audio_output.h (right):

http://codereview.chromium.org/357004/diff/10001/11005#newcode95
Line 95: // sources the volume must be the same in both channels.
comment needs updating

http://codereview.chromium.org/357004/diff/10001/11005#newcode99
Line 99: // sources the level is returned in both channels.
comment needs updating

[Mark Mentovai, 2009-11-04 05:07:24]

http://codereview.chromium.org/357004/diff/10013/11032
File media/audio/audio_output.h (right):

http://codereview.chromium.org/357004/diff/10013/11032#newcode100
Line 100: virtual void GetVolume(double* volume) = 0;
Make this return a double?

http://codereview.chromium.org/357004/diff/10013/11030
File media/audio/mac/audio_output_mac.h (right):

http://codereview.chromium.org/357004/diff/10013/11030#newcode62
Line 62: float volume_;
I wonder why the getter and setter operate in terms of double, but this is
float.  Why the discrepancy?

The same thing happens in media/audio/win/waveout_output_win.h, but not in
media/audio/fake_audio_output_stream.h.

[fbarchard, 2009-11-04 09:06:44]

fixed nits.
locally builds, but requires clobber, so hopefully try bot knows how to do that.

http://codereview.chromium.org/357004/diff/10001/11017
File chrome/renderer/audio_message_filter.cc (right):

http://codereview.chromium.org/357004/diff/10001/11017#newcode98
Line 98: double volume) {
On 2009/11/04 04:40:42, scherkus wrote:
> nit: this can now fit on a single line

Done.

http://codereview.chromium.org/357004/diff/10001/11016
File chrome/renderer/audio_message_filter_unittest.cc (right):

http://codereview.chromium.org/357004/diff/10001/11016#newcode142
Line 142: kVolume));
On 2009/11/04 04:40:42, scherkus wrote:
> nit: this can now fit on a single line

Done.

http://codereview.chromium.org/357004/diff/10001/11005
File media/audio/audio_output.h (right):

http://codereview.chromium.org/357004/diff/10001/11005#newcode95
Line 95: // sources the volume must be the same in both channels.
On 2009/11/04 04:40:42, scherkus wrote:
> comment needs updating

Done.

http://codereview.chromium.org/357004/diff/10001/11005#newcode99
Line 99: // sources the level is returned in both channels.
On 2009/11/04 04:40:42, scherkus wrote:
> comment needs updating

Done.

http://codereview.chromium.org/357004/diff/10013/11032#newcode100
Line 100: virtual void GetVolume(double* volume) = 0;
On 2009/11/04 05:07:24, Mark Mentovai wrote:
> Make this return a double?

That may be possible, but its more of a change than I'd intended to make, and
the longer term may yet do multiple volumes, but likely an array (for
multichannel).
This part of the API is just internal for the renderer to talk to the browser.

http://codereview.chromium.org/357004/diff/10013/11030
File media/audio/mac/audio_output_mac.h (right):

http://codereview.chromium.org/357004/diff/10013/11030#newcode62
Line 62: float volume_;
ipc doesnt support float.

[fbarchard, 2009-11-04 18:34:02]

added a chrome unittest change.  To test chrome unittest on windows, do this:

cd /d C:\chrome-svn\src\chrome
devenv /build Release chrome.sln /project chrome.vcproj

[scherkus (not reviewing), 2009-11-04 19:03:32]

I think some of your other changes made it into the CL

other than that some few nits but looking good -- thanks for doing this!

http://codereview.chromium.org/357004/diff/11100/11113
File chrome/browser/renderer_host/audio_renderer_host.cc (right):

http://codereview.chromium.org/357004/diff/11100/11113#newcode229
Line 229: host_->Send(new ViewMsg_NotifyAudioStreamVolume(route_id_, stream_id_,
nit: I think this can fit on a single line now

http://codereview.chromium.org/357004/diff/11100/11113#newcode468
Line 468: double volume) {
nit: I think this can fit on a single line now

http://codereview.chromium.org/357004/diff/11100/11112
File chrome/browser/renderer_host/audio_renderer_host.h (right):

http://codereview.chromium.org/357004/diff/11100/11112#newcode302
Line 302: void OnSetVolume(const IPC::Message& msg, int stream_id,
nit: I think this can fit on a single line now

http://codereview.chromium.org/357004/diff/11100/11121
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/357004/diff/11100/11121#newcode259
Line 259: if (sample_rate_ > 48000) {
I think your other patch made it into this change

http://codereview.chromium.org/357004/diff/11100/11122
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/357004/diff/11100/11122#newcode219
Line 219: int sample_rate_;
why was const removed?

http://codereview.chromium.org/357004/diff/11100/11111
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/357004/diff/11100/11111#newcode217
Line 217: (duration_.ToInternalValue() && (elapsed > duration_))) {
is this change intended to be here?  it seems unrelated

[fbarchard, 2009-11-04 19:27:51]

Almost there!  It builds, its nit free, the unittest bleeps in delight.
Now if we only have a working volume slider...

http://codereview.chromium.org/357004/diff/11100/11113
File chrome/browser/renderer_host/audio_renderer_host.cc (right):

http://codereview.chromium.org/357004/diff/11100/11113#newcode229
Line 229: host_->Send(new ViewMsg_NotifyAudioStreamVolume(route_id_, stream_id_,
82 characters :(

http://codereview.chromium.org/357004/diff/11100/11113#newcode468
Line 468: double volume) {
93 characters

http://codereview.chromium.org/357004/diff/11100/11112
File chrome/browser/renderer_host/audio_renderer_host.h (right):

http://codereview.chromium.org/357004/diff/11100/11112#newcode302
Line 302: void OnSetVolume(const IPC::Message& msg, int stream_id,
On 2009/11/04 19:03:32, scherkus wrote:
> nit: I think this can fit on a single line now

Done.
sigh.  I think on my 1 year anniversary at Google, I should repropose 80 columns
rule be changed :)

http://codereview.chromium.org/357004/diff/11100/11121
File media/audio/linux/alsa_output.cc (right):

http://codereview.chromium.org/357004/diff/11100/11121#newcode259
Line 259: if (sample_rate_ > 48000) {
On 2009/11/04 19:03:32, scherkus wrote:
> I think your other patch made it into this change

Done.

http://codereview.chromium.org/357004/diff/11100/11122
File media/audio/linux/alsa_output.h (right):

http://codereview.chromium.org/357004/diff/11100/11122#newcode219
Line 219: int sample_rate_;
so rate could be changed.  I'll revert that.

http://codereview.chromium.org/357004/diff/11100/11111
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/357004/diff/11100/11111#newcode217
Line 217: (duration_.ToInternalValue() && (elapsed > duration_))) {
unintentional.  reverted

[Alpha Left Google, 2009-11-04 19:36:10]

If trybots are all green then LGTM.

[fbarchard, 2009-11-04 20:56:05]

Its green.  Good to go folks?

[scherkus (not reviewing), 2009-11-04 22:22:23]

LGTM -- go for it!