Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466613002:
Adds thread safety annotations to the AudioDeviceBuffer class (Closed)
|
Created:
4 years, 1 month ago by henrika_webrtc Modified:
4 years, 1 month ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Description- Adds thread safety annotations to the AudioDeviceBuffer class.
- Removes the lock that was used to protect the audio transport object.
It is now protected "by design" instead.
- Removes rec/play_bytes_per_sample_ since we only support 16-bit samples.
BUG=webrtc:6560
R=kwiberg@webrtc.org
Committed: https://crrev.com/f50222244371fdcb00190017a40afa2c0796fed1
Cr-Commit-Position: refs/heads/master@{#14950}
Patch Set 1 #Patch Set 2 : Adds race checker and removes test of ResetAudioDevice #
Total comments: 17
Patch Set 3 : Removed race checker #Patch Set 4 : Initial feedback from kwiberg@ #Patch Set 5 : Improved thread annotation #
Total comments: 14
Patch Set 6 : More changes based on feedback from kwiberg@ #Patch Set 7 : Improved annotation and removed unused logging of playout difftimes #Patch Set 8 : Removed race checkers #Patch Set 9 : improved comment #
Total comments: 2
Patch Set 10 : Minor changes to ensure that all unit tests works #Patch Set 11 : Minor thread change for Windows #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== Adds thread safety annotations to the AudioDeviceBuffer class BUG= ========== to ========== Adds thread safety annotations to the AudioDeviceBuffer class BUG=webrtc:6560 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
Description was changed from ========== Adds thread safety annotations to the AudioDeviceBuffer class BUG=webrtc:6560 ========== to ========== Adds thread safety annotations to the AudioDeviceBuffer class. Also adds a race checker and removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. BUG=webrtc:6560 ==========
PTAL
Description was changed from ========== Adds thread safety annotations to the AudioDeviceBuffer class. Also adds a race checker and removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. BUG=webrtc:6560 ========== to ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Adds a race checker and removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 ==========
Description was changed from ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Adds a race checker and removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 ========== to ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 ==========
(I reviewed patch set #2, where there was still a race checker.) Nice, but I have some comments. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:209: rtc::CritScope lock(&lock_); If every call to this function is on the same thread, why do you need to lock? You should only need one of these, I think. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:217: rtc::CritScope lock(&lock_); Again, you shouldn't need both the thread checker and the lock, right? (This happens more times below. The only legitimate reason I can think of is if you have one member protected by the thread checker and one protected by the lock, and you need to access both at the same time; it's the same sort of circumstance under which you'd need to acquire to locks.) https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:372: }(); By taking the lock twice like this, you pay twice the performance cost, and also risk getting a pair of values that never existed simultaneously (if someone changes stuff after you release the lock the first time, before you acquire it for the second time). It's better to read both variables at once, I think (but you can't easily use this cute lambda trick to do so until C++17). https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:500: // Read |play_sample_rate_| and |rec_sample_rate_| under exclusive lock. The comment isn't necessary. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:110: rtc::ThreadChecker thread_checker_; No member variable is annotated as being protected by thread_checker_. Is it not used? https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:128: rtc::TaskQueue task_queue_; I presume this object is itself thread safe, so that you don't need to protect it by a separate critical section? https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:134: bool recording_; These are not annotated. It's usually good to make sure that *every* member variable (apart from the mutex(es)...) are annotated, or have a comment explaining why they don't need to be protected. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:154: uint32_t new_mic_level_; More unannotated variables. Please annotate all of them. (It's not that a partial annotation isn't useful, it's just that if one member needs protection, very likely they all do, so there's usually no point in not annotating everything.)
Thanks! Don't review yet. Will upload new version. If you have time, please check my questions.
https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:209: rtc::CritScope lock(&lock_); Please correct me if I am wrong but: I want to protect rec_sample_rate_ given an assumption that it is set on one thread (the creating thread) and read on another thread (OS/audio specific). So we need a lock. But, I want to be sure that the lock is needed and therefore I add the thread check. Wrong thinking? https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:217: rtc::CritScope lock(&lock_); see above https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:372: }(); Removed lambda. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:500: // Read |play_sample_rate_| and |rec_sample_rate_| under exclusive lock. On 2016/11/01 15:53:50, kwiberg-webrtc wrote: > The comment isn't necessary. Done. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:110: rtc::ThreadChecker thread_checker_; As we discussed. Will try to fix that. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:128: rtc::TaskQueue task_queue_; Yes, dtor and ctor are thread checked and I can't see any reason why a lock is needed. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:134: bool recording_; On 2016/11/01 15:53:50, kwiberg-webrtc wrote: > These are not annotated. It's usually good to make sure that *every* member > variable (apart from the mutex(es)...) are annotated, or have a comment > explaining why they don't need to be protected. Acknowledged. https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:154: uint32_t new_mic_level_; On 2016/11/01 15:53:50, kwiberg-webrtc wrote: > More unannotated variables. Please annotate all of them. (It's not that a > partial annotation isn't useful, it's just that if one member needs protection, > very likely they all do, so there's usually no point in not annotating > everything.) Acknowledged.
https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/20001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:209: rtc::CritScope lock(&lock_); On 2016/11/02 10:29:17, henrika_webrtc wrote: > Please correct me if I am wrong but: I want to protect rec_sample_rate_ given an > assumption that it is set on one thread (the creating thread) and read on > another thread (OS/audio specific). So we need a lock. But, I want to be sure > that the lock is needed and therefore I add the thread > check. > > Wrong thinking? Yes, for that situation you need a lock. I don't see what you want the thread checker to do, though. It will verify that you're always on the same thread when you call it. How does this help here? If I understand correctly, you want to verify that two different methods are always called from different threads.
I was able to annotate almost all members. Not sure if I can add for the last three as well. PTAL
https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:117: // here since the media has not started yet (known by design). So RTC_DCHECK_RUN_ON the audio thread, and detach it afterwards? That way, you can annotate last_playout_time_ as being protected by that thread checker. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:375: size_t rec_sample_rate = 0; Better to not initialize. That way, the reader can tell that they must be set further down. And MSan will know if you try to read the without setting them first. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:411: size_t play_sample_rate = 0; Same here. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:508: size_t rec_sample_rate = 0; Same here. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:107: rtc::ThreadChecker thread_checker_; Could this one get a descriptive name too? https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:111: rtc::CriticalSection lock_; OK, *three* thread checkers... *and* a lock. I'd like a comment that explains this. (You're being called from three different threads, and some data is private to each thread, while some data is shared between threads and needs to be locked?) I have a feeling that this indicates that it would be useful to split this class into (at least) three pieces, one for each thread, but that's out of scope for this CL. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:163: size_t num_stat_reports_ ACCESS_ON(task_queue_); Aha, a task queue annotation. Excellent!
On 2016/11/02 13:36:17, henrika_webrtc wrote: > I was able to annotate almost all members. > > Not sure if I can add for the last three as well. We should really try to get those annotated too.
Was able to come up with a version without any locks at all. It is super hard to annotate all members or explain in comments how it is possible. Perhaps we can take it offline instead. Please note that this class is old and has not been changed in a very long time but the surroundings have and it is now used in a more complex environment than when it was created. Anyhow, I think I can explain all the details if we sit down together. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:117: // here since the media has not started yet (known by design). Not sure if I understand; care to elaborate? https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:375: size_t rec_sample_rate = 0; On 2016/11/02 14:14:57, kwiberg-webrtc wrote: > Better to not initialize. That way, the reader can tell that they must be set > further down. And MSan will know if you try to read the without setting them > first. Done. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:411: size_t play_sample_rate = 0; On 2016/11/02 14:14:57, kwiberg-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:508: size_t rec_sample_rate = 0; On 2016/11/02 14:14:57, kwiberg-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:107: rtc::ThreadChecker thread_checker_; On 2016/11/02 14:14:57, kwiberg-webrtc wrote: > Could this one get a descriptive name too? Done. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:111: rtc::CriticalSection lock_; It is actually not that complicated. Let me try to explain that inline. https://codereview.webrtc.org/2466613002/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:163: size_t num_stat_reports_ ACCESS_ON(task_queue_); ;-)
I think it is perfect now ;-) PTAL
lgtm, but see comment. Too bad you didn't get all the way there, but annotating 90% of the members is still a vast improvement over the previous state of affairs. https://codereview.webrtc.org/2466613002/diff/160001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/160001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:442: // TODO(henrika): FIX A more descriptive comment before you commit this, please. :-)
Thanks. Might need some minor changes to ensure that all unit tests works. Trust me ;-) https://codereview.webrtc.org/2466613002/diff/160001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2466613002/diff/160001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:442: // TODO(henrika): FIX Sorry. Done.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2466613002/#ps180001 (title: "Minor changes to ensure that all unit tests works")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/15753)
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2466613002/#ps200001 (title: "Minor thread change for Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/19338)
Landing manually since I am unable to reproduce the issues above locally. Must be flake.
Landing manually since I am unable to reproduce the issues above locally. Must be flake.
Description was changed from ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 ========== to ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f50222244371fdcb00190017a... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as f50222244371fdcb00190017a40afa2c0796fed1 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f50222244371fdcb00190017a... ========== to ========== - Adds thread safety annotations to the AudioDeviceBuffer class. - Removes the lock that was used to protect the audio transport object. It is now protected "by design" instead. - Removes rec/play_bytes_per_sample_ since we only support 16-bit samples. BUG=webrtc:6560 R=kwiberg@webrtc.org Committed: https://crrev.com/f50222244371fdcb00190017a40afa2c0796fed1 Cr-Commit-Position: refs/heads/master@{#14950} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
