Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339573002:
Android SurfaceViewRenderer: Create EGL context on render thread (Closed)
|
Created:
4 years, 3 months ago by magjed_webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid SurfaceViewRenderer: Create EGL context on render thread
This CL avoids eglMakeCurrent failing on some problematic Marvel based
Jelly Bean devices.
BUG=webrtc:6350
R=perkj@webrtc.org, sakal@webrtc.org
Committed: https://crrev.com/4ed5b9f62d7850fc5d9b2a006d3c368a9183c16d
Cr-Commit-Position: refs/heads/master@{#14234}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add link to bug in code comment #Patch Set 3 : Rebase #Messages
Total messages: 14 (7 generated)
Description was changed from ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 ========== to ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 ==========
magjed@webrtc.org changed reviewers: + perkj@webrtc.org, sakal@webrtc.org
Please take a look.
nice- lgtm https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:163: // some Marvel based JB devices. nit: link comment to bug. https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:167: eglBase = EglBase.create(sharedContext, configAttributes); ok- so previously we created it on this thread and released it on the render thread. This lgtm
lgtm https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:164: ThreadUtils.invokeAtFrontUninterruptibly(renderThreadHandler, new Runnable() { Invoking is safer but do we need to do that? I think all access to the eglBase is done on the rendering thread so it should be safe to post?
https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... File webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java (right): https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:163: // some Marvel based JB devices. On 2016/09/14 07:02:05, perkj_webrtc wrote: > nit: link comment to bug. Done. https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:164: ThreadUtils.invokeAtFrontUninterruptibly(renderThreadHandler, new Runnable() { On 2016/09/14 07:10:05, sakal wrote: > Invoking is safer but do we need to do that? I think all access to the eglBase > is done on the rendering thread so it should be safe to post? I'm worried about release() being called immediately after init(). In release() we postAtFrontOfQueue(), so there will be a race between that and the post from init(). If release() is executed before init(), we will currently fail with null pointer exception on EglBase. I could add a check for that, but init() will still be called after release() which means that we leak the EglBase. I would like to land this fix as simple as possible, and the current fix is as simple as it can be I think. I can change this class to always post normally, i.e. not at front of queue, in a follow-up CL, to preserve the fifo order. Then I can remove the invoke. https://codereview.webrtc.org/2339573002/diff/1/webrtc/api/android/java/src/o... webrtc/api/android/java/src/org/webrtc/SurfaceViewRenderer.java:167: eglBase = EglBase.create(sharedContext, configAttributes); On 2016/09/14 07:02:05, perkj_webrtc wrote: > ok- so previously we created it on this thread and released it on the render > thread. > This lgtm Yes, which should be fine according to the documentation, but doesn't work in practice on all devices.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2339573002/#ps20001 (title: "Add link to bug in code comment")
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_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_msan on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 ========== to ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 R=perkj@webrtc.org, sakal@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/4ed5b9f62d7850fc5d9b2a006... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 4ed5b9f62d7850fc5d9b2a006d3c368a9183c16d (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 R=perkj@webrtc.org, sakal@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/4ed5b9f62d7850fc5d9b2a006... ========== to ========== Android SurfaceViewRenderer: Create EGL context on render thread This CL avoids eglMakeCurrent failing on some problematic Marvel based Jelly Bean devices. BUG=webrtc:6350 R=perkj@webrtc.org, sakal@webrtc.org Committed: https://crrev.com/4ed5b9f62d7850fc5d9b2a006d3c368a9183c16d Cr-Commit-Position: refs/heads/master@{#14234} ========== |
