Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1987663002:
android: Don't do a thread jump for incoming frames. (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't do a thread jump for incoming frames.
We're now supposed to accept incoming frames from any thread.
BUG=webrtc:5902
Committed: https://crrev.com/c82d0902e1f1e902223df3452f9a2a3b7fdb71f4
Cr-Commit-Position: refs/heads/master@{#12844}
Patch Set 1 #Patch Set 2 : Take capturer_lock_. #
Total comments: 7
Patch Set 3 : Delete thread check in AndroidVideoCapturer::OnIncomingFrame. #Patch Set 4 : Delete VideoBroadcaster thread checker #
Total comments: 5
Patch Set 5 : Revert most of VideoBroadcaster change. Check for NULL capturer_.' #Patch Set 6 : Clarified comment. #
Total comments: 2
Messages
Total messages: 33 (9 generated)
nisse@webrtc.org changed reviewers: + perkj@webrtc.org
This is a somewhat speculative cl, it will be interesting to see if anything breaks. The video engine should accept frames on any thread, and all state attached to the video capturer, in particular, the video adapter, should have their own locks.
lgtm
On 2016/05/17 12:33:28, perkj_webrtc wrote: > lgtm Can we file a bug for this to track this?
On 2016/05/17 12:33:56, perkj_webrtc wrote: > Can we file a bug for this to track this? I've searched for pbos' changes which enabled frames on any thread, hoping to find some appropriate bug. But I couldn't find any. pbos, do you know if there were any bug tracking that change?
On 2016/05/17 14:02:25, nisse-webrtc wrote: > On 2016/05/17 12:33:56, perkj_webrtc wrote: > > > Can we file a bug for this to track this? > > I've searched for pbos' changes which enabled frames on any thread, > hoping to find some appropriate bug. But I couldn't find any. > > pbos, do you know if there were any bug tracking that change? Nah, it was more of a side effect of https://bugs.chromium.org/p/webrtc/issues/detail?id=5410#c7 and waaay old refactoring.
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); Any risk of this deadlocking?
Description was changed from ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG= ========== to ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG=webrtc:5902 ==========
Patch set 3 crashes in the thread checks in VideoBroadcaster. Does that thread checker still serve any purpose? As far as I can see, everything is syncronized using the sinks_and_wants_lock_. https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/17 14:26:06, pbos-webrtc wrote: > Any risk of this deadlocking? No idea... The capturer_ pointer is declared with a GUARDED_BY annotation, but I'm not sure what this lock is intended to protect. Maybe a race with the Stop method (but in that case, there ought to be a check that capturer_ != nullptr, after taking the lock.
nisse@webrtc.org changed reviewers: + magjed@webrtc.org
Magnus, can you explain the use of capturer_lock_? Is it still needed? https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/18 07:44:01, nisse-webrtc wrote: > On 2016/05/17 14:26:06, pbos-webrtc wrote: > > Any risk of this deadlocking? > > No idea... The capturer_ pointer is declared with a GUARDED_BY annotation, but > I'm not sure what this lock is intended to protect. > Maybe a race with the Stop method (but in that case, there ought to be a check > that capturer_ != nullptr, after taking the lock. It seems capturer_lock_ was introduced in cl https://codereview.webrtc.org/1307973002 Magnus, can you explain why the lock was needed? Anyway, I'm a bit weary of removing it in this cl, since the videoframefactory used is not thread safe (in particular, UpdateCapturedFrame). Or if all frames arrive on the same thread, that wouldn't be a problem?
https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/18 12:28:03, nisse-webrtc wrote: > On 2016/05/18 07:44:01, nisse-webrtc wrote: > > On 2016/05/17 14:26:06, pbos-webrtc wrote: > > > Any risk of this deadlocking? > > > > No idea... The capturer_ pointer is declared with a GUARDED_BY annotation, but > > I'm not sure what this lock is intended to protect. > > Maybe a race with the Stop method (but in that case, there ought to be a check > > that capturer_ != nullptr, after taking the lock. > > It seems capturer_lock_ was introduced in cl > https://codereview.webrtc.org/1307973002 > > Magnus, can you explain why the lock was needed? > > Anyway, I'm a bit weary of removing it in this cl, since the videoframefactory > used is not thread safe (in particular, UpdateCapturedFrame). Or if all frames > arrive on the same thread, that wouldn't be a problem? It was/is used to protect against callbacks from the Java capturer after AndroidVideoCapturerJni::Stop is called. I don't think you can remove it - how will you handle e.g. pending nativeOnTextureFrameCaptured() when Stop is called? https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/androidvideoca... File webrtc/api/androidvideocapturer.cc (right): https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/androidvideoca... webrtc/api/androidvideocapturer.cc:183: // NOTE: Calls to this method may come on any thread, but they are They come from the Java thread. It's the same thread every time, if that helps.
https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:186: capturer_->OnIncomingFrame(buffer, rotation, timestamp_ns); You have introduced a race here. This code might be executed after Stop(), so you need to check if |capturer_| is null before calling functions on it.
https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/18 15:12:50, magjed_webrtc wrote: > On 2016/05/18 12:28:03, nisse-webrtc wrote: > > On 2016/05/18 07:44:01, nisse-webrtc wrote: > > > On 2016/05/17 14:26:06, pbos-webrtc wrote: > > > > Any risk of this deadlocking? > > > > > > No idea... The capturer_ pointer is declared with a GUARDED_BY annotation, > but > > > I'm not sure what this lock is intended to protect. > > > Maybe a race with the Stop method (but in that case, there ought to be a > check > > > that capturer_ != nullptr, after taking the lock. > > > > It seems capturer_lock_ was introduced in cl > > https://codereview.webrtc.org/1307973002 > > > > Magnus, can you explain why the lock was needed? > > > > Anyway, I'm a bit weary of removing it in this cl, since the videoframefactory > > used is not thread safe (in particular, UpdateCapturedFrame). Or if all frames > > arrive on the same thread, that wouldn't be a problem? > > It was/is used to protect against callbacks from the Java capturer after > AndroidVideoCapturerJni::Stop is called. I don't think you can remove it - how > will you handle e.g. pending nativeOnTextureFrameCaptured() when Stop is called? Don't we need something like rtc::CritScope cs(&capturer_lock_); if (!capturer_) { ... frame arrive when stopped... ; return; } I don't seen any such checks. Or is the opposite race, Stop shouldn't return until after On..Frame is completed? I'm still a bit confused. https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/androidvideoca... File webrtc/api/androidvideocapturer.cc (right): https://codereview.webrtc.org/1987663002/diff/60001/webrtc/api/androidvideoca... webrtc/api/androidvideocapturer.cc:183: // NOTE: Calls to this method may come on any thread, but they are On 2016/05/18 15:12:50, magjed_webrtc wrote: > They come from the Java thread. It's the same thread every time, if that helps. Should be ok, then. Would it make sense with a thread checker to back that up?
https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/18 15:31:31, nisse-webrtc wrote: > On 2016/05/18 15:12:50, magjed_webrtc wrote: > > On 2016/05/18 12:28:03, nisse-webrtc wrote: > > > On 2016/05/18 07:44:01, nisse-webrtc wrote: > > > > On 2016/05/17 14:26:06, pbos-webrtc wrote: > > > > > Any risk of this deadlocking? > > > > > > > > No idea... The capturer_ pointer is declared with a GUARDED_BY annotation, > > but > > > > I'm not sure what this lock is intended to protect. > > > > Maybe a race with the Stop method (but in that case, there ought to be a > > check > > > > that capturer_ != nullptr, after taking the lock. > > > > > > It seems capturer_lock_ was introduced in cl > > > https://codereview.webrtc.org/1307973002 > > > > > > Magnus, can you explain why the lock was needed? > > > > > > Anyway, I'm a bit weary of removing it in this cl, since the > videoframefactory > > > used is not thread safe (in particular, UpdateCapturedFrame). Or if all > frames > > > arrive on the same thread, that wouldn't be a problem? > > > > It was/is used to protect against callbacks from the Java capturer after > > AndroidVideoCapturerJni::Stop is called. I don't think you can remove it - how > > will you handle e.g. pending nativeOnTextureFrameCaptured() when Stop is > called? > > Don't we need something like > > rtc::CritScope cs(&capturer_lock_); > if (!capturer_) > { ... frame arrive when stopped... ; return; } > > I don't seen any such checks. Or is the opposite race, Stop shouldn't return > until after On..Frame is completed? I'm still a bit confused. We have exactly such a check in AsyncCapturerInvoke: AsyncCapturerInvoke(...) { rtc::CritScope cs(&capturer_lock_); if (!invoker_) { LOG(LS_WARNING) << method_name << "() called for closed capturer."; return; } invoker_->AsyncInvoke<void>(rtc::Bind(method, capturer_, args...)); } You have removed the thread jump by removing the use of AsyncCapturerInvoke in these functions, but you still need the check.
https://codereview.webrtc.org/1987663002/diff/60001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (left): https://codereview.webrtc.org/1987663002/diff/60001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:43: RTC_DCHECK(thread_checker_.CalledOnValidThread()); please only remove the thread check from this method. Not the other.
https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope cs(&capturer_lock_); On 2016/05/18 15:51:28, magjed_webrtc wrote: > On 2016/05/18 15:31:31, nisse-webrtc wrote: > > On 2016/05/18 15:12:50, magjed_webrtc wrote: > > > On 2016/05/18 12:28:03, nisse-webrtc wrote: > > > > On 2016/05/18 07:44:01, nisse-webrtc wrote: > > > > > On 2016/05/17 14:26:06, pbos-webrtc wrote: > > > > > > Any risk of this deadlocking? > > > > > > > > > > No idea... The capturer_ pointer is declared with a GUARDED_BY > annotation, > > > but > > > > > I'm not sure what this lock is intended to protect. > > > > > Maybe a race with the Stop method (but in that case, there ought to be a > > > check > > > > > that capturer_ != nullptr, after taking the lock. > > > > > > > > It seems capturer_lock_ was introduced in cl > > > > https://codereview.webrtc.org/1307973002 > > > > > > > > Magnus, can you explain why the lock was needed? > > > > > > > > Anyway, I'm a bit weary of removing it in this cl, since the > > videoframefactory > > > > used is not thread safe (in particular, UpdateCapturedFrame). Or if all > > frames > > > > arrive on the same thread, that wouldn't be a problem? > > > > > > It was/is used to protect against callbacks from the Java capturer after > > > AndroidVideoCapturerJni::Stop is called. I don't think you can remove it - > how > > > will you handle e.g. pending nativeOnTextureFrameCaptured() when Stop is > > called? > > > > Don't we need something like > > > > rtc::CritScope cs(&capturer_lock_); > > if (!capturer_) > > { ... frame arrive when stopped... ; return; } > > > > I don't seen any such checks. Or is the opposite race, Stop shouldn't return > > until after On..Frame is completed? I'm still a bit confused. > > We have exactly such a check in AsyncCapturerInvoke: > AsyncCapturerInvoke(...) { > rtc::CritScope cs(&capturer_lock_); > if (!invoker_) { > LOG(LS_WARNING) << method_name << "() called for closed capturer."; > return; > } > invoker_->AsyncInvoke<void>(rtc::Bind(method, capturer_, args...)); > } > You have removed the thread jump by removing the use of AsyncCapturerInvoke in > these functions, but you still need the check. Done. https://codereview.webrtc.org/1987663002/diff/60001/webrtc/media/base/videobr... File webrtc/media/base/videobroadcaster.cc (left): https://codereview.webrtc.org/1987663002/diff/60001/webrtc/media/base/videobr... webrtc/media/base/videobroadcaster.cc:43: RTC_DCHECK(thread_checker_.CalledOnValidThread()); On 2016/05/18 16:43:24, perkj_webrtc wrote: > please only remove the thread check from this method. Not the other. Done.
lgtm
lgtm
On 2016/05/17 14:26:06, pbos-webrtc wrote: > https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... > File webrtc/api/java/jni/androidvideocapturer_jni.cc (right): > > https://codereview.webrtc.org/1987663002/diff/20001/webrtc/api/java/jni/andro... > webrtc/api/java/jni/androidvideocapturer_jni.cc:197: rtc::CritScope > cs(&capturer_lock_); > Any risk of this deadlocking? Not really sure. This is the java camera thread. Not sure if it holds any other locks when getting here, or if the video pipeline has any reason to compete for any such locks.
lgtm, really hoping we're not putting a deadlock in here :) https://codereview.webrtc.org/1987663002/diff/100001/webrtc/api/androidvideoc... File webrtc/api/androidvideocapturer.cc (right): https://codereview.webrtc.org/1987663002/diff/100001/webrtc/api/androidvideoc... webrtc/api/androidvideocapturer.cc:183: // NOTE: The frame_factory hack isn't thread safe. It works because Should there be a TODO here?
I'm trying to land this... Have a nice weekend! https://codereview.webrtc.org/1987663002/diff/100001/webrtc/api/androidvideoc... File webrtc/api/androidvideocapturer.cc (right): https://codereview.webrtc.org/1987663002/diff/100001/webrtc/api/androidvideoc... webrtc/api/androidvideocapturer.cc:183: // NOTE: The frame_factory hack isn't thread safe. It works because On 2016/05/20 13:22:47, pbos-webrtc wrote: > Should there be a TODO here? I have a different cl which deletes that frame factory. And we all know that, so I don't think a TODO comment is needed. I really hope to land that deletion next week.
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987663002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987663002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nisse@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1987663002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1987663002/100001
Message was sent while issue was closed.
Description was changed from ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG=webrtc:5902 ========== to ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG=webrtc:5902 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG=webrtc:5902 ========== to ========== Don't do a thread jump for incoming frames. We're now supposed to accept incoming frames from any thread. BUG=webrtc:5902 Committed: https://crrev.com/c82d0902e1f1e902223df3452f9a2a3b7fdb71f4 Cr-Commit-Position: refs/heads/master@{#12844} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c82d0902e1f1e902223df3452f9a2a3b7fdb71f4 Cr-Commit-Position: refs/heads/master@{#12844} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
