Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2437913003:
Replaced thread checker with race checker in AudioMixer. (Closed)
DescriptionReplaced thread checker with race checker in AudioMixer.
This change is due to an incorrect understanding of the threading
model in Chrome. The new AudioMixer has a thread checker to ensure
that mixing is always done from a single thread. Mixing is done on the
Audio Output Thread. When run in Chrome, it can change. Even if the thread
changes, there is never more than one audio thread, and mixing is done
sequentially.
The threading checks and variable access checks are replaced with
rtc::RaceChecker counterparts.
NOTRY=True
BUG=webrtc:6346
Committed: https://crrev.com/920d30bc743b76d30c6cae7ad4c97af2e6c24e4c
Cr-Commit-Position: refs/heads/master@{#14712}
Patch Set 1 #
Total comments: 7
Patch Set 2 : reworded comments and removed lock addresses. #Patch Set 3 : Rebase after landing dependencies. #
Messages
Total messages: 18 (9 generated)
aleloi@webrtc.org changed reviewers: + kwiberg@webrtc.org
Short change, please take a look! https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:105: // Checks that Mix is called sequentially. We cannot *ensure* at compile time, because (if I understand correctly) the race checker is not guaranteed to detect a race.
Description was changed from ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. If the thread changes, there is never more than one audio thread, so mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. BUG=webrtc:6346 ========== to ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. BUG=webrtc:6346 ==========
lgtm, but see comments https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:93: int output_frequency_ GUARDED_BY(&race_checker_); Is the & necessary? https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:106: rtc::RaceChecker race_checker_; I would move this up to where crit_ is defined, and have a single comment that describes both of them. (Protecting different parts of a class with different protection thingies is slightly unusual, and deserves an explanation.)
https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:93: int output_frequency_ GUARDED_BY(&race_checker_); On 2016/10/20 09:37:14, kwiberg-webrtc wrote: > Is the & necessary? Turns out it isn't. I've removed it. I looked in thread_annotations.h and couldn't find anything. Then I searched for what is used in the rest of WebRTC code. We use both, but no address is more common than address. I checked that it still works by compiling without acquiring a lock. https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:106: rtc::RaceChecker race_checker_; On 2016/10/20 09:37:14, kwiberg-webrtc wrote: > I would move this up to where crit_ is defined, and have a single comment that > describes both of them. (Protecting different parts of a class with different > protection thingies is slightly unusual, and deserves an explanation.) Moved up and reworded comment.
lgtm https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:93: int output_frequency_ GUARDED_BY(&race_checker_); On 2016/10/20 09:52:01, aleloi wrote: > On 2016/10/20 09:37:14, kwiberg-webrtc wrote: > > Is the & necessary? > Turns out it isn't. I've removed it. > > I looked in thread_annotations.h and couldn't find anything. Then I searched for > what is used in the rest of WebRTC code. We use both, but no address is more > common than address. I checked that it still works by compiling without > acquiring a lock. 👍 https://codereview.webrtc.org/2437913003/diff/1/webrtc/modules/audio_mixer/au... webrtc/modules/audio_mixer/audio_mixer_impl.h:106: rtc::RaceChecker race_checker_; On 2016/10/20 09:52:01, aleloi wrote: > On 2016/10/20 09:37:14, kwiberg-webrtc wrote: > > I would move this up to where crit_ is defined, and have a single comment that > > describes both of them. (Protecting different parts of a class with different > > protection thingies is slightly unusual, and deserves an explanation.) > > Moved up and reworded comment. Excellent.
Description was changed from ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. BUG=webrtc:6346 ========== to ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. NOTRY=True BUG=webrtc:6346 ==========
The CQ bit was checked by aleloi@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/2437913003/#ps40001 (title: "Rebase after landing dependencies.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by aleloi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. NOTRY=True BUG=webrtc:6346 ========== to ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. NOTRY=True BUG=webrtc:6346 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. NOTRY=True BUG=webrtc:6346 ========== to ========== Replaced thread checker with race checker in AudioMixer. This change is due to an incorrect understanding of the threading model in Chrome. The new AudioMixer has a thread checker to ensure that mixing is always done from a single thread. Mixing is done on the Audio Output Thread. When run in Chrome, it can change. Even if the thread changes, there is never more than one audio thread, and mixing is done sequentially. The threading checks and variable access checks are replaced with rtc::RaceChecker counterparts. NOTRY=True BUG=webrtc:6346 Committed: https://crrev.com/920d30bc743b76d30c6cae7ad4c97af2e6c24e4c Cr-Commit-Position: refs/heads/master@{#14712} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/920d30bc743b76d30c6cae7ad4c97af2e6c24e4c Cr-Commit-Position: refs/heads/master@{#14712} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
