Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10592014:
media/audio/linux: Add CrasInputStream. (Closed)
|
Created:
8 years, 6 months ago by dgreid Modified:
8 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptionmedia/audio/linux: Add CrasInputStream.
This parallels the CrasOutputStream that was added previously. ChromeOS
already talks to CRAS for audio input, through an alsa plug in. Remove
that unneeded layer. Only built when use_cras is set.
A following patch will pull the CRAS client connection up a level and
share one client connection for all input and output streams that are
active.
BUG=chromium-os:25410
TEST=Newly added cras_input_unittest to media_unittests. Manual test
with apprtc and youtube.com/my_webcam.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146881
Patch Set 1 #
Total comments: 24
Patch Set 2 : Now better after input from xians. #
Total comments: 26
Patch Set 3 : new #
Total comments: 15
Patch Set 4 : allow repeated starts #
Total comments: 10
Patch Set 5 : don't allow re-start, use event in UT, and clean up set-volume. #
Total comments: 4
Patch Set 6 : Add scoped traces for capture tests. #
Total comments: 1
Patch Set 7 : fix order of includes #Patch Set 8 : rebase before trying cq #
Messages
Total messages: 27 (0 generated)
Finally got time to hook up the input side properly. Thanks for looking.
It looks really nice. Some comments for the first look. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... media/audio/linux/audio_manager_linux.cc:305: stream = new CrasInputStream(params, this); btw, do you support device selection? or any plan to support it? http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... media/audio/linux/audio_manager_linux.cc:306: } else { Why do we need the else case, it is a bit hard to read. Do we really need to DCHECK below, if not, how about changing it to: #if defined(USE_CRAS) if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseCras)) { return new CrasInputStream(params, this); #endif http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.cc File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:23: frame_rate_(params.sample_rate()), How about changing the name to sample_rate_? We definitely want using the same name for all the implementations. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:31: CrasInputStream::~CrasInputStream() {} Add a DCHECK here to make sure things have already cleaned up. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:41: LOG(WARNING) << "Unsupported audio frequency."; I know we use LOG for alsa too, but is it really needed? What about using DLOG instead. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:47: LOG(WARNING) << "Unsupported audio format."; ditto http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:52: LOG(WARNING) << "Unsupported bits/sample: " << params_.bits_per_sample(); ditto http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:58: LOG(WARNING) << "Couldn't create CRAS client.\n"; ditto http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:64: LOG(WARNING) << "Couldn't connect CRAS client.\n"; ditto http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:98: DCHECK(!callback_ && callback); Does this mean we only allow calling Start() once? What about if the users want to pause/start the stream? http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:116: frames_per_packet, // Total latency. Curiously, does it mean we are using one queue with the size of frames_per_packet as buffer? http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:118: frames_per_packet, // Don't care for capture. I am curious, what does this parameter mean? http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:138: audio_manager_->IncreaseActiveInputStreamCount(); In case Start() fails for any of the reason, should users call stop()? We call DecreaseActiveInputStreamCount() in Stop() as long as !client || !callback_, this does not match what Start() does. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:140: LOG(WARNING) << "Failed to add the stream"; DLOG, and add a period.
Thanks xians, comments addressed. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... File media/audio/linux/audio_manager_linux.cc (right): http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... media/audio/linux/audio_manager_linux.cc:305: stream = new CrasInputStream(params, this); On 2012/06/20 10:45:26, xians1 wrote: > btw, do you support device selection? or any plan to support it? The hope is that ChromeOS automatically selecting the most recently attached device will satisfy most use cases. Therefor there is not UI exposed to switch devices, but it works from the command line, so adding it won't be difficult if necessary. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/audio_manage... media/audio/linux/audio_manager_linux.cc:306: } else { On 2012/06/20 10:45:26, xians1 wrote: > Why do we need the else case, it is a bit hard to read. > Do we really need to DCHECK below, if not, how about changing it to: > #if defined(USE_CRAS) > if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUseCras)) { > return new CrasInputStream(params, this); > #endif Good suggestion, I also took the opportunity to modify the related MakeOutputStream(). http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.cc File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:23: frame_rate_(params.sample_rate()), On 2012/06/20 10:45:26, xians1 wrote: > How about changing the name to sample_rate_? > We definitely want using the same name for all the implementations. Done. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:31: CrasInputStream::~CrasInputStream() {} On 2012/06/20 10:45:26, xians1 wrote: > Add a DCHECK here to make sure things have already cleaned up. Done. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:41: LOG(WARNING) << "Unsupported audio frequency."; On 2012/06/20 10:45:26, xians1 wrote: > I know we use LOG for alsa too, but is it really needed? What about using DLOG > instead. Done for whole file. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:98: DCHECK(!callback_ && callback); On 2012/06/20 10:45:26, xians1 wrote: > Does this mean we only allow calling Start() once? > What about if the users want to pause/start the stream? Yes, call Start once until Stop is called. This struck me as odd as well. It is the same restriction in alsa_input. Is there a need to call it twice? http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:116: frames_per_packet, // Total latency. On 2012/06/20 10:45:26, xians1 wrote: > Curiously, does it mean we are using one queue with the size of > frames_per_packet as buffer? On the server side there are two buffers of this size created. They are passed to the capture callback ping-pong style. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:118: frames_per_packet, // Don't care for capture. On 2012/06/20 10:45:26, xians1 wrote: > I am curious, what does this parameter mean? For an output stream, it indicates the minimum amount of frames the client wants to send at a time. For capture streams it is ignored, comment updated. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:138: audio_manager_->IncreaseActiveInputStreamCount(); On 2012/06/20 10:45:26, xians1 wrote: > In case Start() fails for any of the reason, should users call stop()? > We call DecreaseActiveInputStreamCount() in Stop() as long as !client || > !callback_, this does not match what Start() does. Thanks, good catch. Made them match up, so that client_ will only be non-null if IncreaseActiveInputStreamCount() was called. http://codereview.chromium.org/10592014/diff/1/media/audio/linux/cras_input.c... media/audio/linux/cras_input.cc:140: LOG(WARNING) << "Failed to add the stream"; On 2012/06/20 10:45:26, xians1 wrote: > DLOG, and add a period. Done.
http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:24: num_channels_(params.channels()), if you save params_ do you need to save sample_rate_ and num_channels_ ? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:100: DCHECK(!callback_ && callback); split these DCHECKs into two so if they fail we know which one is the culprit http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:155: if (!client_ || !callback_) is this a DCHECK/programmer error? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:191: timespec latency_ts = {0, 0}; nit: remove extra space before = http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:197: double latency_usec = (latency_ts.tv_sec * 1e6) + latency_ts.tv_nsec * 1e-3; magic numbers! you can use base/time.h for kMicrosecondsPerSecond instead of 1e6 and kNanosecondsPerMicrosecond http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:224: return static_cast<double>(cras_client_get_system_max_capture_gain(client_)); ditto here -- see my concerns about casting below http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:241: if (!client_) SetVolume() when client_ == NULL is DCHECK() but getting is OK? is anyone relying on being able to call GetVolume() today when client_ is NULL? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:250: return static_cast<double>(current_volume); what's the range of value for current_volume? doesn't GetVolume() return a float between 0.0f and 1.0f? wouldn't casting mean we either return 0 or 1 and nothing inbetween? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:23: MOCK_METHOD5(OnData, void(AudioInputStream* stream, const uint8* src, nit: you can drop the param names and just use types http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:24: uint32 size, uint32 hardware_delay_bytes, these should be aligned at the ( http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:60: mock_manager_.reset(new StrictMock<MockAudioManagerLinuxInput>()); I don't see any expectations being set on mock_manager_ -- what are we using it for? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:87: // Render Callback allow 8 frames variance for SRC where is the variance coming from? http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:98: usleep(duration_ms * 1000); can this not be event driven in some way?
Thanks for helping Andrew, sorry I didn't get a response up earlier, was a crazy day in kernel space... http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:24: num_channels_(params.channels()), On 2012/06/21 21:39:46, scherkus wrote: > if you save params_ do you need to save sample_rate_ and num_channels_ ? Nice, I don't really need these two or pcm_format_, removed. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:100: DCHECK(!callback_ && callback); On 2012/06/21 21:39:46, scherkus wrote: > split these DCHECKs into two so if they fail we know which one is the culprit Done. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:155: if (!client_ || !callback_) On 2012/06/21 21:39:46, scherkus wrote: > is this a DCHECK/programmer error? Half and half, changed client_ to DCHECK. I left callback_ as an if as stop can still be called if start fails. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:191: timespec latency_ts = {0, 0}; On 2012/06/21 21:39:46, scherkus wrote: > nit: remove extra space before = Done. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:197: double latency_usec = (latency_ts.tv_sec * 1e6) + latency_ts.tv_nsec * 1e-3; On 2012/06/21 21:39:46, scherkus wrote: > magic numbers! > > you can use base/time.h for kMicrosecondsPerSecond instead of 1e6 and > kNanosecondsPerMicrosecond Done. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:224: return static_cast<double>(cras_client_get_system_max_capture_gain(client_)); On 2012/06/21 21:39:46, scherkus wrote: > ditto here -- see my concerns about casting below it's weird, see below. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:241: if (!client_) On 2012/06/21 21:39:46, scherkus wrote: > SetVolume() when client_ == NULL is DCHECK() but getting is OK? > > is anyone relying on being able to call GetVolume() today when client_ is NULL? Not that I can find. The only path to get here that I found was through the calls to UpdateAgcVolume and QueryAgcVolume from within this file. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input.cc:250: return static_cast<double>(current_volume); On 2012/06/21 21:39:46, scherkus wrote: > what's the range of value for current_volume? > > doesn't GetVolume() return a float between 0.0f and 1.0f? wouldn't casting mean > we either return 0 or 1 and nothing inbetween? I thought this was really strange too, but went with the precedent set by alsa_input after seeing that scaling is done up one layer in audio_input_controller.cc: // Only ask for the maximum volume at first call and use cached value // for remaining function calls. if (!max_volume_) { max_volume_ = stream_->GetMaxVolume(); } if (max_volume_ == 0.0) { DLOG(WARNING) << "Failed to access input volume control"; return; } // Set the stream volume and scale to a range matched to the platform. stream_->SetVolume(max_volume_ * volume); http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:23: MOCK_METHOD5(OnData, void(AudioInputStream* stream, const uint8* src, On 2012/06/21 21:39:46, scherkus wrote: > nit: you can drop the param names and just use types Done. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:24: uint32 size, uint32 hardware_delay_bytes, On 2012/06/21 21:39:46, scherkus wrote: > these should be aligned at the ( all fit on following line with param names removed. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:60: mock_manager_.reset(new StrictMock<MockAudioManagerLinuxInput>()); On 2012/06/21 21:39:46, scherkus wrote: > I don't see any expectations being set on mock_manager_ -- what are we using it > for? It is just needed for this last function(so the real one doesn't delete the stream). Removed the rest of the unused things. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:87: // Render Callback allow 8 frames variance for SRC On 2012/06/21 21:39:46, scherkus wrote: > where is the variance coming from? When sample rate converting for non-integer multiples, the size will varry from packet to packet due to residual samples left in the converter. It takes 8.7 192k samples to make one 22.050k sample. I'll update the comment to provide a bit more detail. http://codereview.chromium.org/10592014/diff/4002/media/audio/linux/cras_inpu... media/audio/linux/cras_input_unittest.cc:98: usleep(duration_ms * 1000); On 2012/06/21 21:39:46, scherkus wrote: > can this not be event driven in some way? Would that provide any benefit? I'm not a GMock expert but I assumed I had to block before returning from the test function so the results would be valid. Not true?
http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:100: DCHECK(callback); Currently we don't have user case for it, but I think we should allow the users to call start/stop multiple times. We can simply remove the DCHECK and add a flag like started_ to protect the functions. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:32: // We need to override this function in order to skip the checking the number skip checking http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:62: return new CrasInputStream(params, mock_manager_.get()); I am wondering what is preventing using audio_manager->MakeAudioInputStream? http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:67: CrasInputStream* test_stream = new CrasInputStream(params, Use CreateStream() here? http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:93: static const unsigned int kTestBitsPerSample; Isn't it easier to extend the tests if we put these variables to members, and overwrite them for different values when creating streams. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:134: new CrasInputStream(bad_bps_params, mock_manager_.get()); We can use CreateStream instead. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:146: new CrasInputStream(bad_format_params, mock_manager_.get()); ditto
http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:100: DCHECK(callback); On 2012/06/25 15:59:27, xians1 wrote: > Currently we don't have user case for it, but I think we should allow the users > to call start/stop multiple times. > We can simply remove the DCHECK and add a flag like started_ to protect the > functions. Since this function is void I just automatically stopped if currently playing. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:32: // We need to override this function in order to skip the checking the number On 2012/06/25 15:59:27, xians1 wrote: > skip checking Done. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:62: return new CrasInputStream(params, mock_manager_.get()); On 2012/06/25 15:59:27, xians1 wrote: > I am wondering what is preventing using audio_manager->MakeAudioInputStream? Isn't that something that belongs in an AudioManager unit test? http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:67: CrasInputStream* test_stream = new CrasInputStream(params, On 2012/06/25 15:59:27, xians1 wrote: > Use CreateStream() here? CreateStream is used to create a stream with the default settings specified by kTestBitsPerSample and friends. This needs to override those defaults so calling CreateStream doesn't work. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:93: static const unsigned int kTestBitsPerSample; On 2012/06/25 15:59:27, xians1 wrote: > Isn't it easier to extend the tests if we put these variables to members, and > overwrite them for different values when creating streams. These are just meant as defaults, When they need to be over-ridden it is best to create audio parameters with each value set explicitly.
http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:89: if (callback_) I think the speech recognition team needs this callback. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:62: return new CrasInputStream(params, mock_manager_.get()); On 2012/06/25 21:29:28, dgreid wrote: > On 2012/06/25 15:59:27, xians1 wrote: > > I am wondering what is preventing using audio_manager->MakeAudioInputStream? > Isn't that something that belongs in an AudioManager unit test? We don't have the specific AudioManager unit test, and usually we test it in the platform specific unittest like this. I know the Cras class in behind a flag, so if it is not easy, please ignore my comment. Otherwise we prefer using MakeAudioInputStream. http://codereview.chromium.org/10592014/diff/10001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:67: CrasInputStream* test_stream = new CrasInputStream(params, On 2012/06/25 21:29:28, dgreid wrote: > On 2012/06/25 15:59:27, xians1 wrote: > > Use CreateStream() here? > CreateStream is used to create a stream with the default settings specified by > kTestBitsPerSample and friends. This needs to override those defaults so > calling CreateStream doesn't work. The code looks much cleaner if we only use one way to create the stream. What about passing AudioParameters to CreateStream as input for the case overriding the defaults? http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:99: if (callback_) use an extra flag like started_ to protect calling start consecutively. And we should simply return in case it is already playing. http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:167: callback_ = NULL; I think we should keep OnClose callback in Close() and do not set the callback_ to NULL.
http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:259: return static_cast<double>(current_volume); maddening!! want to add some docs to say this is intentional? also do you think we should fix this API + file a bug? it seems confusing :( http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:85: usleep(duration_ms * base::Time::kMillisecondsPerSecond); when running on build bots if OnData() isn't called within the 250ms deadline we'll get a gmock failure a less flaky solution is to use a WaitableEvent that is signaled in the expectation to OnData() base::WaitableEvent event(false, false); EXPECT_CALL(...) .WillRepeatedly(InvokeWithoutArgs(&event, &base::WaitableEvent::Signal)); EXPECT_TRUE(event.TimedWait(base::TestTimeouts::action_timeout_ms()); Check out audio_output_controller_unittest.cc: http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_output_c... the syntax is a little hairy so if you don't want to clutter the test code you could alternatively have the MockAudioInputCallback class have a WaitableEvent itself and implement OnData() to signal, and expose a WaitForOnData() method (or similar) http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:181: // Then re-start the stream with a seconds callback. s/seconds/second
Thanks for the commetns fells much better now, will push new patch set in the morning as soon as the build works again so I can test it. http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... File media/audio/linux/cras_input.cc (right): http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:99: if (callback_) On 2012/06/26 21:42:50, xians1 wrote: > use an extra flag like started_ to protect calling start consecutively. And we > should simply return in case it is already playing. Done. http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:167: callback_ = NULL; On 2012/06/26 21:42:50, xians1 wrote: > I think we should keep OnClose callback in Close() and do not set the callback_ > to NULL. Done. http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input.cc:259: return static_cast<double>(current_volume); On 2012/06/26 22:10:10, scherkus wrote: > maddening!! want to add some docs to say this is intentional? > > also do you think we should fix this API + file a bug? it seems confusing :( It is confusing. I should have thought about this some more before copying the alsa_input code. In the next patchset I changed this to convert from the passed volume ratio to the alsa representation in decibels * 100. Makes this much cleaner, we should consider doing the same for alsa. http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:85: usleep(duration_ms * base::Time::kMillisecondsPerSecond); On 2012/06/26 22:10:10, scherkus wrote: > when running on build bots if OnData() isn't called within the 250ms deadline > we'll get a gmock failure > > a less flaky solution is to use a WaitableEvent that is signaled in the > expectation to OnData() > > base::WaitableEvent event(false, false); > EXPECT_CALL(...) > .WillRepeatedly(InvokeWithoutArgs(&event, &base::WaitableEvent::Signal)); > EXPECT_TRUE(event.TimedWait(base::TestTimeouts::action_timeout_ms()); > > Check out audio_output_controller_unittest.cc: > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/audio_output_c... > > the syntax is a little hairy so if you don't want to clutter the test code you > could alternatively have the MockAudioInputCallback class have a WaitableEvent > itself and implement OnData() to signal, and expose a WaitForOnData() method (or > similar) Slick, Since being called once is sufficient, this with a WillOnce() makes the test way faster, Thanks! http://codereview.chromium.org/10592014/diff/19001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:181: // Then re-start the stream with a seconds callback. On 2012/06/26 22:10:10, scherkus wrote: > s/seconds/second removed.
Now with faster-running unit test, and disabled double-start. The volume setting was re-work. If this looks good I'll do something similar for the ALSA input. Thanks again for looking.
LGTM w/ nits on tests http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:183: nit: remove extra blank line http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:198: CaptureSomeFrames(params_mono, kTestCaptureDurationMs); if this fails we'll have no idea which combo of sample rate or channels failed! I'd recommend either using SCOPED_TRACE() around calls to CaptureSomeFrames() so that if it fails it'll print out the message associated w/ the failure: http://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_Su... Alternatively you can parameterize this particular test: http://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_Tests
note I'm heading off to vacation so I'll be OOO for the next ~10 days!
http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:183: On 2012/06/28 01:36:44, scherkus wrote: > nit: remove extra blank line Done. http://codereview.chromium.org/10592014/diff/29001/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:198: CaptureSomeFrames(params_mono, kTestCaptureDurationMs); On 2012/06/28 01:36:44, scherkus wrote: > if this fails we'll have no idea which combo of sample rate or channels failed! > > I'd recommend either using SCOPED_TRACE() around calls to CaptureSomeFrames() so > that if it fails it'll print out the message associated w/ the failure: > http://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_Su... > > > Alternatively you can parameterize this particular test: > http://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_Tests Very cool, added SCOPED_TRACEs for sample rate and mono/stereo.
On 2012/06/28 01:36:59, scherkus wrote: > note I'm heading off to vacation so I'll be OOO for the next ~10 days! Have a fun vacation!
On 2012/06/28 01:53:01, dgreid wrote: > On 2012/06/28 01:36:59, scherkus wrote: > > note I'm heading off to vacation so I'll be OOO for the next ~10 days! > > Have a fun vacation! lgtm with one nit.
http://codereview.chromium.org/10592014/diff/30006/media/audio/linux/cras_inp... File media/audio/linux/cras_input_unittest.cc (right): http://codereview.chromium.org/10592014/diff/30006/media/audio/linux/cras_inp... media/audio/linux/cras_input_unittest.cc:11: #include "base/synchronization/waitable_event.h" fix the order.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/10592014/46001
Try job failure for 10592014-46001 (retry) (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/10592014/46001
Try job failure for 10592014-46001 (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/10592014/46001
Try job failure for 10592014-46001 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/10592014/46001
Try job failure for 10592014-46001 (retry) on win_rel for step "runhooks". It's a second try, previously, step "runhooks" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgreid@chromium.org/10592014/46001
Change committed as 146881 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
