Issue 115693004: Added volume adjust sound behind the flag.

Issue 115693004: Added volume adjust sound behind the flag. (Closed)

Created:
7 years ago by ygorshenin1

Modified:
6 years, 11 months ago

Reviewers:
oshima, DaleCurtis, jennyz

CC:
chromium-reviews, sadrul, feature-media-reviews_chromium.org, oshima+watch_chromium.org, kalyank, stevenjb+watch_chromium.org, ben+ash_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Added volume adjust sound behind the flag. Partially based on https://codereview.chromium.org/24618003/. BUG=225886 TEST=media_unittests:AudioStreamHandler* Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245342

Patch Set 4 : A lot of variables in WavParser a replaced by a single AudioParameters field. #

Patch Set 5 : Split of WavAudioHandler is rolled back. #

Total comments: 16

Patch Set 7 : Fixed creation of AudioParameters. #

Total comments: 6

Patch Set 11 : Fix, rebase. #

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+146 lines, -73 lines)			Patch
A +	chrome/browser/ui/ash/DEPS	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
M	chrome/browser/ui/ash/volume_controller_chromeos.cc	View	1 2 3 4 5 6 7 8 9 10	4 chunks	+28 lines, -2 lines	0 comments	Download
M	chromeos/audio/chromeos_sounds.h	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	chromeos/chromeos_switches.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/chromeos_switches.cc	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	media/audio/sounds/audio_stream_handler.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+6 lines, -4 lines	0 comments	Download
M	media/audio/sounds/audio_stream_handler.cc	View	1 2 3 4 5 6 7 8 9	9 chunks	+66 lines, -30 lines	0 comments	Download
M	media/audio/sounds/audio_stream_handler_unittest.cc	View	1 2 3 4 5 6 7	2 chunks	+4 lines, -4 lines	0 comments	Download
M	media/audio/sounds/sounds_manager.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -3 lines	0 comments	Download
M	media/audio/sounds/sounds_manager_unittest.cc	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
D	media/audio/sounds/wav_audio_handler.h	View	1 2 3 4 5 6	3 chunks	+6 lines, -8 lines	0 comments	Download
M	media/audio/sounds/wav_audio_handler.cc	View	1 2 3 4 5 6 7 8	5 chunks	+17 lines, -13 lines	0 comments	Download
D	media/audio/sounds/wav_audio_handler_unittest.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+12 lines, -8 lines	0 comments	Download

Messages

Total messages: 20 (0 generated)

Expand Messages | Collapse Messages

DaleCurtis

I think the class splitting you've done makes the code harder to follow. I also ...

7 years ago (2013-12-17 19:28:52 UTC) #2

ygorshenin1

This refactoring was made because I didn't see any easy way to implement test for ...

7 years ago (2013-12-18 14:33:12 UTC) #3

DaleCurtis

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc File media/audio/sounds/audio_stream_handler.cc (right): https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc#newcode39 media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false), I think instead of all this replay_scheduled_ and ...

7 years ago (2013-12-18 21:20:21 UTC) #4

ygorshenin1

PTAL https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc File media/audio/sounds/audio_stream_handler.cc (right): https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc#newcode39 media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false), On 2013/12/18 21:20:21, DaleCurtis wrote: > I ...

7 years ago (2013-12-19 15:42:29 UTC) #5

PTAL

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false),
On 2013/12/18 21:20:21, DaleCurtis wrote:
> I think instead of all this replay_scheduled_ and schedule_replay_for_testing
> stuff, you should just use a base::OneShotTimer to stop the stream after a
> couple seconds. A lot of this code gets much simpler then:
> - Add a lock around cursor_.
> - Rename is_idle_ to is_stream_stopped_.
> - Add timer which calls StopStream() (which sets is_stream_stopped_).
> - During Play(), run Timer::Stop() if !is_stream_stopped_. No need to check
> replay bits.
> - During OnMoreData(), if AtEnd(), reset the timer which will start countdown
> until Stopping.
> 
> Timer's are self canceling, so you don't need any CancellableClosure, or
WeakPtr
> to clean it up upon destruction either.

