Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2111933002:
Android SurfaceViewRenderer: Fix deadlock (Closed)
|
Created:
4 years, 5 months ago by magjed_webrtc Modified:
4 years, 5 months ago Reviewers:
sakal 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. |
DescriptionAndroid SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
R=sakal@webrtc.org
Committed: https://crrev.com/897d932e0b0b81b81393316692d8687e5975a62a
Cr-Commit-Position: refs/heads/master@{#13364}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressing samis comments #Messages
Total messages: 15 (8 generated)
Description was changed from
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
==========
to
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
==========
magjed@webrtc.org changed reviewers: + sakal@webrtc.org
Sami - Please take a look.
https://codereview.webrtc.org/2111933002/diff/1/webrtc/api/java/android/org/w... File webrtc/api/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2111933002/diff/1/webrtc/api/java/android/org/w... webrtc/api/java/android/org/webrtc/SurfaceViewRenderer.java:291: pendingFrame = frame; If renderThreadHandler is null, pendingFrame will still be set. This didn't happen earlier. If renderThreadHandler is null, renderFrameDone will be called on the frame. However, it can be called again by renderFrameOnRenderThread, release or renderFrame. This can lead to renderFrameDone being called twice on the same frame. I am not sure how serious this is but I'd like it not being possible to happen. updateFrameDimensionsAndReportEvents is also called when not initialized even though it wasn't called earlier. I don't think this will cause any problems though.
https://codereview.webrtc.org/2111933002/diff/1/webrtc/api/java/android/org/w... File webrtc/api/java/android/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2111933002/diff/1/webrtc/api/java/android/org/w... webrtc/api/java/android/org/webrtc/SurfaceViewRenderer.java:291: pendingFrame = frame; On 2016/07/01 08:02:26, sakal wrote: > If renderThreadHandler is null, pendingFrame will still be set. This didn't > happen earlier. If renderThreadHandler is null, renderFrameDone will be called > on the frame. However, it can be called again by renderFrameOnRenderThread, > release or renderFrame. This can lead to renderFrameDone being called twice on > the same frame. I am not sure how serious this is but I'd like it not being > possible to happen. Good catch. renderFrameDone() will call into JNI and decrement a ref count, so it's paramount it's called exactly once per frame. I reverted most of these changes, and only moved updateFrameDimensionsAndReportEvents().
The CQ bit was checked by sakal@webrtc.org
The CQ bit was unchecked by sakal@webrtc.org
lgtm
The CQ bit was checked by magjed@webrtc.org
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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14732)
Description was changed from
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
==========
to
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
R=sakal@webrtc.org
Committed:
https://chromium.googlesource.com/external/webrtc/+/897d932e0b0b81b8139331669...
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 897d932e0b0b81b81393316692d8687e5975a62a (presubmit successful).
Message was sent while issue was closed.
Description was changed from
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
R=sakal@webrtc.org
Committed:
https://chromium.googlesource.com/external/webrtc/+/897d932e0b0b81b8139331669...
==========
to
==========
Android SurfaceViewRenderer: Fix deadlock
Deadlock caused by two methods grabbing two locks in the opposite order:
renderFrame():
handlerLock
layoutLock
onMeasure():
layoutLock
handlerLock
This CL removs the nested locking to fix the deadlock and make it less
error prone for the future.
BUG=webrtc:6003
R=sakal@webrtc.org
Committed: https://crrev.com/897d932e0b0b81b81393316692d8687e5975a62a
Cr-Commit-Position: refs/heads/master@{#13364}
==========
|
