Closed Bug 814159 Opened 7 years ago Closed 6 years ago

Trim GLContextProviderEGL: Remove X11, Qt, backing-surface and global-GL-context code, and split out TextureImageEGL

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bjacob, Assigned: bjacob)

References

()

Details

Attachments

(4 files, 2 obsolete files)

The context for this bug is that in bug 813783 we stopped using a global context on B2G. We already stopped doing that on Android and on Windows. So the last user of global-context-on-EGL is Meego.

If we agree to drop support for Meego, we can not only remove the global-context code from the EGL provider, but also remove the #ifdef MOZ_X11 / MOZ_WIDGET_QT paths from that file. This allows to remove 400 lines of code from it. As the over-complication of that particular file has been a substantial drag for people working on mobile Gfx, that seems worth it.

Do you think that we need to take to some mailing list the question of whether we have to keep supporting Meego?
Here's a patch that just disables the global context. So it's easy to land and back out. And as I'm not even 100% sure that Meego requires a global context, it might even not kill Meego.
Attachment #684162 - Flags: review?(jmuizelaar)
With Meego/Qt we don't have multiple EGL context/surfaces/windows, basically Window Surface GL Context created only for main toplevel window on Qt side.

Qt EGL provider use that toplevel context as external-created GL context and creating WebGL offscreen contexts shared with that top level context.
But anyway, Meego use IPC rendering path, and global sharing should have any effect there.
Attachment #684162 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/e9cfdd1d72a5
Assignee: nobody → bjacob
Whiteboard: [leave open]
Now that bug 906072 is fixed, we should be free to proceed here, right?
Depends on: 906072
(In reply to Benoit Jacob [:bjacob] from comment #6)
> Now that bug 906072 is fixed, we should be free to proceed here, right?

Sounds like it.
Attachment #684158 - Attachment is obsolete: true
Attachment #684162 - Attachment is obsolete: true
Attachment #815083 - Flags: review?(bgirard)
The global context is only used anymore AFAICR on desktop linux with GL layers (non-default config), not in any EGL setting.
Attachment #815084 - Flags: review?(bgirard)
Theoretically, B2G was using it, but in practice, it was only implemented on X11.
Attachment #815085 - Flags: review?(bgirard)
Summary: Remove X11, Qt, and global-GL-context code from the EGL context provider → Trim GLContextProviderEGL: Remove X11, Qt, backing-surface and global-GL-context code, and split out TextureImageEGL
Comment on attachment 815083 [details] [diff] [review]
part 1: remove X11, Qt, and unused Gonk code

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ -683,5 @@
> -     * and mNullEGLImage will be initialized once to be an EGLImage wrapping it.
> -     *
> -     * This happens in GetNullEGLImage().
> -     */
> -    EGLImage mNullEGLImage;

Was b2g using the null EGLImage at some point to unbind gralloc or something like that (heard jeff say something like that)? Could be an argument to leave some of this code in but we can always fish it out from our history.
Attachment #815083 - Flags: review?(bgirard) → review+
Attachment #815084 - Flags: review?(bgirard) → review+
Attachment #815085 - Flags: review?(bgirard) → review+
Attachment #815086 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #12)
> Comment on attachment 815083 [details] [diff] [review]
> part 1: remove X11, Qt, and unused Gonk code
> 
> Review of attachment 815083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ -683,5 @@
> > -     * and mNullEGLImage will be initialized once to be an EGLImage wrapping it.
> > -     *
> > -     * This happens in GetNullEGLImage().
> > -     */
> > -    EGLImage mNullEGLImage;
> 
> Was b2g using the null EGLImage at some point to unbind gralloc or something
> like that (heard jeff say something like that)? Could be an argument to
> leave some of this code in but we can always fish it out from our history.

Yes, it used to be used in that way; is not used anymore; and I prefer to remove unused code than to give the wrong impression that it is known to be working.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=bf5c812927fd
Raspberry Pi uses X11 with EGL. Does this affect that?
rasppi don't really need X11... it is just EGL context, even qt5 does need only EGL no x11, so we should be good here
Also to be clear, I really really want to be as supportive as possible of platforms such as Raspberry Pi, but IMO it makes more sense to look into doing this the right/modern/supported way (New Layers) and fix GLContext/GLContextProvider to be nice and extensible. We should shoot for making Firefox so good that people would want it to be the default browser, and I think it's OK to temporarily regress support for such platforms if that enables us to get there in a way we can actually support.
No reason to [leave open] anymore.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.