* added a lock around internal state;
* is_stream_idle_ is replaced by is_stream_stopped_;

Not sure that I understood your idea about OneShotTimer. We need a replay bit,
because otherwise if frequency of notifications from keyboard (about 40ms on
Lumpy for pressed volume up key) is comparable with duration of a sound (48ms
for volume adjust sound), there will be undesired sound effects on each Play()
request, like overlapping of sounds or gaps between sounds.

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:76: const int64 size = data_.size();
On 2013/12/18 21:20:21, DaleCurtis wrote:
> If you set AudioBuffer::frames_per_buffer = data_.size() / (channels *
> bytes_per_sample), the duration returned by AudioParameters::GetBufferDuration
> should match this calculation.

frames_per_buffer is used in creation of AudioStreamProxy, and when the change
you proposed is applied, volume up/down sound becomes very harsh. I'd prefer to
use kDefaultFrameCount here.

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:105: params_.bits_per_sample() >> 3);
On 2013/12/18 21:20:21, DaleCurtis wrote:
> Just use * 8, the compiler will optimize as necessary.

Done.

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.h (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:32: int GetByteRate() const;
On 2013/12/18 21:20:21, DaleCurtis wrote:
> Looks unused? Does this end up being different than
> AudioParameters::GetBytesPerSecond() ?

Done.

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:34: const base::TimeDelta& duration()
const { return duration_; }
On 2013/12/18 21:20:21, DaleCurtis wrote:
> TimeDelta can be passed by value efficiently, so no reference here. Also, does
> AudioParameters::GetBufferDuration() not work here?

Fixed to return by value now.

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.h:35: const base::StringPiece& data() const
{ return data_; }
It's used once in wav_audio_handler_unittest.cc, for testing CopyTo().

On 2013/12/18 21:20:21, DaleCurtis wrote:
> I don't think this should be exposed. I don't see where this is used either?

DaleCurtis

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc File media/audio/sounds/audio_stream_handler.cc (right): https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audio_stream_handler.cc#newcode39 media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false), On 2013/12/19 15:42:29, ygorshenin1 wrote: > On 2013/12/18 ...

7 years ago (2013-12-19 19:44:07 UTC) #6

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false),
On 2013/12/19 15:42:29, ygorshenin1 wrote:
> On 2013/12/18 21:20:21, DaleCurtis wrote:
> > I think instead of all this replay_scheduled_ and
schedule_replay_for_testing
> > stuff, you should just use a base::OneShotTimer to stop the stream after a
> > couple seconds. A lot of this code gets much simpler then:
> > - Add a lock around cursor_.
> > - Rename is_idle_ to is_stream_stopped_.
> > - Add timer which calls StopStream() (which sets is_stream_stopped_).
> > - During Play(), run Timer::Stop() if !is_stream_stopped_. No need to check
> > replay bits.
> > - During OnMoreData(), if AtEnd(), reset the timer which will start
countdown
> > until Stopping.
> > 
> > Timer's are self canceling, so you don't need any CancellableClosure, or
> WeakPtr
> > to clean it up upon destruction either.
> 
> * added a lock around internal state;
> * is_stream_idle_ is replaced by is_stream_stopped_;
> 
> Not sure that I understood your idea about OneShotTimer. We need a replay bit,
> because otherwise if frequency of notifications from keyboard (about 40ms on
> Lumpy for pressed volume up key) is comparable with duration of a sound (48ms
> for volume adjust sound), there will be undesired sound effects on each Play()
> request, like overlapping of sounds or gaps between sounds.

I'm only suggesting the timer is used to Stop() the stream.  Are you concerned
that setting cursor_ = 0 during each Play() request will chop off the current
playing sound if the user is pressing keys too quickly?

