Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 8 months ago by henrika_webrtc Modified:
3 years, 8 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest.
BUG=webrtc:7273
Review-Url: https://codereview.webrtc.org/2788883002
Cr-Commit-Position: refs/heads/master@{#17555}
Committed: https://chromium.googlesource.com/external/webrtc/+/e24991d7d659c6d33b17652dc07244c8cfad6124
Patch Set 1 #
Total comments: 33
Patch Set 2 : Feedback from kwiberg@ #
Total comments: 16
Patch Set 3 : Second round of comments from kwiberg@ #Patch Set 4 : Now using rtc::Buffer #
Total comments: 14
Patch Set 5 : almost done #Patch Set 6 : Final changes #
Created: 3 years, 8 months ago
Messages
Total messages: 31 (19 generated)
Description was changed from ========== Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest Changed flush() Addes full-duplex test mode to ADM test BUG= ========== to ========== Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest. BUG=webrtc:7273 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
The CQ bit was checked by henrika@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:58: static const int kFullDuplexTimeInSec = 5; These two can be constexpr instead of const. Not that important in this specific case, but a good habit to get into. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:70: class AudioStreamInterface { Suggestion: Just "AudioStream". The "Interface" suffix isn't necessary (but if you really like it, keep it). https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:77: size_t channels) = 0; Can you make this a concrete type instead of void? That makes the code easier to read, and the compiler will catch more errors for you. Also, strongly consider replacing the raw pointer with ArrayView or something that keeps track of the length. That makes the code much easier to read, since you don't have to read the docs to see how long the array is supposed to be. (And in this case, as in so many others, there aren't even any docs...) https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:79: AudioStreamInterface() = default; This is a pure interface. No constructor needed. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:108: int16_t* memory = new int16_t[samples_per_buffer]; Raw pointers shouldn't own stuff. Use unique_ptr instead. (This is a good place to use WrapUnique.) Also, it's less error prone to use rtc::Buffer or std::vector or something that also tracks its length, instead of a raw heap array. Especially in test code, where performance isn't critical... https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:109: memcpy(static_cast<int16_t*>(&memory[0]), source, bytes_per_buffer); This cast looks unnecessary. (But if you use rtc::Buffer instead, this memcpy will go away entirely, since you can give the data as a constructor argument.) https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:115: } You can avoid mutating the size variable: const size_t size = [&]{ rtc::CritScope lock(&lock_); fifo_->push_back(memory); return fifo_->size(); }(); https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:133: int16_t* memory = fifo_->front(); Again, definitely use unique_ptr instead of a raw pointer, and consider using rtc::Buffer instead. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:140: size_t size() const { return fifo_->size(); } You should take lock_ before touching fifo_. See the request for lock annotations below... https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:159: using AudioBufferList = std::list<int16_t*>; This should be std::list<std::unique_ptr<int16_t[]>>, or std::list<rtc::BufferT<int16_t>>. Raw pointers shouldn't own things. Once you do this, you can get rid of the Flush method. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:161: std::unique_ptr<AudioBufferList> fifo_; Why the indirection? Can't you have the AudioBufferList as a normal class member? Also, I don't think the AudioBufferList alias is very useful. You can probably get rid of it without making the code any harder to read. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:164: size_t written_elements_ = 0; Please annotate which variables are protected by what. Also, for the variables that aren't protected by the lock, consider using a race checker or thread checker. (But if performance isn't an issue, consider protecting everything with lock_ for simplicity.) https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:492: // one packet on average. However, under more realistic conditions, the size Max 1, average somewhere in (0,1) depending on how long the packets are buffered. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:501: std::unique_ptr<FifoAudioStream> audio_stream(new FifoAudioStream()); Why the indirection? It looks like you could put the FifoAudioStream directly on the stack. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:517: EXPECT_LE(audio_stream->average_size(), 5u); This looks like a test that can be expected to flake sometimes. Can that be avoided, or won't it be a problem?
Thanks! PTAL ;-) PS still no rtc::Buffer https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:58: static const int kFullDuplexTimeInSec = 5; Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:70: class AudioStreamInterface { Removed "Interface" https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:77: size_t channels) = 0; On 2017/04/03 08:40:12, kwiberg-webrtc wrote: > Can you make this a concrete type instead of void? That makes the code easier to > read, and the compiler will catch more errors for you. > > Also, strongly consider replacing the raw pointer with ArrayView or something > that keeps track of the length. That makes the code much easier to read, since > you don't have to read the docs to see how long the array is supposed to be. > (And in this case, as in so many others, there aren't even any docs...) Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:79: AudioStreamInterface() = default; On 2017/04/03 08:40:11, kwiberg-webrtc wrote: > This is a pure interface. No constructor needed. Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:108: int16_t* memory = new int16_t[samples_per_buffer]; Switched to unique_ptr. I am unable to see any real benefit by using WrapUniqe. Can you elaborate? https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:109: memcpy(static_cast<int16_t*>(&memory[0]), source, bytes_per_buffer); Let me add first round without using rtc::Buffer first. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:115: } Yes we can :-) https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:133: int16_t* memory = fifo_->front(); Using unique_ptr to see if you like it :-) https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:140: size_t size() const { return fifo_->size(); } On 2017/04/03 08:40:11, kwiberg-webrtc wrote: > You should take lock_ before touching fifo_. See the request for lock > annotations below... Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:159: using AudioBufferList = std::list<int16_t*>; On 2017/04/03 08:40:12, kwiberg-webrtc wrote: > This should be std::list<std::unique_ptr<int16_t[]>>, or > std::list<rtc::BufferT<int16_t>>. Raw pointers shouldn't own things. > > Once you do this, you can get rid of the Flush method. Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:161: std::unique_ptr<AudioBufferList> fifo_; On 2017/04/03 08:40:11, kwiberg-webrtc wrote: > Why the indirection? Can't you have the AudioBufferList as a normal class > member? > > Also, I don't think the AudioBufferList alias is very useful. You can probably > get rid of it without making the code any harder to read. Acknowledged. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:164: size_t written_elements_ = 0; On 2017/04/03 08:40:12, kwiberg-webrtc wrote: > Please annotate which variables are protected by what. > > Also, for the variables that aren't protected by the lock, consider using a race > checker or thread checker. (But if performance isn't an issue, consider > protecting everything with lock_ for simplicity.) Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:492: // one packet on average. However, under more realistic conditions, the size Tried to rewrite as you suggest but I might have misunderstood your intention. Please check again. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:501: std::unique_ptr<FifoAudioStream> audio_stream(new FifoAudioStream()); On 2017/04/03 08:40:12, kwiberg-webrtc wrote: > Why the indirection? It looks like you could put the FifoAudioStream directly on > the stack. Done. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:517: EXPECT_LE(audio_stream->average_size(), 5u); I use rather strict conditions to be able to tun this test. No ALSA, no TSAN, MSAN etc. It should be a very rare case to end up with more than five in the list (famous last words...)
Much better now that you use unique_ptr. I still recommend that you use rtc::Buffer instead of handling the arrays manually (you can get rid of the memcpy and memset calls, and won't have to count bytes!), but it's just a recommendation. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:108: int16_t* memory = new int16_t[samples_per_buffer]; On 2017/04/03 15:14:29, henrika_webrtc wrote: > Switched to unique_ptr. I am unable to see any real benefit by using WrapUniqe. > Can you elaborate? You avoid having to name the type twice. That's more of an advantage for types with names longer than "int16_t"... However, I'll have to retract my suggestion, because WrapUnique doesn't work for array types. You could use MakeUnique: auto buffer = rtc::MakeUnique<int16_t[]>(source.size()); but for some reason this is specified to value-initialize the array (a.k.a. filling it with zeros). Personally, I would probably not use MakeUnique here. https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:492: // one packet on average. However, under more realistic conditions, the size On 2017/04/03 15:14:30, henrika_webrtc wrote: > Tried to rewrite as you suggest but I might have misunderstood your intention. > Please check again. It may be better to just say "Under such conditions, the FIFO would contain at most one packet." That the average would be in the range (0,1) is a trivial consequence of that; we'll probably just confuse people if we try to mention it... https://codereview.webrtc.org/2788883002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_unittest.cc:517: EXPECT_LE(audio_stream->average_size(), 5u); On 2017/04/03 15:14:29, henrika_webrtc wrote: > I use rather strict conditions to be able to tun this test. No ALSA, no TSAN, > MSAN etc. It should be a very rare case to end up with more than five in the > list (famous last words...) Acknowledged. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:95: virtual ~FifoAudioStream() {} You don't need these two lines. The compiler will give you precisely this destructor and default constructor anyway. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:102: memcpy(static_cast<int16_t*>(buffer.get()), source.data(), Is the cast necessary? https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:103: bytes_per_buffer); If channels were ever != 1, this math would be incorrect. You treat source.size() as if it is the number of samples per channel, but it must be the total number of samples. (Because the whole point of ArrayView is that its .size() always says how many elements you're supposed to access.) One way to fix it is to not have the samples_per_buffer variable, and use source.size() instead. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:119: sizeof(int16_t) * destination.size() * channels; Same problem here: channels > 1 would lead to out-of-bounds accesses for |destination|. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:125: memcpy(destination.data(), static_cast<int16_t*>(memory.get()), Is the cast necessary? https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:146: size_t written_elements_ = 0; I can haz a comment that explains why these three don't need protection against concurrent access? (Or just protect them with a thread checker, race checker, or lock_, as I suggested earlier. It may be fewer lines than a well-written comment...) https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:211: channels); You have the same problem here if channels != 1. The length of the ArrayView needs to be samples_per_channel * channels. Also, if you want, you can use MakeArrayView---that will allow you to not repeat the type name. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:251: channels); And here.
Thanks. Will see if I can use rtc::Buffer as well next. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:95: virtual ~FifoAudioStream() {} On 2017/04/03 20:44:02, kwiberg-webrtc wrote: > You don't need these two lines. The compiler will give you precisely this > destructor and default constructor anyway. Acknowledged. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:102: memcpy(static_cast<int16_t*>(buffer.get()), source.data(), no ;-) https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:103: bytes_per_buffer); Ack. I do check that #channels is one in line 98 but see your point. Changed now but kept stereo parameter to be able to verify that both sides use same #channels (otherwise I must convert the audio). https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:119: sizeof(int16_t) * destination.size() * channels; On 2017/04/03 20:44:02, kwiberg-webrtc wrote: > Same problem here: channels > 1 would lead to out-of-bounds accesses for > |destination|. Done. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:125: memcpy(destination.data(), static_cast<int16_t*>(memory.get()), no https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:146: size_t written_elements_ = 0; Well, they are in fact only accessed on the "Play thread" and then read after media has stopped. Hence, they are protected by design. Will explain. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:211: channels); On 2017/04/03 20:44:02, kwiberg-webrtc wrote: > You have the same problem here if channels != 1. The length of the ArrayView > needs to be samples_per_channel * channels. > > Also, if you want, you can use MakeArrayView---that will allow you to not repeat > the type name. Done. https://codereview.webrtc.org/2788883002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:251: channels); On 2017/04/03 20:44:02, kwiberg-webrtc wrote: > And here. Done.
I think it is perfect now ;-) PTAL
The CQ bit was checked by henrika@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.
Getting there! But you should probably pass the Buffers by value everywhere instead of by pointer. (They're not copyable, but they are moveable, which is all you need.) https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:97: Buffer16* buffer = new Buffer16(source.data(), source.size()); Here's a raw pointer that owns stuff. I would advise you to use unique_ptr instead, but I'm pretty sure that you should just put buffer directly on the stack. See the comment below on the fifo_ member variable. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:115: memset(destination.data(), 0, bytes_per_buffer); std::fill(destination.begin(), destination.end(), 0); That way, you can't mis-calculate the size of the region to fill. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:118: memcpy(destination.data(), buffer->data(), bytes_per_buffer); RTC_CHECK_EQ(buffer.size(), destination.size()); std::copy(buffer.begin(), buffer.end(), destination.begin()); Again, this makes it easier to not mis-calculate the size of the region. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:119: fifo_.pop_front(); Here you leak |buffer|. See the comment below on the fifo_ member variable. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:134: using Buffer16 = rtc::BufferT<int16_t>; You reduce the name from 21 to 8 characters, but readers will have to look up what "Buffer16" means. I'm guessing this is a bad trade-off, but I'll let you decide. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:136: std::list<Buffer16*> fifo_ GUARDED_BY(lock_); Who owns these buffers? (It would make more sense for the std::list to own them directly, in which case this type should be std::list<Buffer16>.) https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:139: // has been stopped. Hence, no lock is needed. OK. Using a race checker to actually verify this claim would have been nice, but you don't have to.
Thanks. Almost there. Just need rtc::Buffer::begin() and end() as well. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_unittest.cc (right): https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:97: Buffer16* buffer = new Buffer16(source.data(), source.size()); On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > Here's a raw pointer that owns stuff. I would advise you to use unique_ptr > instead, but I'm pretty sure that you should just put buffer directly on the > stack. See the comment below on the fifo_ member variable. Done. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:115: memset(destination.data(), 0, bytes_per_buffer); On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > std::fill(destination.begin(), destination.end(), 0); > > That way, you can't mis-calculate the size of the region to fill. Done. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:118: memcpy(destination.data(), buffer->data(), bytes_per_buffer); On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > RTC_CHECK_EQ(buffer.size(), destination.size()); > std::copy(buffer.begin(), buffer.end(), destination.begin()); > > Again, this makes it easier to not mis-calculate the size of the region. Done. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:119: fifo_.pop_front(); On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > Here you leak |buffer|. See the comment below on the fifo_ member variable. Done. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:134: using Buffer16 = rtc::BufferT<int16_t>; Keeping Buffer16 ;-) https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:136: std::list<Buffer16*> fifo_ GUARDED_BY(lock_); On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > Who owns these buffers? (It would make more sense for the std::list to own them > directly, in which case this type should be std::list<Buffer16>.) Acknowledged. https://codereview.webrtc.org/2788883002/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_unittest.cc:139: // has been stopped. Hence, no lock is needed. On 2017/04/05 09:29:12, kwiberg-webrtc wrote: > OK. Using a race checker to actually verify this claim would have been nice, but > you don't have to. Done.
The CQ bit was checked by henrika@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 checked by henrika@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/...
Now using rtc::Buffer::begin() and end(). PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm!!!
The CQ bit was checked by henrika@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": 100001, "attempt_start_ts": 1491466328514080,
"parent_rev": "978504e25cafc1ce59cf7fbcace3a331a9b98112", "commit_rev":
"e24991d7d659c6d33b17652dc07244c8cfad6124"}
Message was sent while issue was closed.
Description was changed from ========== Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest. BUG=webrtc:7273 ========== to ========== Adds AudioDeviceTest.RunPlayoutAndRecordingInFullDuplex unittest. BUG=webrtc:7273 Review-Url: https://codereview.webrtc.org/2788883002 Cr-Commit-Position: refs/heads/master@{#17555} Committed: https://chromium.googlesource.com/external/webrtc/+/e24991d7d659c6d33b17652dc... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/e24991d7d659c6d33b17652dc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
