1136364 - Remove EGLSurface creation in GLController

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Almost all of the stuff in GLController is there because we needed to call eglCreateWindowSurface from Java, as we had to pass the result of SurfaceView.getHolder(). We have a native call to do this now, so I think a lot of the config choosing, etc, can be avoided and handled in GLContextProviderEGL.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Incomplete patch

snorp, here's what I have at the moment.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch

This moves EGLSurface creation to native code and kills off EGL-related code in GLController.java.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8746235 [details] [diff] [review]
Proposed patch

Review of attachment 8746235 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nits here, but I don't understand  why you want to return null in GLController.getSurface(), so r- until we figure that out :)

::: gfx/gl/moz.build
@@ +93,5 @@
>      LOCAL_INCLUDES += ['/widget/gonk']
>      LOCAL_INCLUDES += ['%' + '%s/%s' % (CONFIG['ANDROID_SOURCE'], 'hardware/libhardware/include')]
>  
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'android':
> +    LOCAL_INCLUDES += ['/widget/android']

I don't think you need this because AndroidBridge is exported.

::: mobile/android/base/java/org/mozilla/gecko/gfx/GLController.java
@@ +27,4 @@
>      private static final String LOGTAG = "GeckoGLController";
>  
>      /* package */ LayerView mView;
>      private boolean mServerSurfaceValid;

Do we still need this?

@@ +80,5 @@
>          // in turn will synchronize with the compositor thread.
>          if (mCompositorCreated) {
>              pauseCompositor();
>          }
> +        return;

Looks like you forgot to remove this

@@ +102,5 @@
>              public void run() {
>                  updateCompositor();
>              }
>          });
> +        return;

Same

@@ +123,5 @@
>          // Android doesn't have a chance to destroy our surface in between.
>          if (mView.getLayerClient().isGeckoReady()) {
>              createCompositor(mWidth, mHeight);
>          }
> +        return;

Same

