Closed Bug 1563035 Opened 5 months ago Closed 5 months ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: robert.mader, Assigned: robert.mader)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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

Blocks: 1474281
Type: defect → enhancement
OS: Unspecified → Linux
Priority: -- → P3

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

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 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.
Attachment #9075512 - Flags: feedback?(jmuizelaar)

(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 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.
Attachment #9075512 - Flags: feedback?(jmuizelaar) → feedback+
Attachment #9075512 - Flags: feedback+ → feedback?(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
Attachment #9075512 - Flags: review-
Attachment #9075512 - Flags: feedback?(jgilbert)
Attachment #9075512 - Flags: feedback-
Attached patch check_for_gles_not_egl.patch (obsolete) — Splinter Review

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

Attachment #9075512 - Attachment is obsolete: true
Attachment #9075819 - Flags: feedback?(jmuizelaar)

(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 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();
}
Attached patch check_for_gles_not_egl.patch (obsolete) — Splinter Review

Incorporated suggestion Jeff Gilbert

Attachment #9075819 - Attachment is obsolete: true
Attachment #9075819 - Flags: feedback?(jmuizelaar)
Attachment #9075836 - Flags: feedback?(jmuizelaar)
Attachment #9075836 - Flags: feedback?(jgilbert)
Attachment #9075836 - Flags: feedback?(jgilbert) → feedback+

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=

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

Attachment #9075836 - Attachment is obsolete: true
Attachment #9075836 - Flags: feedback?(jmuizelaar)
Attachment #9075853 - Flags: feedback?(jmuizelaar)

(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 on attachment 9075853 [details] [diff] [review]
check_for_gles_not_egl.patch

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

Looks good to me.
Attachment #9075853 - Flags: feedback?(jmuizelaar) → feedback+
Assignee: nobody → robert.mader
Status: UNCONFIRMED → ASSIGNED
Type: enhancement → task
Ever confirmed: true
OS: Linux → All
Hardware: Unspecified → All

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?

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

(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 :)

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.

Attachment #9075853 - Flags: review+

Ah, right, waiting until 70.

Keywords: checkin-needed

Pushed by cbrindusan@mozilla.com:
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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.