Bug 236769 - [GTK][WPE][WC] Move ANGLE context initialisation to GraphicsContextGLTextureMapper::initialize
Summary: [GTK][WPE][WC] Move ANGLE context initialisation to GraphicsContextGLTextureM...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Other
Hardware: Unspecified Linux
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords: InRadar
Depends on:
Blocks: gcgranglecleanup
  Show dependency treegraph
 
Reported: 2022-02-17 04:08 PST by Kimmo Kinnunen
Modified: 2022-02-28 01:00 PST (History)
16 users (show)

See Also:


Attachments
WIP patch (30.96 KB, patch)
2022-02-20 23:23 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (46.46 KB, patch)
2022-02-24 14:18 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Fix WinCairo (45.62 KB, patch)
2022-02-24 16:35 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2022-02-17 04:08:05 PST
[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 };
Comment 1 Fujii Hironori 2022-02-17 21:34:02 PST
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?
Comment 2 Alejandro G. Castro 2022-02-18 02:37:07 PST
(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.
Comment 3 Kimmo Kinnunen 2022-02-18 12:28:10 PST
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.
Comment 4 Fujii Hironori 2022-02-20 23:23:41 PST
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?
Comment 5 Alejandro G. Castro 2022-02-23 04:54:02 PST
(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.
Comment 6 Alejandro G. Castro 2022-02-24 14:18:03 PST
Created attachment 453137 [details]
Patch
Comment 7 Alejandro G. Castro 2022-02-24 14:21:15 PST
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 :-).
Comment 8 Fujii Hironori 2022-02-24 16:28:46 PST
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)?
Comment 9 Fujii Hironori 2022-02-24 16:35:31 PST
Created attachment 453156 [details]
Fix WinCairo
Comment 10 Alejandro G. Castro 2022-02-25 03:18:21 PST
(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.
Comment 11 Chris Lord 2022-02-25 03:24:32 PST
Nice work, I'm just building and testing now before doing the review.
Comment 12 Chris Lord 2022-02-25 03:50:16 PST
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.
Comment 13 Chris Lord 2022-02-25 06:53:48 PST
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.
Comment 14 EWS 2022-02-25 18:04:43 PST
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].
Comment 15 Radar WebKit Bug Importer 2022-02-25 18:05:24 PST
<rdar://problem/89501470>
Comment 16 Kimmo Kinnunen 2022-02-28 01:00:22 PST
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.