@@ +139,5 @@
>  
> +    @WrapForJNI(allowMultithread = true)
> +    private synchronized Object getSurface() {
> +        compositorCreated();
> +        if (mServerSurfaceChanged) {

I don't think I understand the point of those. Why would we ever want to return a null surface here?

::: widget/android/nsWindow.cpp
@@ +956,5 @@
>      {
>          return mCompositorPaused;
>      }
>  
> +    void* getSurface()

GetSurface()

@@ +961,2 @@
>      {
> +        auto tempSurface = mGLController->GetSurface();

Just 'surface' will suffice I think

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Fixed all the nits I think.

Returning null from getSurface sometimes was to address a stale reference problem I was having, but switching mSurface to a GlobalRef as I did here is a better solution I think.

I'm pretty sure mServerSurfaceValid is still necessary for now, it gets used by LayerView to determine what to do in onSizeChanged.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8746789 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8746789 [details] [diff] [review]:
-----------------------------------------------------------------

The nsWindow::GetSurface() thing worries me. Things should work if you just remove the guard in there. If it doesn't, then there is another issue that will need fixed instead.

::: mobile/android/base/java/org/mozilla/gecko/gfx/GLController.java
@@ +137,2 @@
>          compositorCreated();
> +        return mView.getSurface();

Check if mView is null here

::: widget/android/nsWindow.cpp
@@ +933,5 @@
>                          const GLController::LocalRef& aInstance)
>          : window(*aWindow)
>          , mGLController(aInstance)
>          , mCompositorPaused(true)
> +        , mSurface(nullptr)

The GlobalRef should do this for you, you don't need to explicitly init to nullptr

@@ +961,3 @@
>      {
> +        auto surface = mGLController->GetSurface();
> +        if (surface) {

I meant to comment on this last time but evidently missed it. If mSurface is supposed to be a cache of the currently-used surface we should set mSurface to null here too. Otherwise, if we get null from mGLController->GetSurface(), anyone that uses mSurface is using a stale (and probably invalid) surface. If nobody uses it then we don't need the cache anyway.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Should address the above

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8747103 [details] [diff] [review]
Proposed patch (updated)

Review of attachment 8747103 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +848,5 @@
>      nsWindow& window;
>      GLController::GlobalRef mGLController;
>      GeckoLayerClient::GlobalRef mLayerClient;
>      Atomic<bool, ReleaseAcquire> mCompositorPaused;
> +    mozilla::jni::GlobalRef<mozilla::jni::Object> mSurface;

As discussed on IRC, it might be useful to include a comment for why this exists, since you only set it in GetSurface() and nobody else uses it.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

A bit of clarification for the above: nsWindow::GLControllerSupport::mSurface is just stored to keep the JNI reference from going stale, since GetSurface is called by GetNativeData, which ultimately must pass back a void*

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78731bdec697

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78731bdec697

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Reopening this since the patch bounced due to bug 1271103 (and other similar bugs).

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Attachment: Updated patch

An updated patch that stops us from creating a compositor when we don't have a valid surface, hopefully fixing the crash that plagued the old patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2c10a25003
Move EGLSurface creation out of GLController.java. r=snorp

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a2c10a25003

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was backed out from 49/50.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

Ryan, the second patch (see comment 12) was landed after the backout in bug 1271103. This bug should stay resolved.

Comment 17

[Tommy Kuo (away forever...)]

Hi Dylan, James! I'm Tommy, and I work on Chromecast for remote display support (see Bug 1252788 / Bug 1282003 / Bug 1285870). I found you change NS_NATIVE_WINDOW to NS_JAVA_SURFACE in this patch. Can you tell me why?

Because I want to keep a reference of the surface in nsScreenAndroid to make [1] work! Or, do you have any suggestion on that?

Thank you!

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp#610-613

Comment 18

[Dylan Roeh (:droeh) (he/him)]

Tommy, I briefly used NS_NATIVE_WINDOW in lieu of NS_JAVA_SURFACE with an older version of this patch, but that wasn't really correct as it didn't return the native window, but rather the Java surface. As it stands now you can use GetNativeData(NS_JAVA_SURFACE) to get the Java surface and use that to Acquire and Release the native window as necessary like I do in GLContextProviderEGL.cpp[0]. Also, I'm not sure storing a reference to the Java surface is a good idea here unless you have some way of being sure that the surface is still valid. Let me know if you think something like [0] will work for you.

[0]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#180

Comment 19

[Tommy Kuo (away forever...)]

Hi Dylan, thanks for your response. After looking [0], I think I should keep the reference to ANativeWindow (from `AcquireNativeWindow`). Can we keep a reference to ANativeWindow in nsWindow and nsScreen? And return it when someone calls `nsWindow::GetNativeData(NS_NATIVE_WINDOW)`. I want to compare them when `nsScreenManagerAndroid::ScreenForNativeWidget` called (I'll add the logic like [1] to nsScreenManagerAndroid).

Sorry, I'm not very familiar with Android dev. Thanks for your patience.

[0]: https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#180
[1]: https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsScreenManagerGonk.cpp#872-884

Comment 20

[Dylan Roeh (:droeh) (he/him)]

The problem I see with keeping a reference to the ANativeWindow is that I don't think there's a way of guaranteeing that it is still valid -- that's why we don't already cache the Java surface in nsWindow. I think if the Java surface is destroyed the native window will be as well. The best approach may be not to store a reference, but to get the Java surface each time you need the native window, and use the Java surface to call Acquire/ReleaseNativeWindow. This should only add a little bit of complexity to the ScreenForNativeWidget function, I would think: instead of having a function like nsScreenGonk::GetNativeWindow() you would have something like nsScreenAndroid::AcquireNativeWindow() and nsScreenAndroid::ReleaseNativeWindow(), so for each screen you could just acquire the native window reference, do the comparison, and release the reference. As long as nsScreenAndroid has an nsWindow reference this should be doable I think.

If you think that's not going to work, let me know why and we'll see if we can figure something else out.

Comment 21

[Tommy Kuo (away forever...)]

Ok, I'll try to do with your suggestion! Thanks for your help!!

Comment 22

[Tommy Kuo (away forever...)]

Hi Dylan, do you think there is any problem if I just compare the Java surface? I think I can keep the reference to the Java surface in nsScreenAndroid, and change the code in [1] to query native data by NS_JAVA_SURFACE on Fennec.

I think it's reasonable, because nsWindow return nothing when query NS_NATIVE_WINDOW. 

In this way, it could make the logic simpler. But I'm worried about there could be some risk I don't know.

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.cpp#610-613

Comment 23

[Dylan Roeh (:droeh) (he/him)]

That sounds like it should work, yes.

Comment 24

[Tommy Kuo (away forever...)]

Is it possible to get a different surface from `mGLControllerSupport->GetSurface()`. I found the address of the java surface I kept in nsScreenAndroid is different to the one I queried from the same nsWindow in next time.

Comment 25

[Dylan Roeh (:droeh) (he/him)]

So, this may be harder than I thought initially. Since mGLControllerSupport->GetSurface() returns a raw jobject rather than the mozilla::jni::GlobalRef that we get from GLController.getSurface(), you can't just compare the result of two calls to see if they are the same surface; even if they're the same surface the raw jobject may point to different locations, I believe.

Do you have a work in progress patch that I could take a look at? It might be easier for me to think of a way of getting around this if I know exactly how you're trying to do it now.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hi Tommy, we actually already support mirroring to Chromecast via a tee inserted into the Compositor. Randall did this work, so maybe you should talk to him about it.

Comment 27

[Tommy Kuo (away forever...)]

Hi Dylan, my work in progress source code is at https://github.com/kuoe0/gecko-dev/tree/chromecast-on-fennec. For now, I decide to use the displayType as ID to query the screen. Feel free to give me some suggestions!

Hi James, did you mean mirroring videos on Chromecast? I saw some source code can streaming a video on Chromcast. And the work I'm working on is used to streaming a custom surface (a web page different to the one on primary screen) on Chromecast for Presentation API.

Comment 28

[Randall Barker [:rbarker]]

(In reply to Tommy Kuo [:KuoE0] from comment #27)
> Hi Dylan, my work in progress source code is at
> https://github.com/kuoe0/gecko-dev/tree/chromecast-on-fennec. For now, I
> decide to use the displayType as ID to query the screen. Feel free to give
> me some suggestions!
> 
> Hi James, did you mean mirroring videos on Chromecast? I saw some source
> code can streaming a video on Chromcast. And the work I'm working on is used
> to streaming a custom surface (a web page different to the one on primary
> screen) on Chromecast for Presentation API.

Currently when Presentation mode is switched on, we do a second pass composite to the presentation surface so that UI is not included in the mirror sent to the ChromeCast. Last time I tested it, the rendering was broken and only compositing a subset of the content. See: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/PresentationMediaPlayerManager.java