Where are you getting the 40ms number from?  Someone would have to hit the
volume key 25 times per second to reach your 40ms number (1000/25).  I'm a
pretty fast typist and was only able to manage 5 presses in a second :)

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:76: const int64 size = data_.size();
On 2013/12/19 15:42:29, ygorshenin1 wrote:
> On 2013/12/18 21:20:21, DaleCurtis wrote:
> > If you set AudioBuffer::frames_per_buffer = data_.size() / (channels *
> > bytes_per_sample), the duration returned by
AudioParameters::GetBufferDuration
> > should match this calculation.
> 
> frames_per_buffer is used in creation of AudioStreamProxy, and when the change
> you proposed is applied, volume up/down sound becomes very harsh. I'd prefer
to
> use kDefaultFrameCount here.

Yes, you shouldn't use the AudioParameters directly from WavAudioHandler in
stream creation.  AudioStreamHandler should choose its own buffer size, it can
copy the rest of these parameters though.

ygorshenin1

7 years ago (2013-12-20 09:05:05 UTC) #7

PTAL

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/audi...
media/audio/sounds/audio_stream_handler.cc:39: replay_scheduled_(false),
You don't need to press a key 5 times per second, you just need to keep the key
pressed for a while. In the last case on my Lumpy notifications come each 40ms.
So, without such replay logic if user'll just keep the volume up key pressed for
a while, volume adjust sound will be pretty harsh.

On 2013/12/19 19:44:08, DaleCurtis (OOO Dec 19-Jan2) wrote:
> On 2013/12/19 15:42:29, ygorshenin1 wrote:
> > On 2013/12/18 21:20:21, DaleCurtis wrote:
> > > I think instead of all this replay_scheduled_ and
> schedule_replay_for_testing
> > > stuff, you should just use a base::OneShotTimer to stop the stream after a
> > > couple seconds. A lot of this code gets much simpler then:
> > > - Add a lock around cursor_.
> > > - Rename is_idle_ to is_stream_stopped_.
> > > - Add timer which calls StopStream() (which sets is_stream_stopped_).
> > > - During Play(), run Timer::Stop() if !is_stream_stopped_. No need to
check
> > > replay bits.
> > > - During OnMoreData(), if AtEnd(), reset the timer which will start
> countdown
> > > until Stopping.
> > > 
> > > Timer's are self canceling, so you don't need any CancellableClosure, or
> > WeakPtr
> > > to clean it up upon destruction either.
> > 
> > * added a lock around internal state;
> > * is_stream_idle_ is replaced by is_stream_stopped_;
> > 
> > Not sure that I understood your idea about OneShotTimer. We need a replay
bit,
> > because otherwise if frequency of notifications from keyboard (about 40ms on
> > Lumpy for pressed volume up key) is comparable with duration of a sound
(48ms
> > for volume adjust sound), there will be undesired sound effects on each
Play()
> > request, like overlapping of sounds or gaps between sounds.
> 
> I'm only suggesting the timer is used to Stop() the stream.  Are you concerned
> that setting cursor_ = 0 during each Play() request will chop off the current
> playing sound if the user is pressing keys too quickly?
> 
> Where are you getting the 40ms number from?  Someone would have to hit the
> volume key 25 times per second to reach your 40ms number (1000/25).  I'm a
> pretty fast typist and was only able to manage 5 presses in a second :)

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/115693004/diff/100001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:76: const int64 size = data_.size();
On 2013/12/19 19:44:08, DaleCurtis (OOO Dec 19-Jan2) wrote:
> On 2013/12/19 15:42:29, ygorshenin1 wrote:
> > On 2013/12/18 21:20:21, DaleCurtis wrote:
> > > If you set AudioBuffer::frames_per_buffer = data_.size() / (channels *
> > > bytes_per_sample), the duration returned by
> AudioParameters::GetBufferDuration
> > > should match this calculation.
> > 
> > frames_per_buffer is used in creation of AudioStreamProxy, and when the
change
> > you proposed is applied, volume up/down sound becomes very harsh. I'd prefer
> to
> > use kDefaultFrameCount here.
> 
> Yes, you shouldn't use the AudioParameters directly from WavAudioHandler in
> stream creation.  AudioStreamHandler should choose its own buffer size, it can
> copy the rest of these parameters though.

