Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389463002:
[Remoting Android] Separate the display core from JniGlDisplayHandler (Closed)
|
Created:
4 years, 2 months ago by Yuwei Modified:
4 years, 2 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Remoting Android] Separate the display core from JniGlDisplayHandler
This CL separates the core that lives entirely on the display thread from
JniGlDisplayHandler so that the thread model can be clearer.
BUG=646116
Committed: https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da
Cr-Commit-Position: refs/heads/master@{#422988}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add DCHECKs and release native window when destroy #Patch Set 3 : Lint #
Total comments: 9
Patch Set 4 : Resolve some feedbacks #Patch Set 5 : Add override for ~Core() #
Messages
Total messages: 22 (8 generated)
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL. Thanks! https://codereview.chromium.org/2389463002/diff/1/remoting/client/jni/jni_gl_... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2389463002/diff/1/remoting/client/jni/jni_gl_... remoting/client/jni/jni_gl_display_handler.cc:123: ANativeWindow* window = ANativeWindow_fromSurface( It's unsure whether eglCreateWindowSurface acquires a reference to the native window so I think we can release it not earlier than SurfaceDestroyed() just to be safe. https://codereview.chromium.org/2389463002/diff/1/remoting/client/jni/jni_gl_... File remoting/client/jni/jni_gl_display_handler.h (left): https://codereview.chromium.org/2389463002/diff/1/remoting/client/jni/jni_gl_... remoting/client/jni/jni_gl_display_handler.h:45: void Initialize(const base::android::JavaRef<jobject>& java_client); Merged Initialize with the constructor since it doesn't look useful.
https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:106: DCHECK(!frame_consumer_); DCHECK here that this function is called on UI thread https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:112: frame_consumer_ = consumer->GetWeakPtr(); frame_consumer_ is set on UI thread, but then it's used on display thread in SurfaceCreated(). Is there anything to ensure there is no race? https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:143: ANativeWindow_release(window_); previously window was released in SurfaceCreatedOnDisplayThread(). Is there any reason this window can't be released in Core::SurfaceCreated()? Then it should be possible to remove window_
https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:106: DCHECK(!frame_consumer_); On 2016/10/03 17:33:44, Sergey Ulanov wrote: > DCHECK here that this function is called on UI thread I think calling on any thread may be fine as long as it is only called once at the beginning? https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:112: frame_consumer_ = consumer->GetWeakPtr(); On 2016/10/03 17:33:44, Sergey Ulanov wrote: > frame_consumer_ is set on UI thread, but then it's used on display thread in > SurfaceCreated(). Is there anything to ensure there is no race? I think it's mostly fine since it is only called once before SurfaceCreated(). Alternatively I can create the consumer in ctor and provide a function for transferring the ownership of the consumer to the caller. https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:143: ANativeWindow_release(window_); On 2016/10/03 17:33:44, Sergey Ulanov wrote: > previously window was released in SurfaceCreatedOnDisplayThread(). Is there any > reason this window can't be released in Core::SurfaceCreated()? Then it should > be possible to remove window_ Looks likes releasing window_ right after eglCreateWindowSurface still work. Since the Android specific behavior of eglCreateWindowSurface is not well documented, my concern is if eglCreateWindowSurface doesn't acquire a reference to |window_| then |window_| might be destroyed before |egl_context_| is detached once the view is destroyed on the UI thread. I remembered that I saw warning saying failed to swap buffer when the surface is being destroyed, couldn't repro it with latest build on Android N though... I also did a code search on ANativeWindow_release and looks like most people will keep the window until some destroy function is called. I'm okay with releasing the window right after eglCreateWindowSurface as long as it doesn't crash the app.
https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:112: frame_consumer_ = consumer->GetWeakPtr(); On 2016/10/03 19:03:39, Yuwei wrote: > On 2016/10/03 17:33:44, Sergey Ulanov wrote: > > frame_consumer_ is set on UI thread, but then it's used on display thread in > > SurfaceCreated(). Is there anything to ensure there is no race? > > I think it's mostly fine since it is only called once before SurfaceCreated(). Problem is that there is nothing in this class to verify that this function is called before SurfaceCreated(). Maybe add DCHECK in JniGlDisplayHandler::OnSurfaceCreated() to verify that it's called after CreateFrameConsumer() > > Alternatively I can create the consumer in ctor and provide a function for > transferring the ownership of the consumer to the caller. That would work as well.
Resolved some comments. PTAL https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:106: DCHECK(!frame_consumer_); On 2016/10/03 17:33:44, Sergey Ulanov wrote: > DCHECK here that this function is called on UI thread Added comment about threading. https://codereview.chromium.org/2389463002/diff/40001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:112: frame_consumer_ = consumer->GetWeakPtr(); On 2016/10/03 19:42:10, Sergey Ulanov wrote: > On 2016/10/03 19:03:39, Yuwei wrote: > > On 2016/10/03 17:33:44, Sergey Ulanov wrote: > > > frame_consumer_ is set on UI thread, but then it's used on display thread in > > > SurfaceCreated(). Is there anything to ensure there is no race? > > > > I think it's mostly fine since it is only called once before SurfaceCreated(). > > Problem is that there is nothing in this class to verify that this function is > called before SurfaceCreated(). Acknowledged. > Maybe add DCHECK in JniGlDisplayHandler::OnSurfaceCreated() to verify that it's > called after CreateFrameConsumer() I think it's not necessary now. |frame_consumer_| is now set inside the ctor so the caller can grab the ownership any time they want. > > Alternatively I can create the consumer in ctor and provide a function for > > transferring the ownership of the consumer to the caller. > > That would work as well. Done.
lgtm
On 2016/10/04 20:13:43, Sergey Ulanov wrote: > lgtm Thanks!
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2389463002/#ps80001 (title: "Add override for ~Core()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Separate the display core from JniGlDisplayHandler This CL separates the core that lives entirely on the display thread from JniGlDisplayHandler so that the thread model can be clearer. BUG=646116 ========== to ========== [Remoting Android] Separate the display core from JniGlDisplayHandler This CL separates the core that lives entirely on the display thread from JniGlDisplayHandler so that the thread model can be clearer. BUG=646116 Committed: https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da Cr-Commit-Position: refs/heads/master@{#422988} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da Cr-Commit-Position: refs/heads/master@{#422988} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
