Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717623003:
Add the ability to read/write to WAV files in FakeAudioDevice (Closed)
|
Created:
3 years, 10 months ago by oprypin_webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, kjellander_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd the ability to read/write to WAV files in FakeAudioDevice
BUG=webrtc:7229
Review-Url: https://codereview.webrtc.org/2717623003
Cr-Commit-Position: refs/heads/master@{#17230}
Committed: https://chromium.googlesource.com/external/webrtc/+/a514584b9a594447b78ed15bba7504b5121d2fcd
Patch Set 1 #
Total comments: 38
Patch Set 2 : Address smaller codereview issues #Patch Set 3 : Rebase #Patch Set 4 : Split into FileReaderAudioDevice and FileWriterAudioDevice #Patch Set 5 : Revert changing to a lambda #Patch Set 6 : Don't run the thread all the time; add more checks #
Total comments: 15
Patch Set 7 : Implement file reading/writing in FakeAudioDevice instead #Patch Set 8 : Add the ability to wait for a FakeAudioDevice to finish #Patch Set 9 : Remove forgotten debug line #Patch Set 10 : Remove obsolete files from build #
Total comments: 26
Patch Set 11 : Address review feedback #Patch Set 12 : Drop the old FakeAudioDevice constructor #Patch Set 13 : Prevent nested CritScope to fix 'use of an invalid mutex' #
Total comments: 26
Patch Set 14 : Address review feedback #
Total comments: 17
Patch Set 15 : Address review feedback #Patch Set 16 : Avoid repetition when checking sample rate #
Messages
Total messages: 64 (37 generated)
The CQ bit was checked by oprypin@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/...
Description was changed from ========== Add fake FileAudioDevice to allow testing with audio from files BUG=webrtc:7229 ========== to ========== Add fake FileAudioDevice to allow testing with audio from files BUG=webrtc:7229 ==========
oprypin@webrtc.org changed reviewers: + kwiberg@webrtc.org, perkj@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
perkj@webrtc.org changed reviewers: + solenberg@webrtc.org-
-perkj@ +solenberg@
perkj@webrtc.org changed reviewers: + solenberg@webrtc.org - perkj@webrtc.org, solenberg@webrtc.org-
Nicely written! Would it be possible (and reasonable...) to split this into separate classes for reading and writing? Ideally that would lead to each one having less state. If so, prefer to put share code and state in a member object rather than a base class. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device.cc File webrtc/test/file_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:30: : filename_(filename), Since you always copy the filename, take it by value instead of const reference. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:42: RTC_DCHECK( Since this is test code, there's no reason not to use CHECK instead of DCHECK. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:51: thread_.Stop(); Why not stop the thread first? (Just asking---I don't know the answer. But stopping threads before cleaning up stuff they might be using usually makes sense.) https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:64: wav_writer_.reset(nullptr); It's better to do either wav_writer_.reset(); or wav_writer_ = nullptr; since they're shorter. The former is probably better on account of telling the reader that wav_writer_ is a smart and not a raw pointer. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:79: wav_reader_.reset(nullptr); // This also finalizes and closes the file Maybe nothing to fix, but you don't have this comment in other places that reset wav_reader_ and wav_writer_. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:86: thread_.SetPriority(rtc::kHighPriority); Do these two in the other order? (Again, just the general observation that you shouldn't have threads running unless you must.) https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:115: } This doesn't need to be a member function. It can be a lambda. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:122: size_t samples_out = wav_reader_->ReadSamples(num_samples_per_frame_, const https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:123: playout_buffer_.data()); CHECK that samples_out <= playout_buffer_.size()? https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:125: uint32_t new_mic_level = 0; Don't initialize if you don't have to. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:136: int64_t ntp_time_ms = -1; Don't initialize here either. In particular, since you actually use samples_out below, if you don't initialize it here you give MemorySanitizer the chance to detect if NeedMorePlayData doesn't fill it in. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:141: } Since playout_buffer_ is only used locally in this function, consider allocating a local buffer instead (because the more state your class has, the harder it is to understand). Just allocating an int16_t[480] on the stack should be both readable and fast. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device.h File webrtc/test/file_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:33: class FileAudioDevice : public FakeAudioDeviceModule { final? Or do you anticipate subclasses? https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:36: // frames will be processed every 100ms / |speed|. Umm... so a speed of 1 gives one frame every 100ms, 0.1 -> every 1000ms, 10 -> every 10ms. That is, the unit of |speed| is "(10ms audio frame processed) per 100ms". Wouldn't it make more sense to use "(10ms audio frame processed) per 10ms", so that speed 1 would be real time, 0.1 would be 0.1x real time, and 10 would be 10x real time? Alternatively---considering that it's easy to make float math give different results depending on target machine, compiler version, optimization flags, and phase of the moon---what about making the speed some sort of integer? https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:37: // |sampling_frequency_in_hz| can be 8, 16, 32, 44.1 or 48kHz. For consistency with other audio code (and for brevity), call this "sample_rate_hz". https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:42: // Block until the input audio file ends. Document the argument? And that it only works if you're reading? https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:72: std::vector<int16_t> playout_buffer_ GUARDED_BY(lock_); Good use of const and GUARDED_BY! https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:76: rtc::Event done_reading_; Document what this one does?
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by oprypin@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/...
https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device.cc File webrtc/test/file_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:30: : filename_(filename), On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Since you always copy the filename, take it by value instead of const reference. Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:42: RTC_DCHECK( On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Since this is test code, there's no reason not to use CHECK instead of DCHECK. Just did the same as in FakeAudioDevice. Changed to CHECK now. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:51: thread_.Stop(); On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Why not stop the thread first? (Just asking---I don't know the answer. But > stopping threads before cleaning up stuff they might be using usually makes > sense.) Just did the same as in FakeAudioDevice. The order does not matter for thread safety, because all of this is done in a lock. But maybe it's better to let the file be finalized immediately and then wait for the thread to end? Leaving as is. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:64: wav_writer_.reset(nullptr); On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > It's better to do either > > wav_writer_.reset(); > > or > > wav_writer_ = nullptr; > > since they're shorter. The former is probably better on account of telling the > reader that wav_writer_ is a smart and not a raw pointer. For some reason I thought `reset()` is a newer feature than `reset(ptr)` and the rest of nullptr, but it isn't. Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:79: wav_reader_.reset(nullptr); // This also finalizes and closes the file On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Maybe nothing to fix, but you don't have this comment in other places that reset > wav_reader_ and wav_writer_. Hm, this is supposed to be a comment for WavWriter, because WavReader doesn't actually finalize anything. I will still keep it in only one place because it's the most important place where a user of this class might look when they are not sure whether the file is actually finalized. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:86: thread_.SetPriority(rtc::kHighPriority); On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Do these two in the other order? (Again, just the general observation that you > shouldn't have threads running unless you must.) A code search shows that it's always done in this order (start thread, then set priority). In fact, I think reversing the order causes an error. Leaving as is. I think I see your point about running threads. When I separate this into two classes, it will make even less sense to have this thread permanently running, so I guess I will start and stop it on demand. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:115: } On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > This doesn't need to be a member function. It can be a lambda. I've tried turning it into a lambda, but getting this: Check failed: spawned_thread_checker_.CalledOnValidThread() https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:122: size_t samples_out = wav_reader_->ReadSamples(num_samples_per_frame_, On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > const Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:123: playout_buffer_.data()); On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > CHECK that samples_out <= playout_buffer_.size()? Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:125: uint32_t new_mic_level = 0; On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Don't initialize if you don't have to. Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:136: int64_t ntp_time_ms = -1; On 2017/02/28 13:43:46, kwiberg-webrtc wrote: > Don't initialize here either. In particular, since you actually use samples_out > below, if you don't initialize it here you give MemorySanitizer the chance to > detect if NeedMorePlayData doesn't fill it in. Thanks, good to know. Again I just took FakeAudioDevice as an example. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.cc:141: } On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Since playout_buffer_ is only used locally in this function, consider allocating > a local buffer instead (because the more state your class has, the harder it is > to understand). Just allocating an int16_t[480] on the stack should be both > readable and fast. I don't see how I can use an array of a constant size here, because `num_samples_per_frame_` depends on some runtime numbers. I also don't want to hardcode some maximal precalculated number. And it's probably inefficient to use a vector here. Keeping as is, waiting for comment. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device.h File webrtc/test/file_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:33: class FileAudioDevice : public FakeAudioDeviceModule { On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > final? Or do you anticipate subclasses? It's hard to predict what people might want to do. In test code I don't see a good reason to limit creativity. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:36: // frames will be processed every 100ms / |speed|. On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Umm... so a speed of 1 gives one frame every 100ms, 0.1 -> every 1000ms, 10 -> > every 10ms. That is, the unit of |speed| is "(10ms audio frame processed) per > 100ms". Wouldn't it make more sense to use "(10ms audio frame processed) per > 10ms", so that speed 1 would be real time, 0.1 would be 0.1x real time, and 10 > would be 10x real time? > > Alternatively---considering that it's easy to make float math give different > results depending on target machine, compiler version, optimization flags, and > phase of the moon---what about making the speed some sort of integer? Again this is just something I copied from FakeAudioDevice. Right now it's kind of a drop-in replacement; I wouldn't make this change without changing the other one. I don't have enough understanding to judge the correctness of this decision. Leaving as is, unless another reply is made. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:37: // |sampling_frequency_in_hz| can be 8, 16, 32, 44.1 or 48kHz. On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > For consistency with other audio code (and for brevity), call this > "sample_rate_hz". Done. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:42: // Block until the input audio file ends. On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Document the argument? And that it only works if you're reading? Named it `timeout_ms`. And this should stay in only one of the 2 classes after the split. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:72: std::vector<int16_t> playout_buffer_ GUARDED_BY(lock_); On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Good use of const and GUARDED_BY! Not sure what you mean. https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device... webrtc/test/file_audio_device.h:76: rtc::Event done_reading_; On 2017/02/28 13:43:47, kwiberg-webrtc wrote: > Document what this one does? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Good news: Except for what I complain about in the comments, this looks just
like I wanted.
Bad news: solenberg@ had another idea about how to organize this, and I agree
that it's better than mine. :-/
Take a look at FakeAudioDevice in webrtc/test/fake_audio_device.{h,cc}. It
currently captures data from a FakeAudioDevice::PulsedNoiseCapturer object, and
throws away data to be rendered. It would not be hard to invent a
FakeAudioDevice::Capturer interface with two implementations,
PulsedNoiseCapturer and WavFileReader, and a FakeAudioDevice::Renderer interface
with two implementations, Discarder and WavFileWriter. The FakeAudioDevice
constructor would then take one unique_ptr<FakeAudioDevice::Capturer> and one
unique_ptr<FakeAudioDevice::Renderer> argument. The advantage would be that we'd
end up with one instead of three copies of the logic that handles multithreading
and all the other stuff that comes with being a FakeAudioDeviceModule subclass.
(In case it's not obvious, by "interface" I mean a class with a virtual
destructor, only virtual =0 methods, and no state. These two interfaces ought to
be very small, too, with probably just one method each.)
(The four interface implementations can live entirely in an anonymous namespace
in webrtc/test/fake_audio_device.cc. The outside world can access them via
static Create functions in FakeAudioDevice.)
Sorry for changing my mind after leading you along another path for so long, but
solenberg@'s idea really does sound much better...
https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device.cc
File webrtc/test/file_audio_device.cc (right):
https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device...
webrtc/test/file_audio_device.cc:115: }
On 2017/03/06 16:45:00, oprypin_webrtc wrote:
> On 2017/02/28 13:43:46, kwiberg-webrtc wrote:
> > This doesn't need to be a member function. It can be a lambda.
>
> I've tried turning it into a lambda, but getting this:
>
> Check failed: spawned_thread_checker_.CalledOnValidThread()
On what line? That makes no sense at all to me. A static member function is just
a function, and a captureless lambda is just a function.
I noted that your lambda returned void instead of bool, though. Would that make
a difference?
https://codereview.webrtc.org/2717623003/diff/1/webrtc/test/file_audio_device...
webrtc/test/file_audio_device.cc:141: }
On 2017/03/06 16:45:00, oprypin_webrtc wrote:
> On 2017/02/28 13:43:47, kwiberg-webrtc wrote:
> > Since playout_buffer_ is only used locally in this function, consider
> allocating
> > a local buffer instead (because the more state your class has, the harder it
> is
> > to understand). Just allocating an int16_t[480] on the stack should be both
> > readable and fast.
>
> I don't see how I can use an array of a constant size here, because
> `num_samples_per_frame_` depends on some runtime numbers.
> I also don't want to
> hardcode some maximal precalculated number.
Just the sample rate, which is capped at 48 kHz. IIUC, 480 entries is as big as
you'll ever need.
> And it's probably inefficient to use a vector here.
> Keeping as is, waiting for comment.
Allocating a new std::vector each time would be bad for performance, yes. But
allocating an array of ~1 kB on the stack is *very* fast---it costs one
instruction to allocate and one to deallocate. And if you're already allocating
things on the stack, it's free.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
File webrtc/test/file_audio_device.cc (right):
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.cc:38: wav_reader_(nullptr),
unique_ptrs are null by default, so you don't need this line.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.cc:58: RTC_CHECK(wav_reader_->num_channels() ==
1);
RTC_CHECK_EQ for these two.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.cc:76: RTC_DCHECK(callback || audio_callback_ !=
nullptr);
Test code, so CHECK instead of DCHECK everywhere?
Also, for consistency, either
callback || audio_callback_
or
callback != nullptr || audio_callback_ != nullptr
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.cc:102: if (samples_out) {
if (samples_out > 0)
to emphasize that samples_out is a number, not some sort of boolean.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.cc:103: RTC_CHECK(samples_out <=
playout_buffer_.size());
RTC_CHECK_LE
Also, move this check before line 102? It seems to apply to both if branches.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
File webrtc/test/file_audio_device.h (right):
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.h:34: // Creates a new FileReaderAudioDevice. When
capturing, 10 ms audio frames
"When capturing, one 10 ms audio frame will [...]"
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.h:35: // will be processed every 100ms / |speed|.
I'll just note for the record that I find the unit of |speed| very strange, but
keep it like this if you must.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.h:51: bool Recording() const override;
Are these four public in FakeAudioDeviceModule? If so, they should probably be
so here too.
https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d...
webrtc/test/file_audio_device.h:66: std::unique_ptr<EventTimerWrapper> tick_;
Can this be a const std::unique_ptr<EventTimerWrapper>? You only set it once, in
the constructor.
On 2017/03/07 10:11:58, kwiberg-webrtc wrote:
> Bad news: solenberg@ had another idea about how to organize this, and I agree
> that it's better than mine. :-/
>
> Take a look at FakeAudioDevice in webrtc/test/fake_audio_device.{h,cc}. It
> currently captures data from a FakeAudioDevice::PulsedNoiseCapturer object,
and
> throws away data to be rendered. It would not be hard to invent a
> FakeAudioDevice::Capturer interface with two implementations,
> PulsedNoiseCapturer and WavFileReader, and a FakeAudioDevice::Renderer
interface
> with two implementations, Discarder and WavFileWriter. The FakeAudioDevice
> constructor would then take one unique_ptr<FakeAudioDevice::Capturer> and one
> unique_ptr<FakeAudioDevice::Renderer> argument. The advantage would be that
we'd
> end up with one instead of three copies of the logic that handles
multithreading
> and all the other stuff that comes with being a FakeAudioDeviceModule
subclass.
>
> (In case it's not obvious, by "interface" I mean a class with a virtual
> destructor, only virtual =0 methods, and no state. These two interfaces ought
to
> be very small, too, with probably just one method each.)
>
> (The four interface implementations can live entirely in an anonymous
namespace
> in webrtc/test/fake_audio_device.cc. The outside world can access them via
> static Create functions in FakeAudioDevice.)
>
> Sorry for changing my mind after leading you along another path for so long,
but
> solenberg@'s idea really does sound much better...
Indeed, this makes a lot of sense.
Let's clarify some things:
When you talk about "static Create functions in FakeAudioDevice", do you mean
functions to create Capturer/Renderer instances? Please clarify how you imagine
that these functions would be named.
It is also not entirely clear to me why these classes should not be used and
constructed directly.
What will happen to the existing constructor: `FakeAudioDevice(float speed, int
sampling_frequency_in_hz, int16_t max_amplitude)`? I assume it should be fine to
drop it and just change its users to construct it with a PulsedNoiseCapturer and
pass the arguments there.
I also have some concerns about further extensibility of this approach, if the
classes are kept private. For example, I think I will need a specialized
WavFileWriter that ignores silence (zeroes) at the beginning of the recording.
And, of course, there could be some problems with keeping the state of the
actual files. Under this very simple interface the Renderer will never know when
it needs to close the file, and I also don't see any good way of resetting the
device to use a different file or something like that, which is possible under
my old approach. There might be other such complications that I'm missing.
On 2017/03/07 12:36:32, oprypin_webrtc wrote:
> On 2017/03/07 10:11:58, kwiberg-webrtc wrote:
> > Bad news: solenberg@ had another idea about how to organize this, and I
agree
> > that it's better than mine. :-/
> >
> > Take a look at FakeAudioDevice in webrtc/test/fake_audio_device.{h,cc}. It
> > currently captures data from a FakeAudioDevice::PulsedNoiseCapturer object,
> and
> > throws away data to be rendered. It would not be hard to invent a
> > FakeAudioDevice::Capturer interface with two implementations,
> > PulsedNoiseCapturer and WavFileReader, and a FakeAudioDevice::Renderer
> interface
> > with two implementations, Discarder and WavFileWriter. The FakeAudioDevice
> > constructor would then take one unique_ptr<FakeAudioDevice::Capturer> and
one
> > unique_ptr<FakeAudioDevice::Renderer> argument. The advantage would be that
> we'd
> > end up with one instead of three copies of the logic that handles
> multithreading
> > and all the other stuff that comes with being a FakeAudioDeviceModule
> subclass.
> >
> > (In case it's not obvious, by "interface" I mean a class with a virtual
> > destructor, only virtual =0 methods, and no state. These two interfaces
ought
> to
> > be very small, too, with probably just one method each.)
> >
> > (The four interface implementations can live entirely in an anonymous
> namespace
> > in webrtc/test/fake_audio_device.cc. The outside world can access them via
> > static Create functions in FakeAudioDevice.)
> >
> > Sorry for changing my mind after leading you along another path for so long,
> but
> > solenberg@'s idea really does sound much better...
>
> Indeed, this makes a lot of sense.
>
> Let's clarify some things:
>
> When you talk about "static Create functions in FakeAudioDevice", do you mean
> functions to create Capturer/Renderer instances? Please clarify how you
imagine
> that these functions would be named.
Yes, exactly. For example:
// Returns a Capturer instance that gets its data from a file.
static std::unique_ptr<Capturer> CreateWavFileCapturer(std::string filename);
// Returns a Capturer instance that generates a signal where every second
// frame is zero and every second frame is evenly distributed random noise
// with max amplitude |max_amplitude|.
static std::unique_ptr<Capturer> CreatePulsedNoiseCapturer(int16_t
max_amplitude);
The actual Capturer subclasses they would create would exist only in the .cc
file.
> It is also not entirely clear to me why these classes should not be used and
> constructed directly.
I don't quite understand what you're asking here.
> What will happen to the existing constructor: `FakeAudioDevice(float speed,
int
> sampling_frequency_in_hz, int16_t max_amplitude)`? I assume it should be fine
to
> drop it and just change its users to construct it with a PulsedNoiseCapturer
and
> pass the arguments there.
Yes, I imagine it would become something like
FakeAudioDevice(float speed, int sample_rate_hz,
std::unique_ptr<Capturer> capturer,
std::unique_ptr<Renderer> renderer);
And yes, it should be OK to just eliminate the old version and update all
callers.
> I also have some concerns about further extensibility of this approach, if the
> classes are kept private. For example, I think I will need a specialized
> WavFileWriter that ignores silence (zeroes) at the beginning of the recording.
If one test needs that sort of thing, it can define its own Renderer that eats
zeroes in the beginning and forwards the remainder to a Renderer that you get by
calling FakeAudioDevice::CreateWavFileRenderer(). If a large fraction of tests
need that functionality, maybe extend the class returned by
FakeAudioDevice::CreateWavFileRenderer() to have this functionality.
> And, of course, there could be some problems with keeping the state of the
> actual files. Under this very simple interface the Renderer will never know
when
> it needs to close the file, and I also don't see any good way of resetting the
> device to use a different file or something like that, which is possible under
> my old approach. There might be other such complications that I'm missing.
Can they simply close their files when being destroyed?
One possibility is to give FakeAudioDevice SetCapturer and SetRenderer methods
that replace the exiting Capturer and Renderer. The old ones would be destroyed,
which would make them close their files.
On 2017/03/07 14:33:16, kwiberg-webrtc wrote: > On 2017/03/07 12:36:32, oprypin_webrtc wrote: > > It is also not entirely clear to me why these classes should not be used and > > constructed directly. > > I don't quite understand what you're asking here. I do not see why these implementation classes are kept private and why functions like `FakeAudioDevice::CreateWavFileRenderer()` should be used, instead of using the classes' constructors directly. The classes would be exposed by the functions anyway, and all of this should be namespaced in webrtc::test::, so why not make all of this public? > If one test needs that sort of thing, it can define its own Renderer that eats > zeroes in the beginning and forwards the remainder to a Renderer that you get by > calling FakeAudioDevice::CreateWavFileRenderer() That is an option, but I thought maybe it would be simpler to subclass it, hence my remark above.
On 2017/03/07 15:03:57, oprypin_webrtc wrote: > On 2017/03/07 14:33:16, kwiberg-webrtc wrote: > > On 2017/03/07 12:36:32, oprypin_webrtc wrote: > > > It is also not entirely clear to me why these classes should not be used and > > > constructed directly. > > > > I don't quite understand what you're asking here. > I do not see why these implementation classes are kept private and why functions > like `FakeAudioDevice::CreateWavFileRenderer()` should be used, instead of using > the classes' constructors directly. The classes would be exposed by the > functions anyway, and all of this should be namespaced in webrtc::test::, so why > not make all of this public? You don't *have* to do it this way, but the *point* of doing it like this is that the fewer implementation details different parts of the code expose to the rest of the code, the easier things are to understand. Usually. In this case, all everyone else needs to know is that the return value of CreateWavFileRenderer() is a subclass of FakeAudioDevice::Renderer. No one needs to know or care what private methods, private data members etc. this class has. Of course, if you have a reason to expose the class, by all means do so. > > If one test needs that sort of thing, it can define its own Renderer that eats > > zeroes in the beginning and forwards the remainder to a Renderer that you get > by > > calling FakeAudioDevice::CreateWavFileRenderer() > > That is an option, but I thought maybe it would be simpler to subclass it, hence > my remark above. You could do that, but please think twice before you subclass a class that isn't a pure interface. Sometimes that's the best way to do a thing, but it's also really easy to make a difficult-to-read mess that way... Consider FakeAudioDevice for example. We *could* have given it two virtual methods---ProduceData() and ConsumeData(), say---and allowed tests to define their own subclasses overriding those methods. But using separate objects for that instead, like solenberg@ suggested, is much less messy. (This is the "prefer composition to inheritance" principle: see e.g. https://en.wikipedia.org/wiki/Composition_over_inheritance)
The CQ bit was checked by oprypin@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: Try jobs failed on following builders: linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Hi, the CL has been reworked as suggested, requesting review. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... File webrtc/test/file_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.cc:58: RTC_CHECK(wav_reader_->num_channels() == 1); On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > RTC_CHECK_EQ for these two. Done. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.cc:76: RTC_DCHECK(callback || audio_callback_ != nullptr); On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > Test code, so CHECK instead of DCHECK everywhere? > > Also, for consistency, either > > callback || audio_callback_ > > or > > callback != nullptr || audio_callback_ != nullptr Done. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.cc:102: if (samples_out) { On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > if (samples_out > 0) > > to emphasize that samples_out is a number, not some sort of boolean. Done. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.cc:103: RTC_CHECK(samples_out <= playout_buffer_.size()); On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > RTC_CHECK_LE > > Also, move this check before line 102? It seems to apply to both if branches. Done. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... File webrtc/test/file_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.h:35: // will be processed every 100ms / |speed|. On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > I'll just note for the record that I find the unit of |speed| very strange, but > keep it like this if you must. Acknowledged. https://codereview.webrtc.org/2717623003/diff/120001/webrtc/test/file_audio_d... webrtc/test/file_audio_device.h:51: bool Recording() const override; On 2017/03/07 10:11:57, kwiberg-webrtc wrote: > Are these four public in FakeAudioDeviceModule? If so, they should probably be > so here too. Done.
The CQ bit was checked by oprypin@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.
Very nice! I have a bunch of nits, but it's very clear that this way of doing things is superior to what I suggested originally. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:28: } // namespace The Capturers and Renderers should be in an anonymous namespace, because they can. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:72: class FakeAudioDevice::WavFileReader : public FakeAudioDevice::Capturer { This class (and the other three) can be final. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:92: // This event is set when the input audio file ends Orphaned comment. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:101: bool Render(rtc::ArrayView<const int16_t> data) { override? https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:273: bool keep_rendering = renderer_->Render( const? https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:53: class WavFileWriter; Why do you need to mention these four in the .h file? https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:61: virtual rtc::ArrayView<const int16_t> Capture() = 0; It's not usually a good idea to return an ArrayView, because then the caller has to know somehow for how long the pointed-to data is going to remain valid. There are several alternatives. I would suggest passing an rtc::Buffer*, and letting Capture() replace its contents. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:62: virtual ~Capturer() {} Put the destructor before the regular methods (just after the constructors, but you don't have any). https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:72: }; Unless you have a very good reason, please make Capturer and Renderer pure interfaces. Non-pure interfaces are almost always more trouble than they are worth. I think a good arrangement here would be to turn Streamer into a pure interface too, and have Capturer and Renderer inherit from Streamer. Then in the .cc file, have a StreamerImpl class that has the shared methods and state that everyone needs. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:82: float speed = 1); The default speed is 0.1x realtime? https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:92: int sampling_frequency_in_hz, int16_t max_amplitude); The sample rate is the last argument in all the other Create functions. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:123: bool WaitForRecordingEnd(int timeout_ms = rtc::Event::kForever); What do these two return?
Description was changed from ========== Add fake FileAudioDevice to allow testing with audio from files BUG=webrtc:7229 ========== to ========== Add the ability to read/write to WAV files in FakeAudioDevice BUG=webrtc:7229 ==========
The CQ bit was checked by oprypin@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/...
https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:28: } // namespace On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > The Capturers and Renderers should be in an anonymous namespace, because they > can. Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:72: class FakeAudioDevice::WavFileReader : public FakeAudioDevice::Capturer { On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > This class (and the other three) can be final. Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:92: // This event is set when the input audio file ends On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > Orphaned comment. Done. Removed. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:101: bool Render(rtc::ArrayView<const int16_t> data) { On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > override? Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:273: bool keep_rendering = renderer_->Render( On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > const? Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:53: class WavFileWriter; On 2017/03/09 10:04:11, kwiberg-webrtc wrote: > Why do you need to mention these four in the .h file? Done. Together with the change to anonymous namespaces, these mentions are now obsolete anyway. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:61: virtual rtc::ArrayView<const int16_t> Capture() = 0; On 2017/03/09 10:04:11, kwiberg-webrtc wrote: > It's not usually a good idea to return an ArrayView, because then the caller has > to know somehow for how long the pointed-to data is going to remain valid. > > There are several alternatives. I would suggest passing an rtc::Buffer*, and > letting Capture() replace its contents. Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:62: virtual ~Capturer() {} On 2017/03/09 10:04:11, kwiberg-webrtc wrote: > Put the destructor before the regular methods (just after the constructors, but > you don't have any). Done. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:72: }; On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > Unless you have a very good reason, please make Capturer and Renderer pure > interfaces. Non-pure interfaces are almost always more trouble than they are > worth. > > I think a good arrangement here would be to turn Streamer into a pure interface > too, and have Capturer and Renderer inherit from Streamer. Then in the .cc file, > have a StreamerImpl class that has the shared methods and state that everyone > needs. I just want to get rid of this num_samples_per_frame_ computation (facilitating the storage for sampling frequency is also nice) and this is the best way I found to have it in only one place. I agree that it's messy but I can't come up with any better way to do it. I don't understand the comment about StreamerImpl, but seems like this introduces some complexity and some code duplication in order to eliminate code duplication. I'd like to discuss this more, kept as is for now. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:82: float speed = 1); On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > The default speed is 0.1x realtime? I'm quite sure this is just realtime, because call_test.cc specifies 1 when creating this object. My CL doesn't touch the logic of what the speed means, and this line... StartTimer(true, kFrameLengthMs / speed_) ... (`kFrameLengthMs` being 10) seems to suggest that "frames will be processed every 10ms / |speed|" which would make more sense then. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:92: int sampling_frequency_in_hz, int16_t max_amplitude); On 2017/03/09 10:04:11, kwiberg-webrtc wrote: > The sample rate is the last argument in all the other Create functions. Done. Made it the last here as well. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:123: bool WaitForRecordingEnd(int timeout_ms = rtc::Event::kForever); On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > What do these two return? Done. Added comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20668)
Thanks for hanging in there. It feels like we're almost done... https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:72: }; On 2017/03/10 10:44:27, oprypin_webrtc wrote: > On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > > Unless you have a very good reason, please make Capturer and Renderer pure > > interfaces. Non-pure interfaces are almost always more trouble than they are > > worth. > > > > I think a good arrangement here would be to turn Streamer into a pure > interface > > too, and have Capturer and Renderer inherit from Streamer. Then in the .cc > file, > > have a StreamerImpl class that has the shared methods and state that everyone > > needs. > > I just want to get rid of this num_samples_per_frame_ computation (facilitating > the storage for sampling frequency is also nice) and this is the best way I > found to have it in only one place. I agree that it's messy but I can't come up > with any better way to do it. > > I don't understand the comment about StreamerImpl, but seems like this > introduces some complexity and some code duplication in order to eliminate code > duplication. > > I'd like to discuss this more, kept as is for now. I have two alternative suggestions. The first one is to not have Streamer in the .h file at all, but just define purely abstract classes Capturer and Renderer; all the shared code would then be in the implementations in the .cc file. The second alternative is to have a purely abstract Streamer in the header, plus purely abstract classes Capturer and Renderer that inherit from it. Then in the .cc file, make an implementation of Streamer, and have the implementations of Capturer and Renderer inherit from it. The second alternative minimizes code duplication, but the first minimizes complexity in the header. My opinion is that minimizing complexity in the header is more important, but feel free to argue if you feel otherwise. https://codereview.webrtc.org/2717623003/diff/200001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:82: float speed = 1); On 2017/03/10 10:44:27, oprypin_webrtc wrote: > On 2017/03/09 10:04:10, kwiberg-webrtc wrote: > > The default speed is 0.1x realtime? > > I'm quite sure this is just realtime, because call_test.cc specifies 1 when > creating this object. My CL doesn't touch the logic of what the speed means, and > this line... > StartTimer(true, kFrameLengthMs / speed_) > ... (`kFrameLengthMs` being 10) seems to suggest that "frames will be processed > every 10ms / |speed|" which would make more sense then. OK, then either the existing comment is wrong, or I'm very confused---a speed of 1 ought to mean we process one 10 ms frame every 10 ms, no? If this is just the speed relative to realtime, say something like "|speed| is the number of 10 ms frames we process every 10 ms; a value >1 is faster than real time, and <1 is slower." https://codereview.webrtc.org/2717623003/diff/260001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:151: FakeAudioDevice::CreateDiscarder(48000), audio_rtp_speed); This looks very nice! Hmm. You don't have to, but call sites like this one might get even easier to read if CreateDiscarder was called CreateDiscardRenderer instead---in general, if the capturers had "Capturer" in their names, and the renderers "Renderer". https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:50: }); Excellent use of SetData with a lambda! https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:255: done_capturing_.Set(); Nit: It looks like keep_recording could be named keep_capturing, for consistency. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:48: }; I still think it's wrong to have this one here---see reply elsewhere. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:53: using Streamer::Streamer; This using statement is an implementation detail that's best left out of the the header. I think that'll happen naturally if you follow my advice about Streamer. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:56: // return an empty ArrayView when the capture finishes. This comment is out of date. Also, instead of "Should capture", just write "Captures"---it's shorter and means the same thing in a context such as this one. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:87: static std::unique_ptr<Capturer> CreateWavFileReader( CreateWavFileCapturer? https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:95: static std::unique_ptr<Renderer> CreateWavFileWriter( CreateWavFileRenderer? https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:98: static std::unique_ptr<Renderer> CreateDiscarder( CreateDiscardRenderer? https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); As I said before, since the sample rate is at most 48kHz, you can easily allocate this one on the stack when you need it.
https://codereview.webrtc.org/2717623003/diff/260001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:151: FakeAudioDevice::CreateDiscarder(48000), audio_rtp_speed); On 2017/03/13 10:18:21, kwiberg-webrtc wrote: > This looks very nice! > > Hmm. You don't have to, but call sites like this one might get even easier to > read if CreateDiscarder was called CreateDiscardRenderer instead---in general, > if the capturers had "Capturer" in their names, and the renderers "Renderer". Changed to CreateDiscardRenderer, but it's hard to do the same for 'CreateWavFileWriter' because if I include 'Renderer' there, it would be confusing as it does not render WAV files... https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:255: done_capturing_.Set(); On 2017/03/13 10:18:21, kwiberg-webrtc wrote: > Nit: It looks like keep_recording could be named keep_capturing, for > consistency. Done. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:48: }; On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > I still think it's wrong to have this one here---see reply elsewhere. Done. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:53: using Streamer::Streamer; On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > This using statement is an implementation detail that's best left out of the the > header. I think that'll happen naturally if you follow my advice about Streamer. Done. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:56: // return an empty ArrayView when the capture finishes. On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > This comment is out of date. Also, instead of "Should capture", just write > "Captures"---it's shorter and means the same thing in a context such as this > one. Done. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:95: static std::unique_ptr<Renderer> CreateWavFileWriter( On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > CreateWavFileRenderer? I avoided this name because it would confuse even myself. This class does not render WAV files. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:98: static std::unique_ptr<Renderer> CreateDiscarder( On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > CreateDiscardRenderer? Done. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > As I said before, since the sample rate is at most 48kHz, you can easily > allocate this one on the stack when you need it. Now that it's a Buffer, it manages a heap allocated array regardless of where I put it. I don't think recreating the buffer every time would be a good idea.
The CQ bit was checked by oprypin@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/2717623003/diff/260001/webrtc/call/call_perf_te... File webrtc/call/call_perf_tests.cc (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/call/call_perf_te... webrtc/call/call_perf_tests.cc:151: FakeAudioDevice::CreateDiscarder(48000), audio_rtp_speed); On 2017/03/13 13:55:14, oprypin_webrtc wrote: > On 2017/03/13 10:18:21, kwiberg-webrtc wrote: > > This looks very nice! > > > > Hmm. You don't have to, but call sites like this one might get even easier to > > read if CreateDiscarder was called CreateDiscardRenderer instead---in general, > > if the capturers had "Capturer" in their names, and the renderers "Renderer". > > Changed to CreateDiscardRenderer, but it's hard to do the same for > 'CreateWavFileWriter' because if I include 'Renderer' there, it would be > confusing as it does not render WAV files... Fair point. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/13 13:55:14, oprypin_webrtc wrote: > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > As I said before, since the sample rate is at most 48kHz, you can easily > > allocate this one on the stack when you need it. > > Now that it's a Buffer, it manages a heap allocated array regardless of where I > put it. I don't think recreating the buffer every time would be a good idea. Since allocating an array on the stack is ~free, the amortized allocation cost is the same for a stack allocated array and a persistent heap allocated array (which is what this rtc::Buffer ends up being). The advantage with having the array on the stack is that it doesn't tie up memory between ProcessAudio() calls, and that stack memory has a higher chance than the persistent heap array of already being in the cache. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:191: sample_rate == 44100 || sample_rate == 48000); Do the CHECK first in this function, so you don't have to write that mouthful twice? You don't have to bend over backwards to not trigger a check for illegal sample_rate if both renderer_ and capturer_ are null... https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { This condition is always true, right? (You could use SetData with a lambda here, if you wanted.) https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:37: static size_t SamplesPerFrame(int sampling_frequency_in_hz); Can this function be in the anonymous namespace in the .cc file? https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:47: // SamplesPerFrame. Returns true if the capturer can keep producing data, Hmmm... why the behavior described in the second sentence? Wouldn't it be enough to say just "Replaces the contents of |buffer| with at most 10 ms of captured audio data.", or something like that?
https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:191: sample_rate == 44100 || sample_rate == 48000); On 2017/03/13 14:22:28, kwiberg-webrtc wrote: > Do the CHECK first in this function, so you don't have to write that mouthful > twice? You don't have to bend over backwards to not trigger a check for illegal > sample_rate if both renderer_ and capturer_ are null... I'm checking two different sample rates. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > This condition is always true, right? > > (You could use SetData with a lambda here, if you wanted.) It is not true at the end of the recording. Don't want to "bother" the receiver when there is actually no data. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:37: static size_t SamplesPerFrame(int sampling_frequency_in_hz); On 2017/03/13 14:22:28, kwiberg-webrtc wrote: > Can this function be in the anonymous namespace in the .cc file? I intend to implement more specialized Capturers/Renderers outside of this .cc file, and they need to use it. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:47: // SamplesPerFrame. Returns true if the capturer can keep producing data, On 2017/03/13 14:22:29, kwiberg-webrtc wrote: > Hmmm... why the behavior described in the second sentence? Wouldn't it be enough > to say just "Replaces the contents of |buffer| with at most 10 ms of captured > audio data.", or something like that? It's just not obvious what 10ms of data is, I thought this would make it more convenient. Made it simpler.
https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > As I said before, since the sample rate is at most 48kHz, you can easily > > > allocate this one on the stack when you need it. > > > > Now that it's a Buffer, it manages a heap allocated array regardless of where > I > > put it. I don't think recreating the buffer every time would be a good idea. > > Since allocating an array on the stack is ~free, the amortized allocation cost > is the same for a stack allocated array and a persistent heap allocated array > (which is what this rtc::Buffer ends up being). The advantage with having the > array on the stack is that it doesn't tie up memory between ProcessAudio() > calls, and that stack memory has a higher chance than the persistent heap array > of already being in the cache. I don't see how I can have an array on the stack when using a BufferT. Unless you're talking about playout_buffer_ (vector)...
https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/13 15:35:20, oprypin_webrtc wrote: > On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > > As I said before, since the sample rate is at most 48kHz, you can easily > > > > allocate this one on the stack when you need it. > > > > > > Now that it's a Buffer, it manages a heap allocated array regardless of > where > > I > > > put it. I don't think recreating the buffer every time would be a good idea. > > > > Since allocating an array on the stack is ~free, the amortized allocation cost > > is the same for a stack allocated array and a persistent heap allocated array > > (which is what this rtc::Buffer ends up being). The advantage with having the > > array on the stack is that it doesn't tie up memory between ProcessAudio() > > calls, and that stack memory has a higher chance than the persistent heap > array > > of already being in the cache. > > I don't see how I can have an array on the stack when using a BufferT. Unless > you're talking about playout_buffer_ (vector)... Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need an old-fashioned C array or a std::array. (BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I guess in principle you could give it a custom allocator that gets its memory from an array on the stack, but don't go there...) https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:191: sample_rate == 44100 || sample_rate == 48000); On 2017/03/13 15:11:36, oprypin_webrtc wrote: > On 2017/03/13 14:22:28, kwiberg-webrtc wrote: > > Do the CHECK first in this function, so you don't have to write that mouthful > > twice? You don't have to bend over backwards to not trigger a check for > illegal > > sample_rate if both renderer_ and capturer_ are null... > > I'm checking two different sample rates. Right, I would have seen that if I hadn't been so sloppy when reading the code... How about a subroutine then? auto good_sample_rate = [](int sr){ return sr == 8000 || sr == 16000 || sr == 32000 || sr == 44100 || sr == 48000; } [...] RTC_CHECK(good_sample_rate(renderer_->SamplingFrequency())); [...] https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/13 15:11:36, oprypin_webrtc wrote: > On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > > This condition is always true, right? > > > > (You could use SetData with a lambda here, if you wanted.) > > It is not true at the end of the recording. Don't want to "bother" the receiver > when there is actually no data. On line 273, you SetSize() the buffer to SamplesPerFrame(capturer_->SamplingFrequency()). Are you saying this number can be zero? https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:37: static size_t SamplesPerFrame(int sampling_frequency_in_hz); On 2017/03/13 15:11:36, oprypin_webrtc wrote: > On 2017/03/13 14:22:28, kwiberg-webrtc wrote: > > Can this function be in the anonymous namespace in the .cc file? > > I intend to implement more specialized Capturers/Renderers outside of this .cc > file, and they need to use it. Acknowledged. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:47: // SamplesPerFrame. Returns true if the capturer can keep producing data, On 2017/03/13 15:11:36, oprypin_webrtc wrote: > On 2017/03/13 14:22:29, kwiberg-webrtc wrote: > > Hmmm... why the behavior described in the second sentence? Wouldn't it be > enough > > to say just "Replaces the contents of |buffer| with at most 10 ms of captured > > audio data.", or something like that? > > It's just not obvious what 10ms of data is, I thought this would make it more > convenient. Made it simpler. Thanks, looks good now.
https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > On 2017/03/13 15:35:20, oprypin_webrtc wrote: > > On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > > > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > > > As I said before, since the sample rate is at most 48kHz, you can easily > > > > > allocate this one on the stack when you need it. > > > > > > > > Now that it's a Buffer, it manages a heap allocated array regardless of > > where > > > I > > > > put it. I don't think recreating the buffer every time would be a good > idea. > > > > > > Since allocating an array on the stack is ~free, the amortized allocation > cost > > > is the same for a stack allocated array and a persistent heap allocated > array > > > (which is what this rtc::Buffer ends up being). The advantage with having > the > > > array on the stack is that it doesn't tie up memory between ProcessAudio() > > > calls, and that stack memory has a higher chance than the persistent heap > > array > > > of already being in the cache. > > > > I don't see how I can have an array on the stack when using a BufferT. Unless > > you're talking about playout_buffer_ (vector)... > > Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need an > old-fashioned C array or a std::array. > > (BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I guess in > principle you could give it a custom allocator that gets its memory from an > array on the stack, but don't go there...) I still don't understand how I can make an improvement here. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:191: sample_rate == 44100 || sample_rate == 48000); On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > On 2017/03/13 15:11:36, oprypin_webrtc wrote: > > On 2017/03/13 14:22:28, kwiberg-webrtc wrote: > > > Do the CHECK first in this function, so you don't have to write that > mouthful > > > twice? You don't have to bend over backwards to not trigger a check for > > illegal > > > sample_rate if both renderer_ and capturer_ are null... > > > > I'm checking two different sample rates. > > Right, I would have seen that if I hadn't been so sloppy when reading the > code... How about a subroutine then? > > auto good_sample_rate = [](int sr){ > return sr == 8000 || sr == 16000 || sr == 32000 > || sr == 44100 || sr == 48000; > } > [...] > RTC_CHECK(good_sample_rate(renderer_->SamplingFrequency())); > [...] Done. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > On 2017/03/13 15:11:36, oprypin_webrtc wrote: > > On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > > > This condition is always true, right? > > > > > > (You could use SetData with a lambda here, if you wanted.) > > > > It is not true at the end of the recording. Don't want to "bother" the > receiver > > when there is actually no data. > > On line 273, you SetSize() the buffer to > SamplesPerFrame(capturer_->SamplingFrequency()). Are you saying this number can > be zero? No. The Capture method is free to change the buffer's size, possibly even zero size at the end of the recording.
The CQ bit was checked by oprypin@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.
lgtm, but see the question about the possibly redundant SetSize() call. Leave recording_buffer_ as it is, or change it to a stack allocated array---your choice. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/14 11:58:12, oprypin_webrtc wrote: > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > On 2017/03/13 15:35:20, oprypin_webrtc wrote: > > > On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > > > > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > > > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > > > > As I said before, since the sample rate is at most 48kHz, you can > easily > > > > > > allocate this one on the stack when you need it. > > > > > > > > > > Now that it's a Buffer, it manages a heap allocated array regardless of > > > where > > > > I > > > > > put it. I don't think recreating the buffer every time would be a good > > idea. > > > > > > > > Since allocating an array on the stack is ~free, the amortized allocation > > cost > > > > is the same for a stack allocated array and a persistent heap allocated > > array > > > > (which is what this rtc::Buffer ends up being). The advantage with having > > the > > > > array on the stack is that it doesn't tie up memory between ProcessAudio() > > > > calls, and that stack memory has a higher chance than the persistent heap > > > array > > > > of already being in the cache. > > > > > > I don't see how I can have an array on the stack when using a BufferT. > Unless > > > you're talking about playout_buffer_ (vector)... > > > > Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need an > > old-fashioned C array or a std::array. > > > > (BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I guess > in > > principle you could give it a custom allocator that gets its memory from an > > array on the stack, but don't go there...) > > I still don't understand how I can make an improvement here. By removing this member, and doing something like this in ProcessAudio(): if (capturing_) { // Capture 10ms of audio. 2 bytes per sample. int16_t recording_buffer[kMaxRecordingBufferSize]; size_t recorded_size; const bool keep_capturing = capturer_->Capture(recording_buffer, &recorded_size); uint32_t new_mic_level; if (recorded_size > 0) { audio_callback_->RecordedDataIsAvailable( recording_buffer, recorded_size, 2, 1, capturer_->SamplingFrequency(), 0, 0, 0, false, new_mic_level); } if (!keep_capturing) { capturing_ = false; done_capturing_.Set(); } } This would require Capture to have a signature such as bool(ArrayView<int16_t>, size_t*), though, which is different from bool(BufferT<int16_t>*) that I've pushed for elsewhere in this review. I hadn't realized the discrepancy---sorry! No wonder you're confused... The solution you've already coded, the one with Buffer, is easier to read (because you don't have to manage the memory manually); the solution with a stack-allocated array probably has better performance because it uses less memory and is more cache friendly. Use whichever one you like. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/14 11:58:13, oprypin_webrtc wrote: > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > On 2017/03/13 15:11:36, oprypin_webrtc wrote: > > > On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > > > > This condition is always true, right? > > > > > > > > (You could use SetData with a lambda here, if you wanted.) > > > > > > It is not true at the end of the recording. Don't want to "bother" the > > receiver > > > when there is actually no data. > > > > On line 273, you SetSize() the buffer to > > SamplesPerFrame(capturer_->SamplingFrequency()). Are you saying this number > can > > be zero? > > No. The Capture method is free to change the buffer's size, possibly even zero > size at the end of the recording. Ah, right! Sorry for being dense. However, since Capture will discard the contents of recording_buffer_ and fill it with however many elements it wants, the SetSize call on line 273 is entirely redundant, isn't it?
Thank you very much for the reviews, Karl! I really learned a lot in the process. https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/14 13:46:46, kwiberg-webrtc wrote: > On 2017/03/14 11:58:12, oprypin_webrtc wrote: > > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > > On 2017/03/13 15:35:20, oprypin_webrtc wrote: > > > > On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > > > > > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > > > > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > > > > > As I said before, since the sample rate is at most 48kHz, you can > > easily > > > > > > > allocate this one on the stack when you need it. > > > > > > > > > > > > Now that it's a Buffer, it manages a heap allocated array regardless > of > > > > where > > > > > I > > > > > > put it. I don't think recreating the buffer every time would be a good > > > idea. > > > > > > > > > > Since allocating an array on the stack is ~free, the amortized > allocation > > > cost > > > > > is the same for a stack allocated array and a persistent heap allocated > > > array > > > > > (which is what this rtc::Buffer ends up being). The advantage with > having > > > the > > > > > array on the stack is that it doesn't tie up memory between > ProcessAudio() > > > > > calls, and that stack memory has a higher chance than the persistent > heap > > > > array > > > > > of already being in the cache. > > > > > > > > I don't see how I can have an array on the stack when using a BufferT. > > Unless > > > > you're talking about playout_buffer_ (vector)... > > > > > > Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need > an > > > old-fashioned C array or a std::array. > > > > > > (BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I > guess > > in > > > principle you could give it a custom allocator that gets its memory from an > > > array on the stack, but don't go there...) > > > > I still don't understand how I can make an improvement here. > > By removing this member, and doing something like this in ProcessAudio(): > > if (capturing_) { > // Capture 10ms of audio. 2 bytes per sample. > int16_t recording_buffer[kMaxRecordingBufferSize]; > size_t recorded_size; > const bool keep_capturing = capturer_->Capture(recording_buffer, > &recorded_size); > uint32_t new_mic_level; > if (recorded_size > 0) { > audio_callback_->RecordedDataIsAvailable( > recording_buffer, recorded_size, 2, 1, > capturer_->SamplingFrequency(), 0, 0, 0, false, new_mic_level); > } > if (!keep_capturing) { > capturing_ = false; > done_capturing_.Set(); > } > } > > This would require Capture to have a signature such as bool(ArrayView<int16_t>, > size_t*), though, which is different from bool(BufferT<int16_t>*) that I've > pushed for elsewhere in this review. I hadn't realized the discrepancy---sorry! > No wonder you're confused... > > The solution you've already coded, the one with Buffer, is easier to read > (because you don't have to manage the memory manually); the solution with a > stack-allocated array probably has better performance because it uses less > memory and is more cache friendly. > > Use whichever one you like. I understand now. So you meant that a stack allocated array can be used instead of a BufferT, but I did not realize that because I was totally set on using the BufferT. Keeping BufferT. https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/14 13:46:47, kwiberg-webrtc wrote: > On 2017/03/14 11:58:13, oprypin_webrtc wrote: > > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > > On 2017/03/13 15:11:36, oprypin_webrtc wrote: > > > > On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > > > > > This condition is always true, right? > > > > > > > > > > (You could use SetData with a lambda here, if you wanted.) > > > > > > > > It is not true at the end of the recording. Don't want to "bother" the > > > receiver > > > > when there is actually no data. > > > > > > On line 273, you SetSize() the buffer to > > > SamplesPerFrame(capturer_->SamplingFrequency()). Are you saying this number > > can > > > be zero? > > > > No. The Capture method is free to change the buffer's size, possibly even zero > > size at the end of the recording. > > Ah, right! Sorry for being dense. > > However, since Capture will discard the contents of recording_buffer_ and fill > it with however many elements it wants, the SetSize call on line 273 is entirely > redundant, isn't it? Yes, I removed that line in a later patchset. In this patchset it implemented that weird behavior that we agreed to remove: "The final size of the buffer should not exceed its initial size, which is equal to SamplesPerFrame."
https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.h (right): https://codereview.webrtc.org/2717623003/diff/260001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.h:135: rtc::BufferT<int16_t> recording_buffer_ GUARDED_BY(lock_); On 2017/03/14 14:01:40, oprypin_webrtc wrote: > On 2017/03/14 13:46:46, kwiberg-webrtc wrote: > > On 2017/03/14 11:58:12, oprypin_webrtc wrote: > > > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > > > On 2017/03/13 15:35:20, oprypin_webrtc wrote: > > > > > On 2017/03/13 14:22:26, kwiberg-webrtc wrote: > > > > > > On 2017/03/13 13:55:14, oprypin_webrtc wrote: > > > > > > > On 2017/03/13 10:18:22, kwiberg-webrtc wrote: > > > > > > > > As I said before, since the sample rate is at most 48kHz, you can > > > easily > > > > > > > > allocate this one on the stack when you need it. > > > > > > > > > > > > > > Now that it's a Buffer, it manages a heap allocated array regardless > > of > > > > > where > > > > > > I > > > > > > > put it. I don't think recreating the buffer every time would be a > good > > > > idea. > > > > > > > > > > > > Since allocating an array on the stack is ~free, the amortized > > allocation > > > > cost > > > > > > is the same for a stack allocated array and a persistent heap > allocated > > > > array > > > > > > (which is what this rtc::Buffer ends up being). The advantage with > > having > > > > the > > > > > > array on the stack is that it doesn't tie up memory between > > ProcessAudio() > > > > > > calls, and that stack memory has a higher chance than the persistent > > heap > > > > > array > > > > > > of already being in the cache. > > > > > > > > > > I don't see how I can have an array on the stack when using a BufferT. > > > Unless > > > > > you're talking about playout_buffer_ (vector)... > > > > > > > > Yes, rtc::Buffer is heap-only. If you're allocating on the stack, you need > > an > > > > old-fashioned C array or a std::array. > > > > > > > > (BTW, std::vector is just like rtc::Buffer in this regard: heap-only. I > > guess > > > in > > > > principle you could give it a custom allocator that gets its memory from > an > > > > array on the stack, but don't go there...) > > > > > > I still don't understand how I can make an improvement here. > > > > By removing this member, and doing something like this in ProcessAudio(): > > > > if (capturing_) { > > // Capture 10ms of audio. 2 bytes per sample. > > int16_t recording_buffer[kMaxRecordingBufferSize]; > > size_t recorded_size; > > const bool keep_capturing = capturer_->Capture(recording_buffer, > > &recorded_size); > > uint32_t new_mic_level; > > if (recorded_size > 0) { > > audio_callback_->RecordedDataIsAvailable( > > recording_buffer, recorded_size, 2, 1, > > capturer_->SamplingFrequency(), 0, 0, 0, false, new_mic_level); > > } > > if (!keep_capturing) { > > capturing_ = false; > > done_capturing_.Set(); > > } > > } > > > > This would require Capture to have a signature such as > bool(ArrayView<int16_t>, > > size_t*), though, which is different from bool(BufferT<int16_t>*) that I've > > pushed for elsewhere in this review. I hadn't realized the > discrepancy---sorry! > > No wonder you're confused... > > > > The solution you've already coded, the one with Buffer, is easier to read > > (because you don't have to manage the memory manually); the solution with a > > stack-allocated array probably has better performance because it uses less > > memory and is more cache friendly. > > > > Use whichever one you like. > > I understand now. So you meant that a stack allocated array can be used instead > of a BufferT, but I did not realize that because I was totally set on using the > BufferT. > Keeping BufferT. Well, no wonder you were set on using BufferT, because elsewhere in the same review I was busy trying to convince you to do precisely that... https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... File webrtc/test/fake_audio_device.cc (right): https://codereview.webrtc.org/2717623003/diff/280001/webrtc/test/fake_audio_d... webrtc/test/fake_audio_device.cc:277: if (recording_buffer_.size() > 0) { On 2017/03/14 14:01:40, oprypin_webrtc wrote: > On 2017/03/14 13:46:47, kwiberg-webrtc wrote: > > On 2017/03/14 11:58:13, oprypin_webrtc wrote: > > > On 2017/03/14 10:02:03, kwiberg-webrtc wrote: > > > > On 2017/03/13 15:11:36, oprypin_webrtc wrote: > > > > > On 2017/03/13 14:22:27, kwiberg-webrtc wrote: > > > > > > This condition is always true, right? > > > > > > > > > > > > (You could use SetData with a lambda here, if you wanted.) > > > > > > > > > > It is not true at the end of the recording. Don't want to "bother" the > > > > receiver > > > > > when there is actually no data. > > > > > > > > On line 273, you SetSize() the buffer to > > > > SamplesPerFrame(capturer_->SamplingFrequency()). Are you saying this > number > > > can > > > > be zero? > > > > > > No. The Capture method is free to change the buffer's size, possibly even > zero > > > size at the end of the recording. > > > > Ah, right! Sorry for being dense. > > > > However, since Capture will discard the contents of recording_buffer_ and fill > > it with however many elements it wants, the SetSize call on line 273 is > entirely > > redundant, isn't it? > > Yes, I removed that line in a later patchset. > > In this patchset it implemented that weird behavior that we agreed to remove: > "The final size of the buffer should not exceed its initial size, which is equal > to SamplesPerFrame." Acknowledged.
On 2017/03/14 14:01:40, oprypin_webrtc wrote: > Thank you very much for the reviews, Karl! I really learned a lot in the > process. Perhaps most importantly that you shouldn't always trust the reviewers! :-)
The CQ bit was checked by oprypin@webrtc.org
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": 320001, "attempt_start_ts": 1489507168965750,
"parent_rev": "cd386eb13fdc2420a0e6811ec2beca2d553f2166", "commit_rev":
"a514584b9a594447b78ed15bba7504b5121d2fcd"}
Message was sent while issue was closed.
Description was changed from ========== Add the ability to read/write to WAV files in FakeAudioDevice BUG=webrtc:7229 ========== to ========== Add the ability to read/write to WAV files in FakeAudioDevice BUG=webrtc:7229 Review-Url: https://codereview.webrtc.org/2717623003 Cr-Commit-Position: refs/heads/master@{#17230} Committed: https://chromium.googlesource.com/external/webrtc/+/a514584b9a594447b78ed15bb... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/a514584b9a594447b78ed15bb...
Message was sent while issue was closed.
Very nice! LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