Done.

DaleCurtis

Does this functionality sound right? It seems the way you have it now that a ...

6 years, 11 months ago (2014-01-02 20:11:17 UTC) #8

ygorshenin1

I've removed replay logic completely. PTAL. https://codereview.chromium.org/115693004/diff/140001/chrome/browser/ui/ash/volume_controller_chromeos.cc File chrome/browser/ui/ash/volume_controller_chromeos.cc (right): https://codereview.chromium.org/115693004/diff/140001/chrome/browser/ui/ash/volume_controller_chromeos.cc#newcode68 chrome/browser/ui/ash/volume_controller_chromeos.cc:68: PlayVolumeAdjustSound(); On 2014/01/02 ...

6 years, 11 months ago (2014-01-09 19:26:56 UTC) #9

DaleCurtis

https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/volume_controller_chromeos.cc File chrome/browser/ui/ash/volume_controller_chromeos.cc (right): https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/volume_controller_chromeos.cc#newcode70 chrome/browser/ui/ash/volume_controller_chromeos.cc:70: if (play_sound) Can you just move this into the ...

6 years, 11 months ago (2014-01-09 19:58:26 UTC) #10

ygorshenin1

PTAL https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/volume_controller_chromeos.cc File chrome/browser/ui/ash/volume_controller_chromeos.cc (right): https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/volume_controller_chromeos.cc#newcode70 chrome/browser/ui/ash/volume_controller_chromeos.cc:70: if (play_sound) On 2014/01/09 19:58:26, DaleCurtis wrote: > ...

6 years, 11 months ago (2014-01-13 10:00:54 UTC) #11

PTAL

https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/v...
File chrome/browser/ui/ash/volume_controller_chromeos.cc (right):

https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:70: if (play_sound)
On 2014/01/09 19:58:26, DaleCurtis wrote:
> Can you just move this into the else block above w/o the boolean?  Presumably
> !IsOutputMuted() === audio_handler->GetOutputVolumePercent() != 0 ?

Done.

https://codereview.chromium.org/115693004/diff/180001/chrome/browser/ui/ash/v...
chrome/browser/ui/ash/volume_controller_chromeos.cc:91: if (play_sound)
No, because in this case user will not hear volume adjust sound when volume is
increased from, say, 96% to 100%.

On 2014/01/09 19:58:26, DaleCurtis wrote:
> Does just checking OutputVolume != 100 here work?

https://codereview.chromium.org/115693004/diff/180001/media/audio/sounds/audi...
File media/audio/sounds/audio_stream_handler.cc (right):

