Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652803002:
Refactor FakeAudioDevice to have separate methods for starting recording and playout. (Closed)
|
Created:
3 years, 11 months ago by perkj_webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor FakeAudioDevice to have separate methods for starting recording and playout.
Also, change FakeAudioDevice to generate a sine tone instead of using a file.
TBR=henrika@webrtc.org, stefan@webrtc.org
BUG=webrtc:7080
Review-Url: https://codereview.webrtc.org/2652803002
Cr-Commit-Position: refs/heads/master@{#16385}
Committed: https://chromium.googlesource.com/external/webrtc/+/ac61b745df8eb918e8a39368fec7d7c3a890f221
Patch Set 1 #Patch Set 2 : Made some methods private. Readded init. #
Total comments: 16
Patch Set 3 : Addressed comments. #
Total comments: 21
Patch Set 4 : Addressed peahs comments. #Patch Set 5 : Fix bug with sample size. #
Total comments: 9
Patch Set 6 : Use 48000KHz sample rate. Changed from sine to a noise pulse. #
Total comments: 17
Patch Set 7 : Addressed review comments. #
Total comments: 36
Patch Set 8 : Removed drifting clock. #
Total comments: 22
Patch Set 9 : Addressed comments. #
Total comments: 7
Patch Set 10 : Moved initialization to ctor. Made StartRecording and StartPlayout to private. Fixed call_test #
Total comments: 7
Patch Set 11 : Addressed solenbergs comments. #Patch Set 12 : fix size_t #
Messages
Total messages: 71 (42 generated)
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
perkj@webrtc.org changed reviewers: + solenberg@webrtc.org
Please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
solenberg@webrtc.org changed reviewers: + peah@webrtc.org
A few comments. Don't have time to look more right now. peah@, can you take a look at ProcessAudio()? https://codereview.webrtc.org/2652803002/diff/20001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:273: fake_audio_device.StopPlayout(); nit: stop/start in same order - these operations shouldn't have any shared state that needs undoing in a specific order. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:14: #include <cmath> math.h https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:26: const double kPi = std::acos(-1); constexpr? https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (left): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:65: std::unique_ptr<ModuleFileUtility> file_utility_; Are there any build file deps you can remove now? https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:31: FakeAudioDevice(Clock* clock, float speed); What's "speed"? https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:32: virtual ~FakeAudioDevice(); override https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:61: double normalized_frequency_ GUARDED_BY(lock_); float https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:62: uint16_t peak_to_peak_ GUARDED_BY(lock_); float https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:63: double n_; float
ptal https://codereview.webrtc.org/2652803002/diff/20001/webrtc/call/call_perf_tes... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/call/call_perf_tes... webrtc/call/call_perf_tests.cc:273: fake_audio_device.StopPlayout(); On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > nit: stop/start in same order - these operations shouldn't have any shared state > that needs undoing in a specific order. Done. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (left): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:65: std::unique_ptr<ModuleFileUtility> file_utility_; On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > Are there any build file deps you can remove now? Done. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:31: FakeAudioDevice(Clock* clock, float speed); On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > What's "speed"? Added a comment. Note that this is nothing I want to change. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:32: virtual ~FakeAudioDevice(); On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > override Done. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:61: double normalized_frequency_ GUARDED_BY(lock_); On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > float Done. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:62: uint16_t peak_to_peak_ GUARDED_BY(lock_); On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > float Done. https://codereview.webrtc.org/2652803002/diff/20001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:63: double n_; On 2017/01/24 08:58:12, the sun (OOO until Jan 30th) wrote: > float Done.
I'm not fully sure about the background to this class but my immediate feeling is that it should be two separate classes, one for recording and one for capturing. I see the point of having a generic audio device interface but I think that inside the class the recording and capturing functionalities should be separated into two objects. How is it on a real audio device (one that both supports reading and capturing): are the capture and render events always synchronized in time, e.g., is there a simultaneous read and write operation? The current implementation simulates this as being fully synchronized. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:90: if (recording_) { Is there a certain reason for changing the capturing_ variable to recording_ and playing_? -Why not use capturing_ and rendering_? -Can both playing and recording_ be active at the same time? https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:92: for (uint32_t i = 0; i < kFrequencyHz / 100; i = i + 2) { uint32_t -> size_t ? https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:93: n_ += normalized_frequency_; There are a lot of members to this class that only relate to the recording_ call. I'd have preferred if those would be bundled into a class in order to separate the recording part of the code from the playing part of the code. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:98: static_cast<uint16_t>((1.0 + std::sin(n_) / 2) * peak_to_peak_); Since 1.0 + sin()/2 obtains values in the range [0.5:1.5], the cast is saturating for high values of peak_to_peak. It would make sense to document that via a DCHECK. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:99: captured_audio_[i] = sample; What is the sample format for this? Is that a standard format? https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:126: tick_->Wait(WEBRTC_EVENT_INFINITE); As it is now, tick controls the rate of calls for both playing and recording. Is that desired? In practice, I'd imagine that you'd also want to simulate different rates for this. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:142: uint16_t peak_to_peak) { Please add DCHECKS for the allowed range of peak_to_peak (see above comment). https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (left): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:31: class FakeAudioDevice : public FakeAudioDeviceModule { Would it make sense to change the name of the class? Before it was a fake device that read any audio data from a file. Now it is a fake device that generates audio data as a sinusoid. Based on that I think it can be possible to make the class name more descriptive, such as "SinusoidFakeAudioDevice" (or something much better than my suggestion). https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:34: FakeAudioDevice(Clock* clock, float speed); I guess that speed is the multiplying factor applied to the rate, right? Would it not be more straightforward to have this in percent with a sign? When I talk about drift I typically talk in terms of percent faster and slower. If the drift would be specified in percent, I imagine it would be simpler for the user to use this code. Or is the idea that this in practice always should be used with the DriftingClock class? WDYT? https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:55: static const uint32_t kFrequencyHz = 16000; This restricts the rate of the audio to be 16 kHz. Is that desired? If it is, please document that, either by nameing the class accordingly or by adding comments. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:66: float n_; Please change n_ to something more verbose.
ptal https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:90: if (recording_) { On 2017/01/24 12:42:21, peah-webrtc wrote: > Is there a certain reason for changing the capturing_ variable to recording_ and > playing_? > -Why not use capturing_ and rendering_? > -Can both playing and recording_ be active at the same time? > > done and yes, there are current calltests that starts both recording and playout. I think it is a loopback test and I did not want to change that. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:92: for (uint32_t i = 0; i < kFrequencyHz / 100; i = i + 2) { On 2017/01/24 12:42:21, peah-webrtc wrote: > uint32_t -> size_t ? if you prefer that sure. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:98: static_cast<uint16_t>((1.0 + std::sin(n_) / 2) * peak_to_peak_); On 2017/01/24 12:42:20, peah-webrtc wrote: > Since 1.0 + sin()/2 obtains values in the range [0.5:1.5], the cast is > saturating for high values of peak_to_peak. It would make sense to document that > via a DCHECK. oops, I wanted it to be between 0 and 1 just to make sure that can not happen. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:99: captured_audio_[i] = sample; On 2017/01/24 12:42:21, peah-webrtc wrote: > What is the sample format for this? Is that a standard format? I was hoping you guys know. The old comments for the |captured_audio| just said it was 2 bytes per sample. To me it does not matter. I just want to a have a fake device that spits out something I can use for tests in another project. Current tests does not seem to care either. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:126: tick_->Wait(WEBRTC_EVENT_INFINITE); On 2017/01/24 12:42:21, peah-webrtc wrote: > As it is now, tick controls the rate of calls for both playing and recording. Is > that desired? In practice, I'd imagine that you'd also want to simulate > different rates for this. Maybe, but that is not a change in this cl and nothing I currently care about. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:142: uint16_t peak_to_peak) { On 2017/01/24 12:42:21, peah-webrtc wrote: > Please add DCHECKS for the allowed range of peak_to_peak (see above comment). should not happen now. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (left): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:31: class FakeAudioDevice : public FakeAudioDeviceModule { On 2017/01/24 12:42:21, peah-webrtc wrote: > Would it make sense to change the name of the class? Before it was a fake device > that read any audio data from a file. > Now it is a fake device that generates audio data as a sinusoid. Based on that I > think it can be possible to make the class name more descriptive, such as > "SinusoidFakeAudioDevice" (or something much better than my suggestion). VoiceEngine seems to use just one device and it is is used for both playout and capturing. So then I guess it does not really make sence to call this something that just applies to capturing. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:34: FakeAudioDevice(Clock* clock, float speed); On 2017/01/24 12:42:21, peah-webrtc wrote: > I guess that speed is the multiplying factor applied to the rate, right? > Would it not be more straightforward to have this in percent with a sign? When I > talk about drift I typically talk in terms of percent faster and slower. If the > drift would be specified in percent, I imagine it would be simpler for the user > to use this code. > > Or is the idea that this in practice always should be used with the > DriftingClock class? > > WDYT? > I think that this class and the interface it implements needs some love by someone working on audio. Having separate interfaces for capturing and playout would be one idea. I don't care about drift and would like to not change it in this cl. But I am pretty sure you are right. How about I move the recording parts that I have chagnged into a separate inner class that the FakeAudioAudioDevice can use. If it turns out that more implementations of a capturers are needed it is straight forward to do so. I leave the playout parts untouched, including the drifting thingies. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:55: static const uint32_t kFrequencyHz = 16000; On 2017/01/24 12:42:21, peah-webrtc wrote: > This restricts the rate of the audio to be 16 kHz. Is that desired? > > If it is, please document that, either by nameing the class accordingly or by > adding comments. I don't know. I am just trying to make sure I can use this class for a new test and make sure it does not need a file. https://codereview.webrtc.org/2652803002/diff/40001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:66: float n_; On 2017/01/24 12:42:21, peah-webrtc wrote: > Please change n_ to something more verbose. sine_step_?
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:36: uint16_t peak_to_peak) Do you really need to specify frequency_in_hz and peak_to_peak? Is the ability to choose those a feature that will be used? Would it be possible to hardcode the signal features in this class? https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:48: sine_step_ += normalized_frequency_; As we discussed offline you can use a pulsed noise sequence here. Generating int16_t noise values can be done by code similar to void RandomizeSampleVector(Random* random_generator, rtc::ArrayView<float> v) { + for (auto& v_k : v) { + v_k = 2 * 32767.f * random_generator->Rand<float>() - 32767.f; + } +} https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:114: return capturer_ != nullptr; Since capturer_ is a unique_ptr, you can return whether it is set without checking for it to be null, eg. "return capturer_;" (or maybe "return !!capturer_;" https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:149: int32_t FakeAudioDevice::StartPlayout() { Please order the methods in the same order as they are in the class definition. https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.h (left): https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.h:33: FakeAudioDevice(Clock* clock, const std::string& filename, float speed); It seems to me very strange that the sample rate is not part of the constructor call. Afaics, the rate is not hardcoded to be 16 kHz, which means that it will produce 16 kHz of data. Is that desired and sufficient for the usage of this? For instance, the audio processing differs quite a lot if you use 16 or 48 kHz.
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
ptal https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:36: uint16_t peak_to_peak) On 2017/01/25 10:45:27, peah-webrtc wrote: > Do you really need to specify frequency_in_hz and peak_to_peak? Is the ability > to choose those a feature that will be used? Would it be possible to hardcode > the signal features in this class? yes, now use pulsed noise as you suggested. https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:48: sine_step_ += normalized_frequency_; On 2017/01/25 10:45:27, peah-webrtc wrote: > As we discussed offline you can use a pulsed noise sequence here. > > Generating int16_t noise values can be done by code similar to > > void RandomizeSampleVector(Random* random_generator, rtc::ArrayView<float> v) { > + for (auto& v_k : v) { > + v_k = 2 * 32767.f * random_generator->Rand<float>() - 32767.f; > + } > +} Done. https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:114: return capturer_ != nullptr; On 2017/01/25 10:45:27, peah-webrtc wrote: > Since capturer_ is a unique_ptr, you can return whether it is set without > checking for it to be null, > eg. > "return capturer_;" > > (or maybe "return !!capturer_;" Done. https://codereview.webrtc.org/2652803002/diff/80001/webrtc/test/fake_audio_de... webrtc/test/fake_audio_device.cc:149: int32_t FakeAudioDevice::StartPlayout() { On 2017/01/25 10:45:28, peah-webrtc wrote: > Please order the methods in the same order as they are in the class definition. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great! Thanks for the new patch! I have some comments though. PTAL https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:27: random_audio_.SetSize(num_samples); Instead add random_audio_(num_samples, 0) to the initializer list https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:28: null_audio_.SetSize(num_samples); Instead add null_audio_(num_samples, 0) to the initializer list https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:29: memset(null_audio_.data(), 0, null_audio_.size() * sizeof(int16_t)); This can be removed if you use a vector instead. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:32: for (size_t i = 0; i < num_samples; ++i) { You can instead use for (auto& r : random_audio_) { r = random_generator.Rand(-max_amplitude, max_amplitude); } But I think you should move this to the Capture() method. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:37: rtc::ArrayView<int16_t> Capture() { rtc::ArrayView<int16_t> returns an object that can be used to modify the returned buffer. I guess that is not what you want, right? Instead I guess you should use rtc::ArrayView<const int16_t>? Or is it the way that RecordedDataIsAvailable operates that requires this to be non-const https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:38: fill_with_zero_ = !fill_with_zero_; I think you should do this with proper (nonrepeating) random data instead. e.g,: rtc::ArrayView<int16_t> Capture() { fill_with_zero_ = !fill_with_zero_; if (!fill_with_zero_) { for (auto& r : random_audio_) { r = random_generator_.Rand(-max_amplitude, max_amplitude); } } return fill_with_zero_ ? null_audio_ : random_audio_; } Or even cooler rtc::ArrayView<int16_t> Capture() { fill_with_zero_ = !fill_with_zero_; if (!fill_with_zero_) { std::generate(random_audio_.begin(), random_audio_.end(), [&]() { return random_generator_.Rand(-max_amplitude, max_amplitude); }) return fill_with_zero_ ? null_audio_ : random_audio_; } Note, however, that for these variants random_generator_ must be a member variable. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:44: rtc::BufferT<int16_t> random_audio_; Since the sizes of these are constant, I'd rather use std::vector<int16_t> instead of rtc::BufferT. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:60: playout_buffer_.SetSize(sampling_frequency_in_hz_ / 100); Change this to vector, and set the size in the initializer list. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:67: rtc::BufferT<int16_t> playout_buffer_; Since the size of playout_buffer_ is static, I think it is better to use std::vector instead. That allows you to set the length in the initializer list as well.
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I learned a few tricks. Thanks PTAL https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:27: random_audio_.SetSize(num_samples); On 2017/01/26 09:23:59, peah-webrtc wrote: > Instead add random_audio_(num_samples, 0) to the initializer list Done. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:28: null_audio_.SetSize(num_samples); On 2017/01/26 09:23:59, peah-webrtc wrote: > Instead add null_audio_(num_samples, 0) to the initializer list Done. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:29: memset(null_audio_.data(), 0, null_audio_.size() * sizeof(int16_t)); On 2017/01/26 09:23:59, peah-webrtc wrote: > This can be removed if you use a vector instead. I did not know I could use a std::vector. c++11 news apparently. Thanks http://en.cppreference.com/w/cpp/container/vector/data https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:32: for (size_t i = 0; i < num_samples; ++i) { On 2017/01/26 09:23:59, peah-webrtc wrote: > You can instead use > for (auto& r : random_audio_) { > r = random_generator.Rand(-max_amplitude, max_amplitude); > } > > But I think you should move this to the Capture() method. ok, I did that first but decided against it since I guessed one cared and I could save the cycles. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:37: rtc::ArrayView<int16_t> Capture() { On 2017/01/26 09:23:59, peah-webrtc wrote: > rtc::ArrayView<int16_t> returns an object that can be used to modify the > returned buffer. I guess that is not what you want, right? > > Instead I guess you should use rtc::ArrayView<const int16_t>? Or is it the way > that RecordedDataIsAvailable operates that requires this to be non-const > Done. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:38: fill_with_zero_ = !fill_with_zero_; On 2017/01/26 09:23:59, peah-webrtc wrote: > I think you should do this with proper (nonrepeating) random data instead. > e.g,: > rtc::ArrayView<int16_t> Capture() { > fill_with_zero_ = !fill_with_zero_; > if (!fill_with_zero_) { > for (auto& r : random_audio_) { > r = random_generator_.Rand(-max_amplitude, max_amplitude); > } > } > return fill_with_zero_ ? null_audio_ : random_audio_; > } > > Or even cooler > rtc::ArrayView<int16_t> Capture() { > fill_with_zero_ = !fill_with_zero_; > if (!fill_with_zero_) { > std::generate(random_audio_.begin(), random_audio_.end(), [&]() { return > random_generator_.Rand(-max_amplitude, max_amplitude); }) > return fill_with_zero_ ? null_audio_ : random_audio_; > } > > > Note, however, that for these variants random_generator_ must be a member > variable. nice. I like std::generate. https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:44: rtc::BufferT<int16_t> random_audio_; On 2017/01/26 09:23:59, peah-webrtc wrote: > Since the sizes of these are constant, I'd rather use std::vector<int16_t> > instead of rtc::BufferT. sure, I did not know about std::vector::data() https://codereview.webrtc.org/2652803002/diff/100001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:60: playout_buffer_.SetSize(sampling_frequency_in_hz_ / 100); On 2017/01/26 09:23:59, peah-webrtc wrote: > Change this to vector, and set the size in the initializer list. Done.
Thanks for the update! I have some more comments which need to be addressed, though. PTAL https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:20: namespace webrtc { If you here add namespace { constexpr size_t kFrameLengthMs = 10; constexpr size_t kFramesPerSecond = 1000 / kFrameLengthMs; } // namespace https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:49: std::vector<int16_t> null_audio_; You probably should rename this to silent_audio_, since null is quite well-defined in the area of programming and means something very different. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:58: // Assuming 10ms audio packets. This should be documented in the header file as it is a contract between the class and the user of the code. I think you should remove the comment (as it is redundant) and document this in the code instead. I'll add some suggestions in other comments. Probably you should document that the sample rate can only be 8, 16, 32, 44.1, or 48 kHz as well. Preferably, you should add a DCHECK on the correctness of the sample rate here in the constructor. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:59: playout_buffer_(sampling_frequency_in_hz_ / 100, 0), playout_buffer_(rtc::CheckedDivExact(sampling_frequency_in_hz_, kFramesPerSecond), 0), https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:64: thread_(FakeAudioDevice::Run, this, "FakeAudioDevice") {} Please add RTC_DCHECK(clock); (because clock should be a valid pointer, right?) https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:88: sampling_frequency_in_hz_ / 100, max_amplitude)); rtc::CheckedDivExact(sampling_frequency_in_hz_, kFramesPerSecond) https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:98: RTC_CHECK(tick_->StartTimer(true, 10 / speed_)); RTC_CHECK(tick_->StartTimer(true, kFrameLengthMs / speed_)); https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:104: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { Is a callback allowed to be null? If it is not, please add a DCHECK on that. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:116: *delay_ms = 0; Please add RTC_DCHECK(delay_ms); https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:144: size_t samples_needed = sampling_frequency_in_hz_ / 100; With the code construct below, you can skip the comment, ensure that you don't get any wrong number of samples and can avoid having related constants in the code. size_t samples_needed = rtc::CheckedDivExact(sampling_frequency_in_hz_, kFramesPerSecond); https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:146: uint32_t time_since_last_playout_ms = now_ms - last_playout_ms_; I don't see you ever updating last_playout_ms_. How does this really work then? Afaics, the if-statement below should always be false (since last_playout_ms_ is initialized to -1, which in turn means that samples_needed is always 10 ms in length, right? https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:147: if (last_playout_ms_ > 0 && time_since_last_playout_ms > 0) { It would actually be fine to initialize last_playout_ms_ to 0 (instead of to -1 as it is now). In that case you can reduce the if-statement to if (time_since_last_playout_ms > 0) { Furthermore, with that there is no risk that time_since_last_playout_ms wraps around due to last_playout_ms_ being -1 and now_ms being 0 (which I think it can be according to the specification of DriftingClock). https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:149: std::min(static_cast<size_t>(sampling_frequency_in_hz_ / If you change sampling_frequency_in_hz_ to be size_t, you do not need the static_cast. However, I guess you cannot do that due to the NeedMorePlayData call, right? Then I think you should instead change time_since_last_playout_ms to be int. That would shorten this code line significantly. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:67: std::vector<int16_t> playout_buffer_; playout_buffer_ should also be guarded by lock_, right? https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:69: int64_t last_playout_ms_; last_playout_ms_ should also be guarded by lock_, right? https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:72: std::unique_ptr<EventTimerWrapper> tick_; Does clock_ and tick_ need to be guarded by lock_? Afaics, they are accessed while holding the lock which means that I think they should be annotated to require that.
One additional comment. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:136: uint32_t new_mic_level; Please initialize new_mic_level.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
perkj@webrtc.org changed reviewers: + danilchap@webrtc.org
Adding Danil as well since he seemed to have made the last modifications to the tests using the drift. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:20: namespace webrtc { On 2017/01/26 13:20:11, peah-webrtc wrote: > If you here add > namespace { > > constexpr size_t kFrameLengthMs = 10; > constexpr size_t kFramesPerSecond = 1000 / kFrameLengthMs; > > } // namespace ok, but will use int https://google.github.io/styleguide/cppguide.html#Integer_Types https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:49: std::vector<int16_t> null_audio_; On 2017/01/26 13:20:11, peah-webrtc wrote: > You probably should rename this to silent_audio_, since null is quite > well-defined in the area of programming and means something very different. Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:58: // Assuming 10ms audio packets. On 2017/01/26 13:20:11, peah-webrtc wrote: > This should be documented in the header file as it is a contract between the > class and the user of the code. > > I think you should remove the comment (as it is redundant) and document this in > the code instead. I'll add some suggestions in other comments. > > Probably you should document that the sample rate can only be 8, 16, 32, 44.1, > or 48 kHz as well. > Preferably, you should add a DCHECK on the correctness of the sample rate here > in the constructor. Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:59: playout_buffer_(sampling_frequency_in_hz_ / 100, 0), On 2017/01/26 13:20:11, peah-webrtc wrote: > playout_buffer_(rtc::CheckedDivExact(sampling_frequency_in_hz_, > kFramesPerSecond), 0), Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:64: thread_(FakeAudioDevice::Run, this, "FakeAudioDevice") {} On 2017/01/26 13:20:11, peah-webrtc wrote: > Please add > RTC_DCHECK(clock); > > (because clock should be a valid pointer, right?) now only used by the drifting clock and that checks. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:88: sampling_frequency_in_hz_ / 100, max_amplitude)); On 2017/01/26 13:20:11, peah-webrtc wrote: > rtc::CheckedDivExact(sampling_frequency_in_hz_, > kFramesPerSecond) Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:98: RTC_CHECK(tick_->StartTimer(true, 10 / speed_)); On 2017/01/26 13:20:11, peah-webrtc wrote: > RTC_CHECK(tick_->StartTimer(true, kFrameLengthMs / speed_)); Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:104: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { On 2017/01/26 13:20:11, peah-webrtc wrote: > Is a callback allowed to be null? If it is not, please add a DCHECK on that. seems like its called with null to reconfigure. added dcheck for this. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:116: *delay_ms = 0; On 2017/01/26 13:20:11, peah-webrtc wrote: > Please add > RTC_DCHECK(delay_ms); A chrome reviewer would say no since it will generate a crash in any case. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:136: uint32_t new_mic_level; On 2017/01/26 13:25:18, peah-webrtc wrote: > Please initialize new_mic_level. Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:144: size_t samples_needed = sampling_frequency_in_hz_ / 100; On 2017/01/26 13:20:12, peah-webrtc wrote: > With the code construct below, you can skip the comment, ensure that you don't > get any wrong number of samples and can avoid having related constants in the > code. > > size_t samples_needed = rtc::CheckedDivExact(sampling_frequency_in_hz_, > kFramesPerSecond); > > I removed all this weird stuff since the clock has never been used. last_playout_ms_ has not been updated. I now always request the same number of samples. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:146: uint32_t time_since_last_playout_ms = now_ms - last_playout_ms_; On 2017/01/26 13:20:12, peah-webrtc wrote: > I don't see you ever updating last_playout_ms_. How does this really work then? > Afaics, the if-statement below should always be false (since last_playout_ms_ is > initialized to -1, which in turn means that samples_needed is always 10 ms in > length, right? Acknowledged. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:147: if (last_playout_ms_ > 0 && time_since_last_playout_ms > 0) { On 2017/01/26 13:20:11, peah-webrtc wrote: > It would actually be fine to initialize last_playout_ms_ to 0 (instead of to -1 > as it is now). > > In that case you can reduce the if-statement to > if (time_since_last_playout_ms > 0) { > > Furthermore, with that there is no risk that time_since_last_playout_ms wraps > around due to last_playout_ms_ being -1 and now_ms being 0 (which I think it can > be according to the specification of DriftingClock). > Acknowledged. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:149: std::min(static_cast<size_t>(sampling_frequency_in_hz_ / On 2017/01/26 13:20:11, peah-webrtc wrote: > If you change sampling_frequency_in_hz_ to be size_t, you do not need the > static_cast. > > However, I guess you cannot do that due to the NeedMorePlayData call, right? > > Then I think you should instead change time_since_last_playout_ms to be int. > > That would shorten this code line significantly. NeedMorePlayData actually use size_t but style guide recommend int... https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:67: std::vector<int16_t> playout_buffer_; On 2017/01/26 13:20:12, peah-webrtc wrote: > playout_buffer_ should also be guarded by lock_, right? Done. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:69: int64_t last_playout_ms_; On 2017/01/26 13:20:12, peah-webrtc wrote: > last_playout_ms_ should also be guarded by lock_, right? removed https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:72: std::unique_ptr<EventTimerWrapper> tick_; On 2017/01/26 13:20:12, peah-webrtc wrote: > Does clock_ and tick_ need to be guarded by lock_? Afaics, they are accessed > while holding the lock which means that I think they should be annotated to > require that. removed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the update! I think the class looks great now, but I have some concerns regarding -The DCHECK in RegisterAudioCallback which seems weird. -The overriding method implementations RegisterAudioCallback and PlayoutDelay being unnecessary since that code is never used. If I'm correct with that, I would advocate that it would be best to remove that code (since unused code is untested core and untested code is dangerous code). lgtm conditioned on that you look into whether the concerns above are valid and addresses them in case they are. https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:116: *delay_ms = 0; On 2017/01/29 13:25:19, perkj_webrtc wrote: > On 2017/01/26 13:20:11, peah-webrtc wrote: > > Please add > > RTC_DCHECK(delay_ms); > > A chrome reviewer would say no since it will generate a crash in any case. That is interesting. Can one always rely on this crashing in a controlled way? What about debuggability, can one always rely on it being straightforward to locate the location of the crash? (I'm fine with not using the DCHECK, but curious about the impact.) https://codereview.webrtc.org/2652803002/diff/120001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:149: std::min(static_cast<size_t>(sampling_frequency_in_hz_ / On 2017/01/29 13:25:19, perkj_webrtc wrote: > On 2017/01/26 13:20:11, peah-webrtc wrote: > > If you change sampling_frequency_in_hz_ to be size_t, you do not need the > > static_cast. > > > > However, I guess you cannot do that due to the NeedMorePlayData call, right? > > > > Then I think you should instead change time_since_last_playout_ms to be int. > > > > That would shorten this code line significantly. > > NeedMorePlayData actually use size_t but style guide recommend int... I think the style guide leaves some room for interpretation there. I always get the feedback that lengths and sizes should be stored as size_t (and hence consistently write my code accordingly). Afaics, the style guide says it is ok to do so, but advises that due to the storage requirements int's should be used when you know that the data fits into an int (right?). In this case, I personally would use that freedom in the style guide to choose a variable type that avoids the extra code needed for the cast. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:312: fake_send_audio_device_.reset(new FakeAudioDevice(1.0, 48000)); Nit: Should be 1.f instead of 1.0 as the first constructor argument is a float. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:313: fake_recv_audio_device_.reset(new FakeAudioDevice(1.0, 48000)); Nit: Should be 1.f instead of 1.0 as the first constructor argument is a float. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:54: webrtc::Random random_generator_; Since you are in the namespace webrtc, I don't think webrtc:: is needed. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:113: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { Since this is not used anywhere in the class and is private, do you need to override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I guess you should just be able to omit this implementation. Or did I miss anything there? https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != nullptr); This DCHECK I don't follow. It basically checks that callback is not falsy (nullptr) and that audio_callback_ != nullptr, right? (as || has a higher precedence than != (or did I get that wrong?). I guess that what you want to check is RTC_DCHECK(callback || audio_callback_)? (which is the same, but more consistent) Another thing that I don't get with this DCHECK is that it allows RegisterAudioCallback to be called with callback = nullptr if audio_callback_==nullptr, but not otherwise. Is that intended? https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:125: int32_t FakeAudioDevice::PlayoutDelay(uint16_t* delay_ms) const { Since PlayoutDelay, and it is not used inside this class, you don't need to override it and can instead rely on the default implementation in the base class (or did I miss anything there?) https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:148: RTC_CHECK_EQ( Do we really want to crash this code if the return code differs from 0? https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:157: RTC_CHECK_EQ(0, audio_callback_->NeedMorePlayData( Do we really want to crash this code if the return code differs from 0?
Ah, hold on, you need a BUG issue number as well.
Description was changed from ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. BUG=none ========== to ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. BUG=7080 ==========
Description was changed from ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. BUG=7080 ========== to ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. BUG=webrtc:7080 ==========
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
solenberg@ would you like to take a look? I think I need your approval. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc File webrtc/test/call_test.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:312: fake_send_audio_device_.reset(new FakeAudioDevice(1.0, 48000)); On 2017/01/30 06:45:08, peah-webrtc wrote: > Nit: Should be 1.f instead of 1.0 as the first constructor argument is a float. Done. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/call_test.cc... webrtc/test/call_test.cc:313: fake_recv_audio_device_.reset(new FakeAudioDevice(1.0, 48000)); On 2017/01/30 06:45:08, peah-webrtc wrote: > Nit: Should be 1.f instead of 1.0 as the first constructor argument is a float. Done. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:54: webrtc::Random random_generator_; On 2017/01/30 06:45:08, peah-webrtc wrote: > Since you are in the namespace webrtc, I don't think webrtc:: is needed. Done. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:113: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { On 2017/01/30 06:45:08, peah-webrtc wrote: > Since this is not used anywhere in the class and is private, do you need to > override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I guess > you should just be able to omit this implementation. Or did I miss anything > there? ? audio_callback_ is used. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != nullptr); On 2017/01/30 06:45:08, peah-webrtc wrote: > This DCHECK I don't follow. It basically checks that > callback is not falsy (nullptr) and that audio_callback_ != nullptr, right? (as > || has a higher precedence than != (or did I get that wrong?). > > I guess that what you want to check is RTC_DCHECK(callback || audio_callback_)? > (which is the same, but more consistent) > > Another thing that I don't get with this DCHECK is that it allows > RegisterAudioCallback to be called with callback = nullptr if > audio_callback_==nullptr, but not otherwise. Is that intended? > No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to reset audio_callback_ to nullptr. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:125: int32_t FakeAudioDevice::PlayoutDelay(uint16_t* delay_ms) const { On 2017/01/30 06:45:08, peah-webrtc wrote: > Since PlayoutDelay, and it is not used inside this class, you don't need to > override it and can instead rely on the default implementation in the base class > (or did I miss anything there?) We could move the implementation to the base class if we think its worth it. but to be safe delay_ms should be set to 0 to not affect a/v sync tests. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:148: RTC_CHECK_EQ( On 2017/01/30 06:45:09, peah-webrtc wrote: > Do we really want to crash this code if the return code differs from 0? yes I think so. We can not use gtest. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:157: RTC_CHECK_EQ(0, audio_callback_->NeedMorePlayData( On 2017/01/30 06:45:08, peah-webrtc wrote: > Do we really want to crash this code if the return code differs from 0? dito
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I have some further comments. It is still an lgtm conditioned that I'm wrong about the issue regarding the playout code that cannot be used. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:113: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { On 2017/01/30 12:02:12, perkj_webrtc wrote: > On 2017/01/30 06:45:08, peah-webrtc wrote: > > Since this is not used anywhere in the class and is private, do you need to > > override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I > guess > > you should just be able to omit this implementation. Or did I miss anything > > there? > > ? audio_callback_ is used. I cannot see that but I'm probably missing something obvious here. Afaics: 1) RegisterAudioCallback is private which means that it may only be used locally within this class. Since it is never used within this class, it should be unused code, right? 2) audio_callback_ is initialized to nullptr on line 64 and the only place it can be set is within this method. Since this method is private and not called from within this class, I cannot see that it it can be set at all. Since RegisterAudioCallback is never used, and audio_callback_ is never set to anything but nullptr, it seems like the playout part of this class is still broken. Is that correct? https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != nullptr); On 2017/01/30 12:02:12, perkj_webrtc wrote: > On 2017/01/30 06:45:08, peah-webrtc wrote: > > This DCHECK I don't follow. It basically checks that > > callback is not falsy (nullptr) and that audio_callback_ != nullptr, right? > (as > > || has a higher precedence than != (or did I get that wrong?). > > > > I guess that what you want to check is RTC_DCHECK(callback || > audio_callback_)? > > (which is the same, but more consistent) > > > > Another thing that I don't get with this DCHECK is that it allows > > RegisterAudioCallback to be called with callback = nullptr if > > audio_callback_==nullptr, but not otherwise. Is that intended? > > > > No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to reset > audio_callback_ to nullptr. In my mind that is a too strict restriction as that DCHECK will fail if an attempt is made to reset an audio callback which is already null, which should be a very harmless thing to do. This probably merits an motivation in the code.
https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:113: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { On 2017/01/30 17:06:39, peah-webrtc wrote: > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > Since this is not used anywhere in the class and is private, do you need to > > > override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I > > guess > > > you should just be able to omit this implementation. Or did I miss anything > > > there? > > > > ? audio_callback_ is used. > > I cannot see that but I'm probably missing something obvious here. > Afaics: > 1) RegisterAudioCallback is private which means that it may only be used locally > within this class. Since it is never used within this class, it should be unused > code, right? > 2) audio_callback_ is initialized to nullptr on line 64 and the only place it > can be set is within this method. Since this method is private and not called > from within this class, I cannot see that it it can be set at all. > > Since RegisterAudioCallback is never used, and audio_callback_ is never set to > anything but nullptr, it seems like the playout part of this class is still > broken. Is that correct? audio_callback_ is used both for recording an playout. FakeAudioDevice::RegisterAudioCallback is private, but it is public in the the AudioDevice interface which it implements. I tried to make all methods private that I implement from the AudioDevice interface that is not intended to be used directly by a test that instantiate and start a FakeAudioDevice. However, webrtc VoiceEngine use a pointer to a AudioDevice so for the voice engine it is still public. This distinction is quite common but come up for discussion from time to time. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != nullptr); On 2017/01/30 17:06:39, peah-webrtc wrote: > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > This DCHECK I don't follow. It basically checks that > > > callback is not falsy (nullptr) and that audio_callback_ != nullptr, right? > > (as > > > || has a higher precedence than != (or did I get that wrong?). > > > > > > I guess that what you want to check is RTC_DCHECK(callback || > > audio_callback_)? > > > (which is the same, but more consistent) > > > > > > Another thing that I don't get with this DCHECK is that it allows > > > RegisterAudioCallback to be called with callback = nullptr if > > > audio_callback_==nullptr, but not otherwise. Is that intended? > > > > > > > No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to reset > > audio_callback_ to nullptr. > > In my mind that is a too strict restriction as that DCHECK will fail if an > attempt is made to reset an audio callback which is already null, which should > be a very harmless thing to do. This probably merits an motivation in the code. that is not harmfull but is probably not what the user of an AudioDevice interface should do and thus we can force that by a dcheck.
On 2017/01/31 06:50:35, perkj_webrtc wrote: > https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... > File webrtc/test/fake_audio_device.cc (right): > > https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... > webrtc/test/fake_audio_device.cc:113: int32_t > FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { > On 2017/01/30 17:06:39, peah-webrtc wrote: > > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > > Since this is not used anywhere in the class and is private, do you need > to > > > > override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I > > > guess > > > > you should just be able to omit this implementation. Or did I miss > anything > > > > there? > > > > > > ? audio_callback_ is used. > > > > I cannot see that but I'm probably missing something obvious here. > > Afaics: > > 1) RegisterAudioCallback is private which means that it may only be used > locally > > within this class. Since it is never used within this class, it should be > unused > > code, right? > > 2) audio_callback_ is initialized to nullptr on line 64 and the only place it > > can be set is within this method. Since this method is private and not called > > from within this class, I cannot see that it it can be set at all. > > > > Since RegisterAudioCallback is never used, and audio_callback_ is never set > to > > anything but nullptr, it seems like the playout part of this class is still > > broken. Is that correct? > > audio_callback_ is used both for recording an playout. > FakeAudioDevice::RegisterAudioCallback is private, but it is public in the the > AudioDevice interface which it implements. > I tried to make all methods private that I implement from the AudioDevice > interface that is not intended to be used directly by a test that instantiate > and start a FakeAudioDevice. However, webrtc VoiceEngine use a pointer to a > AudioDevice so for the voice engine it is still public. > This distinction is quite common but come up for discussion from time to time. > > https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... > webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != > nullptr); > On 2017/01/30 17:06:39, peah-webrtc wrote: > > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > > This DCHECK I don't follow. It basically checks that > > > > callback is not falsy (nullptr) and that audio_callback_ != nullptr, > right? > > > (as > > > > || has a higher precedence than != (or did I get that wrong?). > > > > > > > > I guess that what you want to check is RTC_DCHECK(callback || > > > audio_callback_)? > > > > (which is the same, but more consistent) > > > > > > > > Another thing that I don't get with this DCHECK is that it allows > > > > RegisterAudioCallback to be called with callback = nullptr if > > > > audio_callback_==nullptr, but not otherwise. Is that intended? > > > > > > > > > > No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to > reset > > > audio_callback_ to nullptr. > > > > In my mind that is a too strict restriction as that DCHECK will fail if an > > attempt is made to reset an audio callback which is already null, which should > > be a very harmless thing to do. This probably merits an motivation in the > code. > > that is not harmfull but is probably not what the user of an AudioDevice > interface should do and thus we can force that by a dcheck. Great! Thanks! Still lgtm :-)
Much better this way, thanks, only one mid-size comment. https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:31: class FakeAudioDevice::PulsedNoiseCapturer { peah@ - is flipping every really a good test? a configurable period/pulse length seems more appropriate? https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:142: RTC_CHECK_EQ( Don't really see why we're checking the return code here. If you must, make it a DCHECK instead. https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:43: void StartRecordingPulsedNoise(int16_t max_amplitude); Make the amplitude settable in ctor instead and create the PulsedNoiseCapturer there instead (the memory cost is insignificant for the tests), then have a "bool capturing_" which you set in "int32_t StartRecording() override", similar to render side. Then you don't have to keep num_samples_per_frame_ around after setup, IIUC.
The CQ bit was checked by perkj@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
ptal https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:142: RTC_CHECK_EQ( On 2017/01/31 13:08:11, the sun wrote: > Don't really see why we're checking the return code here. If you must, make it a > DCHECK instead. removed https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:43: void StartRecordingPulsedNoise(int16_t max_amplitude); On 2017/01/31 13:08:11, the sun wrote: > Make the amplitude settable in ctor instead and create the PulsedNoiseCapturer > there instead (the memory cost is insignificant for the tests), then have a > "bool capturing_" which you set in "int32_t StartRecording() override", similar > to render side. > > Then you don't have to keep num_samples_per_frame_ around after setup, IIUC. Done. but keept num_samples for convenience for use in playout.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.webrtc.org/2652803002/diff/180001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:265: EXPECT_EQ(0, voe_base->StartSend(send_channel_id)); This line shouldn't be needed. https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:39: RTC_DCHECK_GT(max_amplitude, 0); nit: DHCECK num_samples_per_frame >= 0 https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:70: const float speed_; nit: move up with sample rate and samples per frame constants
https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:113: int32_t FakeAudioDevice::RegisterAudioCallback(AudioTransport* callback) { On 2017/01/31 06:50:35, perkj_webrtc wrote: > On 2017/01/30 17:06:39, peah-webrtc wrote: > > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > > Since this is not used anywhere in the class and is private, do you need > to > > > > override it? Afaics, it is not pure virtual in FakeAudioDeviceModule so I > > > guess > > > > you should just be able to omit this implementation. Or did I miss > anything > > > > there? > > > > > > ? audio_callback_ is used. > > > > I cannot see that but I'm probably missing something obvious here. > > Afaics: > > 1) RegisterAudioCallback is private which means that it may only be used > locally > > within this class. Since it is never used within this class, it should be > unused > > code, right? > > 2) audio_callback_ is initialized to nullptr on line 64 and the only place it > > can be set is within this method. Since this method is private and not called > > from within this class, I cannot see that it it can be set at all. > > > > Since RegisterAudioCallback is never used, and audio_callback_ is never set > to > > anything but nullptr, it seems like the playout part of this class is still > > broken. Is that correct? > > audio_callback_ is used both for recording an playout. > FakeAudioDevice::RegisterAudioCallback is private, but it is public in the the > AudioDevice interface which it implements. > I tried to make all methods private that I implement from the AudioDevice > interface that is not intended to be used directly by a test that instantiate > and start a FakeAudioDevice. However, webrtc VoiceEngine use a pointer to a > AudioDevice so for the voice engine it is still public. > This distinction is quite common but come up for discussion from time to time. True, of course, my mistake. Thanks for the explanation! Acknowledged. https://codereview.webrtc.org/2652803002/diff/140001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:115: RTC_DCHECK(callback || audio_callback_ != nullptr); On 2017/01/31 06:50:35, perkj_webrtc wrote: > On 2017/01/30 17:06:39, peah-webrtc wrote: > > On 2017/01/30 12:02:12, perkj_webrtc wrote: > > > On 2017/01/30 06:45:08, peah-webrtc wrote: > > > > This DCHECK I don't follow. It basically checks that > > > > callback is not falsy (nullptr) and that audio_callback_ != nullptr, > right? > > > (as > > > > || has a higher precedence than != (or did I get that wrong?). > > > > > > > > I guess that what you want to check is RTC_DCHECK(callback || > > > audio_callback_)? > > > > (which is the same, but more consistent) > > > > > > > > Another thing that I don't get with this DCHECK is that it allows > > > > RegisterAudioCallback to be called with callback = nullptr if > > > > audio_callback_==nullptr, but not otherwise. Is that intended? > > > > > > > > > > No, it allows callback == nullptr if audio_callback_ != nullptr. I.e- to > reset > > > audio_callback_ to nullptr. > > > > In my mind that is a too strict restriction as that DCHECK will fail if an > > attempt is made to reset an audio callback which is already null, which should > > be a very harmless thing to do. This probably merits an motivation in the > code. > > that is not harmfull but is probably not what the user of an AudioDevice > interface should do and thus we can force that by a dcheck. Acknowledged. https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:31: class FakeAudioDevice::PulsedNoiseCapturer { On 2017/01/31 13:08:10, the sun wrote: > peah@ - is flipping every really a good test? a configurable period/pulse length > seems more appropriate? That definitely depends on what you want to test. I was told that the main requirement was that something audible should end up in the other end. For that I think this should be fine, but I'd probably have set the period to 100ms. Depending on the scope of usage for this class, I'd initially settle for a fixed period if that works. I don't think we need to generalize if there is no need. https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:39: RTC_DCHECK_GT(max_amplitude, 0); On 2017/01/31 16:27:08, the sun wrote: > nit: DHCECK num_samples_per_frame >= 0 Or specify it as a size_t.
https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/160001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:31: class FakeAudioDevice::PulsedNoiseCapturer { On 2017/01/31 16:37:33, peah-webrtc wrote: > On 2017/01/31 13:08:10, the sun wrote: > > peah@ - is flipping every really a good test? a configurable period/pulse > length > > seems more appropriate? > > That definitely depends on what you want to test. I was told that the main > requirement was that something audible should end up in the other end. For that > I think this should be fine, but I'd probably have set the period to 100ms. > > Depending on the scope of usage for this class, I'd initially settle for a fixed > period if that works. I don't think we need to generalize if there is no need. I am going to ignore this discussion and leave this to you or change if the need arise as briefly discussed with solenberg. https://codereview.webrtc.org/2652803002/diff/180001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:265: EXPECT_EQ(0, voe_base->StartSend(send_channel_id)); On 2017/01/31 16:27:07, the sun wrote: > This line shouldn't be needed. Done. https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:39: RTC_DCHECK_GT(max_amplitude, 0); On 2017/01/31 16:37:33, peah-webrtc wrote: > On 2017/01/31 16:27:08, the sun wrote: > > nit: DHCECK num_samples_per_frame >= 0 > > Or specify it as a size_t. size_t since that is what the it is being used as. Despite our previous discussion about style guide... https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2652803002/diff/180001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:70: const float speed_; On 2017/01/31 16:27:08, the sun wrote: > nit: move up with sample rate and samples per frame constants Done.
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2652803002/#ps200001 (title: "Addressed solenbergs comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12876)
Description was changed from ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. BUG=webrtc:7080 ========== to ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. TBR=henrika@webrtc.org, stefan@webrtc.org BUG=webrtc:7080 ==========
TBR: henrika@webrtc.org as owner of audio_device_module stefan@webrtc.org as owner of webrtc/test
perkj@webrtc.org changed reviewers: + henrika@webrtc.org, stefan@webrtc.org
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2652803002/#ps220001 (title: "fix size_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1485894832892460,
"parent_rev": "1c056254a6b8b6521ad2799c8c705b8bbfb9c210", "commit_rev":
"ac61b745df8eb918e8a39368fec7d7c3a890f221"}
Message was sent while issue was closed.
Description was changed from ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. TBR=henrika@webrtc.org, stefan@webrtc.org BUG=webrtc:7080 ========== to ========== Refactor FakeAudioDevice to have separate methods for starting recording and playout. Also, change FakeAudioDevice to generate a sine tone instead of using a file. TBR=henrika@webrtc.org, stefan@webrtc.org BUG=webrtc:7080 Review-Url: https://codereview.webrtc.org/2652803002 Cr-Commit-Position: refs/heads/master@{#16385} Committed: https://chromium.googlesource.com/external/webrtc/+/ac61b745df8eb918e8a39368f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/ac61b745df8eb918e8a39368f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
