Remove EGLSurface creation in GLController

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: snorp, Assigned: droeh)

Tracking

34 Branch
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Summary: Remove EGLSurface creation → Remove EGLSurface creation in GLController
Assignee: nobody → droeh
(Assignee)

Comment 1

3 years ago
Created attachment 8640489 [details] [diff] [review]
Incomplete patch

snorp, here's what I have at the moment.
Flags: needinfo?(snorp)
Flags: needinfo?(snorp)
(Assignee)

Comment 2

3 years ago
Created attachment 8746235 [details] [diff] [review]
Proposed patch

This moves EGLSurface creation to native code and kills off EGL-related code in GLController.java.
Attachment #8640489 - Attachment is obsolete: true
Attachment #8746235 - Flags: review?(snorp)
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
Attachment #8746235 - Flags: review?(snorp) → review-
(Assignee)

Comment 4

3 years ago
Created attachment 8746789 [details] [diff] [review]
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.
Attachment #8746235 - Attachment is obsolete: true
Attachment #8746789 - Flags: review?(snorp)
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.
Attachment #8746789 - Flags: review?(snorp) → review-
(Assignee)

Comment 6

3 years ago
Created attachment 8747103 [details] [diff] [review]
Proposed patch (updated)

Should address the above
Attachment #8746789 - Attachment is obsolete: true
Attachment #8747103 - Flags: review?(snorp)
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.
Attachment #8747103 - Flags: review?(snorp) → review+
(Assignee)

Comment 8

3 years ago
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 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/78731bdec697
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1271747
Depends on: 1273069
(Assignee)

Comment 11

3 years ago
Reopening this since the patch bounced due to bug 1271103 (and other similar bugs).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

3 years ago
Created attachment 8760910 [details] [diff] [review]
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.
Attachment #8747103 - Attachment is obsolete: true
Attachment #8760910 - Flags: review?(snorp)
Attachment #8760910 - Flags: review?(snorp) → review+

Comment 13

3 years ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2c10a25003
Move EGLSurface creation out of GLController.java. r=snorp

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a2c10a25003
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 49 → Firefox 50
This was backed out from 49/50.
Status: RESOLVED → REOPENED
status-firefox49: fixed → ---
status-firefox50: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 50 → ---
(Assignee)

Comment 16

2 years ago
Ryan, the second patch (see comment 12) was landed after the backout in bug 1271103. This bug should stay resolved.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(snorp)
Flags: needinfo?(droeh)
(Assignee)

Comment 18

2 years ago
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
Flags: needinfo?(droeh)
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
(Assignee)

Comment 20

2 years ago
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.
Ok, I'll try to do with your suggestion! Thanks for your help!!
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
Flags: needinfo?(droeh)
(Assignee)

Comment 23

2 years ago
That sounds like it should work, yes.
Flags: needinfo?(droeh)
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.
Flags: needinfo?(droeh)
(Assignee)

Comment 25

2 years ago
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.
Flags: needinfo?(droeh)
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.
Flags: needinfo?(snorp) → needinfo?(rbarker)
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.
(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
Flags: needinfo?(rbarker)
You need to log in before you can comment on or make changes to this bug.