Samsung / GearVRf

The GearVR framework(GearVRf) is an Open Source VR rendering library for application development on VR-supported Android devices.
http://www.gearvrf.org
Apache License 2.0
407 stars 217 forks source link

Fix UI Thread deadlock #1973

Closed ragner closed 6 years ago

ragner commented 6 years ago

This PR should fix issue #1880

GearVRf-DCO-1.0-Signed-off-by: Ragner Magalhaes ragner.n@samsung.com

ragner commented 6 years ago

@liaxim, I can reproduce the same issue everywhere I call getId() at the UIThread!

I am using Android 8.0 and Monoscopic mode backend.

I could implement some GVRSurfateTexture(extending GVRExternalTexture) to handle this issue and and replace all use of SufaceTexture/Surface at the framekwork by this GVRSurfateTexture with some improvements to update the oes texture.

How does it sound for you?

ragner commented 6 years ago

Other possible fix is add support to GVR Texture to return its texture id asynchronously

NolaDonato commented 6 years ago

The texture I'D is not platform independent. Any code that fetches it will not work on Vulkan

liaxim commented 6 years ago

There are a lot of things that do not work with Vulkan. The id is needed for the Android Views support which is kind of important. The relevant Android APIs work with GL texture ids. The method could be better hidden though.

ragner commented 6 years ago

Hi all, do you mean we should not use OES external texture to render videos and views anymore?

liaxim commented 6 years ago

Not at all. Eventually the Vulkan support for these will be there.

liaxim commented 6 years ago

@ragner Do you think you could devise a test case which shows the deadlock and can be used for verification?

liaxim commented 6 years ago

gvr-events crashes for me on Note 8:

09-05 17:40:48.895  1296  1296 E AndroidRuntime: FATAL EXCEPTION: main
09-05 17:40:48.895  1296  1296 E AndroidRuntime: Process: org.gearvrf.events, PID: 1296
09-05 17:40:48.895  1296  1296 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke virtual method 'android.graphics.Canvas android.view.Surface.lockCanvas(android.graphics.Rect)' on a null object reference
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at org.gearvrf.scene_objects.GVRViewSceneObject$RootViewGroup.dispatchDraw(GVRViewSceneObject.java:641)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.View.updateDisplayListIfDirty(View.java:17532)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.View.draw(View.java:18321)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.ViewGroup.drawChild(ViewGroup.java:3946)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.ViewGroup.dispatchDraw(ViewGroup.java:3732)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.View.draw(View.java:18562)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at com.android.internal.policy.DecorView.draw(DecorView.java:986)
09-05 17:40:48.895  1296  1296 E AndroidRuntime:    at android.view.View.updateDisplayListIfDirty(View.java:17537)
liaxim commented 6 years ago

The code seems to have grown to be quite complicated. I'd say the very first thing we need is the external texture id. Since this is all internal stuff, we could add a special ExternalTexture factory that the ViewSO calls on the rendering thread from its ctor (runOnGlThread). This special factory would really create the GL texture object and immediately return the id (bypass the problematic getId method). After this call succeeds then we can proceed with the Android view stuff on the Android UI thread. It seems to me this has the potential to be less complex and easier to follow.

In pseudo code:

ViewSO.ctor {
    runOnGlThread(
        id = createExternalTextureNow()
        runOnUiThread( addView( id ) )
    )
}
liaxim commented 6 years ago

Fyi: using the monoscopic backend with the gvr-sceneobjects deadlocks on the getId method (master). I guess it is form of a test.

ragner commented 6 years ago

Hi @liaxim, I will improve this PR according to your review. Thanks

ragner commented 6 years ago

Hi @liaxim, please refer to https://github.com/gearvrf/GearVRf-Demos/pull/644 to test my last improvements.

ragner commented 6 years ago

it is ready to review.

liaxim commented 6 years ago

Extensions/WidgetPlugin doesn't compile for me.

liaxim commented 6 years ago

gvr-sceneobjects with monoscopic backend works! Also code looks cleaner. Thank you.

ragner commented 6 years ago

@liaxim The compile error on extensions was done!

liaxim commented 6 years ago

gvr-renderableview - the fps is much lower. Note8 without pr - solid 60fps. Note8 with pr - in the 30-40fps range.

ragner commented 6 years ago

Hi @liaxim, I tested with my Note8 and gvr-renderableview was rendering at 60fps. To try to reproduce your issue I duplicated the number of views and got 45+fps. Your issue may be related to the texture buffers size that I changed to be the view's size by default. So I configured the texture buffer size of gvr-renderableview to fix the anti-aliasing according to the display rendering area.

You need fetch the bugfix_viewdeadlock branch at demos' repository.

Thank you!

liaxim commented 6 years ago

Looks good. Planning to merge on Friday.