1563035 - Webrender assumes that an EGL context uses GLES (but it can be GL, too)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task, P3)

Description

[Robert Mader [:rmader]]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Currently Webrender assumes(1,2) a EGL context will use GLES, but that doesn't need to be true. For bug 1474281 we need to differentiate more clearly.

1: https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1113
2: https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1178

Comment 1

[Robert Mader [:rmader]]

Attachment: export_gl_type_from_context.patch

Export the GL type (GL or GLES) from in the GL context and use that in webrender.

Comment 2

[Robert Mader [:rmader]]

As I'm new to the firefox contribution workflow (and c++ and rust), please be generous with criticism and feedback. If there's anything, especially formal stuff, were I can improve, please let me know.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

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

This looks sensible to me, but probably graphics people should review it.

In terms of process, the recommended way of submitting patches is via phabricator: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

We can probably abuse the feedback flag to submit a review request this time, probably. Jeff, mind taking a look at this patch or forwarding to somebody more appropriate?

::: gfx/gl/GLContextTypes.h
@@ +15,4 @@
>  class GLContext;
>  
>  enum class GLContextType { Unknown, WGL, CGL, GLX, EGL, EAGL };
> +enum class GLType { Unknown, GL, GLES };

Is `Unknown` used anywhere? If not probably should be removed.

Comment 4

[Robert Mader [:rmader]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

Review of attachment 9075512 [details] [diff] [review]:

This looks sensible to me, but probably graphics people should review it.

Thanks for the fast reply!

In terms of process, the recommended way of submitting patches is via
phabricator:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Ah of course, will read into it.

::: gfx/gl/GLContextTypes.h
@@ +15,4 @@

class GLContext;

enum class GLContextType { Unknown, WGL, CGL, GLX, EGL, EAGL };
+enum class GLType { Unknown, GL, GLES };

Is Unknown used anywhere? If not probably should be removed.

I thought it might be common style, therefore blindly copied from above, but there shouldn't be any other cases. Would also allow us so set GL as default without overwriting the func for GLX/WGL/CGL. I'll wait for a full review, though, to not mess up things more.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

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

The WebRender parts look fine. Jgilbert is probably a good person to look at the gl parts.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

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

https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/gfx/gl/GLContext.h#252

Comment 7

[Robert Mader [:rmader]]

Attachment: check_for_gles_not_egl.patch

Removed the gl bits as they duplicated existing functionality. Now it's webrender only.

Comment 8

[Robert Mader [:rmader]]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

Review of attachment 9075512 [details] [diff] [review]:

https://searchfox.org/mozilla-central/rev/
11712bd3ce7454923e5931fa92eaf9c01ef35a0a/gfx/gl/GLContext.h#252

Ouch, of course something like that existed already :)
Ok, so now it's only the webrender part. Jeff, could you have another look?

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9075819 [details] [diff] [review]
check_for_gles_not_egl.patch

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +103,5 @@
> +      reinterpret_cast<mozilla::gl::GLContext*>(glcontext_ptr);
> +  if (!glcontext) {
> +    return false;
> +  }
> +  return glcontext->IsGLES();

TBH I'd write this as one of:
bool is_glcontext_gles(void* const glcontext_ptr) {
  MOZ_RELEASE_ASSERT(glcontext_ptr);
  return reinterpret_cast<mozilla::gl::GLContext*>(glcontext_ptr)->IsGLES();
}
or
bool is_glcontext_gles(void* const glcontext_ptr) {
  MOZ_RELEASE_ASSERT(glcontext_ptr);
  const auto glcontext = reinterpret_cast<mozilla::gl::GLContext*>(glcontext_ptr);
  return glcontext->IsGLES();
}

Comment 10

[Robert Mader [:rmader]]

Attachment: check_for_gles_not_egl.patch

Incorporated suggestion Jeff Gilbert

Comment 11

[Darkspirit]

Unused is_glcontext_egl() should be deleted from WebRenderBridgeParent.cpp and webrender_ffi.h as well, or not(?)
https://searchfox.org/mozilla-central/search?q=is_glcontext_egl&path=

Comment 12

[Robert Mader [:rmader]]

Attachment: check_for_gles_not_egl.patch

Properly check for the GL type instead of assuming a EGL context implies GLES

Comment 13

[Robert Mader [:rmader]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #11)

Unused is_glcontext_egl() should be deleted from WebRenderBridgeParent.cpp and webrender_ffi.h as well, or not(?)
https://searchfox.org/mozilla-central/search?q=is_glcontext_egl&path=

Right, it seems unlikely that it will be needed any more. Fixed in lates version

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9075853 [details] [diff] [review]
check_for_gles_not_egl.patch

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

Looks good to me.

Comment 15

[Robert Mader [:rmader]]

Thanks! How do we proceed with this? Can I set the 'checkin needed' flag and the patch will get pushed? Or does that only work for phabricator?

Comment 16

[Darkspirit]

Nightly 70 begins on Monday. Let's wait until then, I guess.(?)

Comment 17

[Robert Mader [:rmader]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #16)

Nightly 70 begins on Monday. Let's wait until then, I guess.(?)

That sounds right. But do I need to do something then or would you or somebody else push it? Sorry for these nooby questions, it's my first time landing a patch in FF :)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Me or somebody else with L3 access can push it, or you can set the checkin-needed flag on the bug and some sheriff will get to it.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Ah, right, waiting until 70.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90032db12be
Webrender assumes that an EGL context uses GLES (but it can be GL, too). r=jgilbert

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e90032db12be