Bug 236769 – [GTK][WPE][WC] Move ANGLE context initialisation to GraphicsContextGLTextureMapper::initialize

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

[GTK][WPE] Move ANGLE context initialisation to GraphicsContextGLTextureMapper::initialize

The long-term plan would be to share more code in GraphicsContextGLANGLE between Cocoa and non-Cocoa.
The long-term plan would be to minimise the ifdefs in GraphicsContextGLANGLE.

The first step in this direction would be to move the context initialisation and holding to GraphicsContextGLTextureMapper, away from GCGLANGLELayer::ANGLEContext and the layer classes.
Later on, Cocoa and non-Cocoa can merge their ::initialize.

The idea would be to have 
GraphicsContextGLANGLE::
    GCGLDisplay m_displayObj { nullptr };
    GCGLContext m_contextObj { nullptr };
    GCGLConfig m_configObj { nullptr };

[Fujii Hironori]

GTK and WPE are still using !USE(ANGLE) code.
Will GCGLLayer move into GraphicsContextGLOpenGL?

Will they remove !USE(ANGLE) code soon?

Who will do this? You? Me for WinCairo part?

[Alejandro G. Castro]

(In reply to Fujii Hironori from comment #1)
> GTK and WPE are still using !USE(ANGLE) code.
> Will GCGLLayer move into GraphicsContextGLOpenGL?
> 
> Will they remove !USE(ANGLE) code soon?
> 
> Who will do this? You? Me for WinCairo part?

Good question! Thanks for bringing this up.

We need to support the !USE(ANGLE) code path for WPE and GTK at least for the next release until we make the new code path default and there is no regression, that means probably 2 more releases, around 1 year. Release is going to happen next week and we do not plan to make it the default for the moment, it is too early.

We can remove the !USE(ANGLE) && ENABLE(WEBGL2), that is something we are not going to use and I think Apple ports are not using. I can open a bug and remove that code. And we are all for sharing more code which I understood this bug is about when Kimmo told us.

We have more plans to try to use ANGLE even more but that is something we still do not know how it will work, so it is more long term.

[Kimmo Kinnunen]

For purposes of this bug:
If I'm not mistaken questions about GraphicsContextGLOpenGL and !USE(ANGLE) should not affect the task here, and should not block this.
E.g. move the initialization code from GCGLANGLELayer::ANGLEContext to GraphicsContextGLTextureMapper ANGLE variant. From general platform code, ANGLE parts should be now quite isolated from the legacy code.

[Fujii Hironori]

Created attachment 452714 [details]
WIP patch

I created a patch for WinCairo.
Because WinCairo and Coordinated Graphics share the code,
It's difficult to keep Coordinated Graphics part untouched.
Alex, could you take a look?

[Alejandro G. Castro]

(In reply to Fujii Hironori from comment #4)
> Created attachment 452714 [details]
> WIP patch
> 
> I created a patch for WinCairo.
> Because WinCairo and Coordinated Graphics share the code,
> It's difficult to keep Coordinated Graphics part untouched.
> Alex, could you take a look?

Thanks for the patch! I'll update with the changes we need.

[Alejandro G. Castro]

Created attachment 453137 [details]
Patch

[Alejandro G. Castro]

I've updated and tested the patch for GTK with ANGLE and with system OpenGL. I've added some changes to the context configuration we are using, hopefully they are ok for WinCairo, in case they are not we can use some ifdef in the GraphicsContextGLTextureMapper::platformInitializeContext function.

I did not test in WPE with both options but nowadays it is very aligned with GTK, but bots will tell us :-).

[Fujii Hironori]

Comment on attachment 453137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453137&action=review

Thank you.

> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:68
> +        EGL_PLATFORM_ANGLE_NATIVE_PLATFORM_TYPE_ANGLE, EGL_PLATFORM_SURFACELESS_MESA,

WinCairo is failing to get a platform display because it's using D3D renderer not OpenGL renderer.
WinCairo needs to condition out these 3 lines. What is a good condition for it? !OS(WINDOWS), !PLATFORM(WIN) or USE(NICOSIA)?

[Fujii Hironori]

Created attachment 453156 [details]
Fix WinCairo

[Alejandro G. Castro]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 453137 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453137&action=review
> 
> Thank you.
> 
> > Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:68
> > +        EGL_PLATFORM_ANGLE_NATIVE_PLATFORM_TYPE_ANGLE, EGL_PLATFORM_SURFACELESS_MESA,
> 
> WinCairo is failing to get a platform display because it's using D3D
> renderer not OpenGL renderer.
> WinCairo needs to condition out these 3 lines. What is a good condition for
> it? !OS(WINDOWS), !PLATFORM(WIN) or USE(NICOSIA)?

Great! Thanks! :-) One of those probably will be ok, not sure if there is any preference.

[Chris Lord]

Nice work, I'm just building and testing now before doing the review.

[Chris Lord]

Just an initial observation of something that needs to be fixed, this patch seems to have some kind of corruption or race when resizing contexts - in a debug build with this patch applied and ANGLE enabled, if you go to https://tiny.vision/demos/Tiny3D/Wasm/Tiny3D.html and resize the window a bit, you'll get a "corrupted double-linked list" message in the terminal, followed by a crash. This doesn't seem to happen without the patch.

[Chris Lord]

Comment on attachment 453156 [details]
Fix WinCairo

View in context: https://bugs.webkit.org/attachment.cgi?id=453156&action=review

I take it back, I can reproduce the crash pre-patch so it would appear unrelated. I also tested this on WPE, compiling both with and without ANGLE enabled and it built and functioned correctly. LGTM!

> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapper.h:67
> +    bool platformInitializeContext() final;
> +#endif
> +    bool platformInitialize() final;

It feels weird to me that these are two separate functions, but that's pre-existing so I assume there's a reason. I think at some point it would be good to consolidate here, we have initialize, platformInitialize and platformInitializeContext - this feels like at least one too many functions and certainly doesn't make for easy reading.

[EWS]

Committed r290540 (247818@main): <https://commits.webkit.org/247818@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453156 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89501470>

[Kimmo Kinnunen]

Thanks! Great work everybody!

> It feels weird to me that these are two separate functions, but that's
> pre-existing so I assume there's a reason. I think at some point it would be
> good to consolidate here, we have initialize, platformInitialize and
> platformInitializeContext - this feels like at least one too many functions
> and certainly doesn't make for easy reading.

Some of the functions certainly can be removed once more code is shared.