Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1263223002:
Adding locking to webrtc::voe::Channel to fix race conditions (Closed)
|
Created:
5 years, 4 months ago by Taylor Brandstetter Modified:
5 years, 4 months ago CC:
hlundin-webrtc, henrika_webrtc, tterriberry_mozilla.com, webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding locking to webrtc::voe::Channel to fix race conditions
Some members are accessed from the video processing thread for the
VoEVideoSync interface, and thus need to be protected. This is a
problem that TSan sometimes reports.
Also moved UpdatePlayoutTimestamp to private section since
it's only needed internally. And renamed least_required_delay_ms
to LeastRequiredDelayMs, since it no longer just returns a cached
value.
BUG=webrtc:4663
Committed: https://crrev.com/743758816853df2040a21c5652b0d0e238b1512f
Cr-Commit-Position: refs/heads/master@{#9706}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Getting rid of cached "least_required_delay_ms_" #Patch Set 3 : Tightening critical sections #Patch Set 4 : Using GUARDED_BY, scoped_ptr, and newer naming conventions #Patch Set 5 : Fixing typo that was somehow introduced.. #
Total comments: 1
Patch Set 6 : Responding to comment #
Total comments: 8
Patch Set 7 : Moved LeastRequiredDelayMs definition to cc file #
Messages
Total messages: 29 (10 generated)
deadbeef@webrtc.org changed reviewers: + tina.legrand@webrtc.org
Hi Tina, I'm working on getting a test working under TSan, and found what seem to be some race conditions in the voice engine. Could you review my fix when you have some time? Thanks, -Taylor
tina.legrand@webrtc.org changed reviewers: + solenberg@webrtc.org - tina.legrand@webrtc.org
On 2015/08/03 at 21:13:10, deadbeef wrote: > Hi Tina, > > I'm working on getting a test working under TSan, and found what seem to be some race conditions in the voice engine. Could you review my fix when you have some time? Thanks, > > -Taylor Hi Taylor, I changed reviewer to Fredrik, since he knows this code way better than I do.
One comment, but I'll let solenberg@ remain the sole reviewer. https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:484: CriticalSectionWrapper& video_sync_critsect_; Please, add GUARDED_BY(video_sync_critsect_) on the member variables that are protected by this lock. It is provided by webrtc/base/thread_annotations.h.
https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:3939: CriticalSectionScoped cs(&video_sync_critsect_); Can you grab this just prior to writing _recPacketDelayMs and _average_jitter_buffer_delay_us ? AFAICT only these fields need to be protected: playout_timestamp_rtp_ _average_jitter_buffer_delay_us _recPacketDelayMs playout_delay_ms_ (and least_required_delay_ms_, unless we can get rid of that - see previous comment) Please make sure the CSs are as tight as possible around the accesses to these fields. https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:278: CriticalSectionScoped cs(&video_sync_critsect_); It looks like this function could call audio_coding_->LeastRequiredDelayMs() directly, skipping the lock (already locked lowere down) and local caching of the value. hlundin@, is there a reason why that wouldn't work?
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:278: CriticalSectionScoped cs(&video_sync_critsect_); On 2015/08/04 09:36:47, the sun wrote: > It looks like this function could call audio_coding_->LeastRequiredDelayMs() > directly, skipping the lock (already locked lowere down) and local caching of > the value. > > hlundin@, is there a reason why that wouldn't work? That should work.
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:3939: CriticalSectionScoped cs(&video_sync_critsect_); On 2015/08/04 09:36:47, the sun wrote: > Can you grab this just prior to writing _recPacketDelayMs and > _average_jitter_buffer_delay_us ? > > AFAICT only these fields need to be protected: > playout_timestamp_rtp_ > _average_jitter_buffer_delay_us > _recPacketDelayMs > playout_delay_ms_ > > (and least_required_delay_ms_, unless we can get rid of that - see previous > comment) > > Please make sure the CSs are as tight as possible around the accesses to these > fields. I mistakenly thought that jitter_buffer_playout_timestamp_ also needed to be protected, since UpdatePlayoutTimestamp was in the VoEVideoSync section. I moved it next to UpdatePacketDelay (if that's ok) and tightened the CSs accordingly. https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:278: CriticalSectionScoped cs(&video_sync_critsect_); On 2015/08/04 09:52:10, hlundin-webrtc wrote: > On 2015/08/04 09:36:47, the sun wrote: > > It looks like this function could call audio_coding_->LeastRequiredDelayMs() > > directly, skipping the lock (already locked lowere down) and local caching of > > the value. > > > > hlundin@, is there a reason why that wouldn't work? > > That should work. Done. https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:484: CriticalSectionWrapper& video_sync_critsect_; On 2015/08/04 06:56:46, hlundin-webrtc wrote: > Please, add GUARDED_BY(video_sync_critsect_) on the member variables that are > protected by this lock. It is provided by webrtc/base/thread_annotations.h. Done.
On 2015/08/05 03:04:48, Taylor Brandstetter wrote: > https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.cc > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.c... > webrtc/voice_engine/channel.cc:3939: CriticalSectionScoped > cs(&video_sync_critsect_); > On 2015/08/04 09:36:47, the sun wrote: > > Can you grab this just prior to writing _recPacketDelayMs and > > _average_jitter_buffer_delay_us ? > > > > AFAICT only these fields need to be protected: > > playout_timestamp_rtp_ > > _average_jitter_buffer_delay_us > > _recPacketDelayMs > > playout_delay_ms_ > > > > (and least_required_delay_ms_, unless we can get rid of that - see previous > > comment) > > > > Please make sure the CSs are as tight as possible around the accesses to these > > fields. > > I mistakenly thought that jitter_buffer_playout_timestamp_ also needed to be > protected, since UpdatePlayoutTimestamp was in the VoEVideoSync section. I moved > it next to UpdatePacketDelay (if that's ok) and tightened the CSs accordingly. > > https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h > File webrtc/voice_engine/channel.h (right): > > https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... > webrtc/voice_engine/channel.h:278: CriticalSectionScoped > cs(&video_sync_critsect_); > On 2015/08/04 09:52:10, hlundin-webrtc wrote: > > On 2015/08/04 09:36:47, the sun wrote: > > > It looks like this function could call audio_coding_->LeastRequiredDelayMs() > > > directly, skipping the lock (already locked lowere down) and local caching > of > > > the value. > > > > > > hlundin@, is there a reason why that wouldn't work? > > > > That should work. > > Done. > > https://codereview.webrtc.org/1263223002/diff/1/webrtc/voice_engine/channel.h... > webrtc/voice_engine/channel.h:484: CriticalSectionWrapper& video_sync_critsect_; > On 2015/08/04 06:56:46, hlundin-webrtc wrote: > > Please, add GUARDED_BY(video_sync_critsect_) on the member variables that are > > protected by this lock. It is provided by webrtc/base/thread_annotations.h. > > Done. LGTM
https://codereview.webrtc.org/1263223002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:3627: unsigned int playout_timestamp_rtp; Nits: use uint32_t (type of the member we're assigning from, and init to zero (someone will likely this code around at a later point)
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1263223002/#ps100001 (title: "Responding to comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1263223002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1263223002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/320)
On 2015/08/05 17:32:19, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/320) It looks like I need an LGTM from an owner (Henrik, Niklas or Shijing); who would you suggest?
On 2015/08/05 17:46:41, Taylor Brandstetter wrote: > On 2015/08/05 17:32:19, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > presubmit on tryserver.webrtc (JOB_FAILED, > > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/320) > > It looks like I need an LGTM from an owner (Henrik, Niklas or Shijing); who > would you suggest? Ah, sorry, should've noticed and told you that. henrikg@ is back from vacation.
solenberg@webrtc.org changed reviewers: + henrikg@webrtc.org
LGTM. I think you should add to the CL description that a function is removed and the audio coding version is used instead. https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:3924: CriticalSectionScoped cs(video_sync_lock_.get()); A bit hard to see the diff when moved :-/ but it afaict the lock is the only change. Should playout_timestamp_rtcp_ also be protected everywhere as here? https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:278: return audio_coding_->LeastRequiredDelayMs(); Move to implementation file. https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:522: uint32_t playout_timestamp_rtcp_; Protect also?
On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > LGTM. > > I think you should add to the CL description that a function is removed and the > audio coding version is used instead. > > https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:3924: CriticalSectionScoped > cs(video_sync_lock_.get()); > A bit hard to see the diff when moved :-/ but it afaict the lock is the only > change. Should playout_timestamp_rtcp_ also be protected everywhere as here? > > https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.h (right): > > https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.h:278: return audio_coding_->LeastRequiredDelayMs(); > Move to implementation file. > > https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.h:522: uint32_t playout_timestamp_rtcp_; > Protect also? Oh, I should say that I never got a mail but I saw I was added as reviewer so I went ahead. (Fredrik asked at some time before if I could.)
Thanks Henrik. My bad for not mailing you earlier; I'm still getting used to the system... https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:3924: CriticalSectionScoped cs(video_sync_lock_.get()); On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > A bit hard to see the diff when moved :-/ but it afaict the lock is the only > change. Should playout_timestamp_rtcp_ also be protected everywhere as here? Yes, the locking is the only change. As I said in my other comment, I don't think playout_timestamp_rtcp_ needs to be protected; this just seemed like the simplest way to protect both playout_timestamp_rtp_ and playout_delay_ms_ within one lock acquisition. https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:278: return audio_coding_->LeastRequiredDelayMs(); On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > Move to implementation file. Done. https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:522: uint32_t playout_timestamp_rtcp_; On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > Protect also? I don't think it needs to be protected (at least not for the same reason as playout_timestamp_rtp_), since it's not accessed by GetPlayoutTimestamp.
lgtm https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:3924: CriticalSectionScoped cs(video_sync_lock_.get()); On 2015/08/12 19:49:04, Taylor Brandstetter wrote: > On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > > A bit hard to see the diff when moved :-/ but it afaict the lock is the only > > change. Should playout_timestamp_rtcp_ also be protected everywhere as here? > > Yes, the locking is the only change. As I said in my other comment, I don't > think playout_timestamp_rtcp_ needs to be protected; this just seemed like the > simplest way to protect both playout_timestamp_rtp_ and playout_delay_ms_ within > one lock acquisition. Acknowledged. https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1263223002/diff/100001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:522: uint32_t playout_timestamp_rtcp_; On 2015/08/12 19:49:04, Taylor Brandstetter wrote: > On 2015/08/12 15:03:22, Henrik Grunell (webrtc) wrote: > > Protect also? > > I don't think it needs to be protected (at least not for the same reason as > playout_timestamp_rtp_), since it's not accessed by GetPlayoutTimestamp. Acknowledged.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1263223002/#ps120001 (title: "Moved LeastRequiredDelayMs definition to cc file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1263223002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1263223002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/743758816853df2040a21c5652b0d0e238b1512f Cr-Commit-Position: refs/heads/master@{#9706} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