https://codereview.chromium.org/115693004/diff/180001/media/audio/sounds/audi...
media/audio/sounds/audio_stream_handler.cc:62: } else if (!CanStart()) {
On 2014/01/09 19:58:26, DaleCurtis wrote:
> You can simplify this by tracking "started_" instead of "can_start_" and using
a
> base::CancelableClosure().  Doing so allows you to remove the extra lock and
> avoid the explosion of Get/Set/Can methods.  Replace this else with:
> 
> <acquire lock_>
> stop_callback_.Reset(base::Bind(
>     &AudioStreamContainer::StopStream, base::Unretained(this)));
> if (started_ && wav_audio_.AtEnd(cursor_)) {
>   cursor_ = 0;
>   return;
> }
> started_ = true;
> 
> Down below inside OnMoreData() change the PostTask() to a PostDelayedTask()
with
> a timeout of whatever you want for your keep alive duration (XXX):
> 
> <acquire lock_>
> AudioManager::Get()->GetTaskRunner()->PostDelayedTask(
>     FROM_HERE, stop_callback_, base::TimeDelta::FromSeconds(XXX));
> 
> Inside StopStream() just add a "started_ = false";

Done.

https://codereview.chromium.org/115693004/diff/180001/media/audio/sounds/audi...
media/audio/sounds/audio_stream_handler.cc:62: } else if (!CanStart()) {
On 2014/01/09 19:58:26, DaleCurtis wrote:
> You can simplify this by tracking "started_" instead of "can_start_" and using
a
> base::CancelableClosure().  Doing so allows you to remove the extra lock and
> avoid the explosion of Get/Set/Can methods.  Replace this else with:
> 
> <acquire lock_>
> stop_callback_.Reset(base::Bind(
>     &AudioStreamContainer::StopStream, base::Unretained(this)));
> if (started_ && wav_audio_.AtEnd(cursor_)) {
>   cursor_ = 0;
>   return;
> }
> started_ = true;
> 
> Down below inside OnMoreData() change the PostTask() to a PostDelayedTask()
with
> a timeout of whatever you want for your keep alive duration (XXX):
> 
> <acquire lock_>
> AudioManager::Get()->GetTaskRunner()->PostDelayedTask(
>     FROM_HERE, stop_callback_, base::TimeDelta::FromSeconds(XXX));
> 
> Inside StopStream() just add a "started_ = false";

Done.

https://codereview.chromium.org/115693004/diff/180001/media/audio/sounds/wav_...
File media/audio/sounds/wav_audio_handler.cc (right):

https://codereview.chromium.org/115693004/diff/180001/media/audio/sounds/wav_...
media/audio/sounds/wav_audio_handler.cc:96: VLOG(1) << "Number of channel
mismatch.";
On 2014/01/09 19:58:26, DaleCurtis wrote:
> DVLOG(1) ?

Done.

DaleCurtis

lgtm % nits. Thanks for your patience! https://codereview.chromium.org/115693004/diff/250013/media/audio/sounds/audio_stream_handler.cc File media/audio/sounds/audio_stream_handler.cc (right): https://codereview.chromium.org/115693004/diff/250013/media/audio/sounds/audio_stream_handler.cc#newcode77 media/audio/sounds/audio_stream_handler.cc:77: if (started_) ...

6 years, 11 months ago (2014-01-13 18:12:31 UTC) #12

ygorshenin1

Thank you for the review! + oshima for chrome/* and chromeos/* https://codereview.chromium.org/115693004/diff/250013/media/audio/sounds/audio_stream_handler.cc File media/audio/sounds/audio_stream_handler.cc (right): ...

6 years, 11 months ago (2014-01-13 18:30:50 UTC) #13

oshima

lgtm with a nit https://codereview.chromium.org/115693004/diff/760001/media/audio/sounds/wav_audio_handler_unittest.cc File media/audio/sounds/wav_audio_handler_unittest.cc (right): https://codereview.chromium.org/115693004/diff/760001/media/audio/sounds/wav_audio_handler_unittest.cc#newcode25 media/audio/sounds/wav_audio_handler_unittest.cc:25: ASSERT_EQ(static_cast<size_t>(4), handler.data().size()); nit: 4U

6 years, 11 months ago (2014-01-15 18:52:43 UTC) #15

ygorshenin1

Thank you, Mitsuru. Jenny, do you have any comments? https://codereview.chromium.org/115693004/diff/760001/media/audio/sounds/wav_audio_handler_unittest.cc File media/audio/sounds/wav_audio_handler_unittest.cc (right): https://codereview.chromium.org/115693004/diff/760001/media/audio/sounds/wav_audio_handler_unittest.cc#newcode25 media/audio/sounds/wav_audio_handler_unittest.cc:25: ...

6 years, 11 months ago (2014-01-16 11:36:28 UTC) #16

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/115693004/890001

6 years, 11 months ago (2014-01-16 20:05:39 UTC) #19

Message was sent while issue was closed.

Change committed as 245342

Expand Messages | Collapse Messages