Bug 166489

Summary: REGRESSION(r208997): [GLX] Google maps labels broken when using glXCreateContextAttribsARB
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Miguel Gomez <magomez>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, commit-queue, dino, graouts, kondapallykalyan, magomez, mcatanzaro, noam, tpopela
Priority: P2 Keywords: Gtk, Regression
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Carlos Garcia Campos 2016-12-27 04:09:22 PST
This reminds me to bug #154069. Labels are shown initially and then they are hidden by black rectangles. It doesn't happen if r208997 is reverted, or if code is forced to use legacy glXCreateContext, by always returning false from hasGLXARBCreateContextExtension().
Comment 1 Carlos Garcia Campos 2016-12-27 04:15:07 PST
Using glXCreateContextAttribsARB, but passing nullptr as contextAttributes also makes it work.
Comment 2 Carlos Garcia Campos 2016-12-28 03:02:10 PST
Ok, this seems to be a problem in Mesa, see:

https://productforums.google.com/forum/#!category-topic/maps/linux/CqvELpJgaxI

Chomium users had the same issue. Chromium fixed it by simply not using OpenGL context versions between 3.0 and 3.2 on Mesa. See:

https://bugs.chromium.org/p/chromium/issues/detail?id=659030
Comment 3 Carlos Garcia Campos 2016-12-28 05:27:27 PST
I've tried with 3.3 but it doesn't work either. Looking at the chromium code, it tries to use 3.3, but checking about:gpu in my chromium (from debian testing) I see it's using 3.0, but I don't understand why.
Comment 4 Carlos Garcia Campos 2016-12-28 05:30:53 PST
hmm, I guess debian chromium is too old (53.x), using chrome (55.x) I see 3.3 is indeed used, and google maps works, so there must be something else
Comment 5 Carlos Garcia Campos 2016-12-28 06:28:39 PST
I've found a workaround, using 3.0 for offscreen contexts and 3.2 for window and sharing contexts makes google maps work again. I don't know if that could cause other issues, though. I've tried other webgls sites and everything seems to work normally.
Comment 6 Michael Catanzaro 2016-12-28 11:03:51 PST
Hmmm. It'd be good to have an opinion from Miguel on whether your proposed workaround could break something, even if that delays 2.14.3 a bit.

We have a big team of mesa hackers; if we decide to use this workaround, then I think we should ask them to investigate, since it appears likely to be a mesa bug.
Comment 7 Miguel Gomez 2017-01-12 00:57:25 PST
The problem seems to be using a core profile. Both using the legacy path or using glXCreateContextAttribsARB without specifying a version return a compatibility profile, and with that it works.

I've tried increasing the version we request for the core profile to 3.3 and it doesn't work, as Carlos mentions. But I'm seeing a mesa error when running the page that could be related. I'll investigate that.
Comment 8 Miguel Gomez 2017-01-13 05:56:56 PST
I've finally found the problem. It happens because WebGL is passing some parameters to OpenGL calls that have been deprecated and are not present in a core profile. That's why it works with a compatibility profile (those parameters are supported in Mesa compatibility) and not with a core profile.

In this concrete case, the problem happens when using glTexImage2D and glTexSubImage2D using GL_ALPHA as the format parameter. GL_ALPHA is a valid parameter in WebGL but not in OpenGL core, so the call fails in core profile and the labels are not rendered.

In order to fix this, GL_ALPHA has to be replaced, using GL_RED (which is the only single component format available in core), and then swizzle the red and alpha components when accessing the texture data.

I'll attach a patch in a while with the fix.
Comment 9 Miguel Gomez 2017-01-13 07:13:39 PST
Created attachment 298758 [details]
Patch
Comment 10 Zan Dobersek 2017-01-13 07:25:47 PST
Comment on attachment 298758 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:37
> +#include "GLContext.h"

I'm not sure whether GLContext should be used in this file. I'd rather see a m_usingCoreProfile boolean (or something like that) in the GraphicsContext3D class that can be set at construction and then checked.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:388
> +#if PLATFORM(GTK)

Should this be USE(GLX) instead? The problem is exclusive to that.
Comment 11 Miguel Gomez 2017-01-16 06:43:44 PST
Created attachment 298958 [details]
Patch
Comment 12 Carlos Garcia Campos 2017-01-16 06:54:06 PST
Comment on attachment 298958 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:390
> +    if (m_usingCoreProfile && openGLInternalFormat == GL_ALPHA) {

m_usingCoreProfile is defined unconditionally, right? Then it will only be true for ports using GraphicsContext3DCairo.cpp, so I don't think we need PLATFORM(GTK) ifdef here.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1749
> +#if PLATFORM(GTK) && !USE(OPENGL_ES_2)

Ditto.
Comment 13 Miguel Gomez 2017-01-16 07:15:44 PST
(In reply to comment #12)
> Comment on attachment 298958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298958&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:390
> > +    if (m_usingCoreProfile && openGLInternalFormat == GL_ALPHA) {
> 
> m_usingCoreProfile is defined unconditionally, right? Then it will only be
> true for ports using GraphicsContext3DCairo.cpp, so I don't think we need
> PLATFORM(GTK) ifdef here.
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1749
> > +#if PLATFORM(GTK) && !USE(OPENGL_ES_2)
> 
> Ditto.

Very true! I'll fix that
Comment 14 Miguel Gomez 2017-01-16 07:21:43 PST
Created attachment 298962 [details]
Patch
Comment 15 Miguel Gomez 2017-01-17 01:41:36 PST
Created attachment 299022 [details]
Patch
Comment 16 Zan Dobersek 2017-01-17 02:18:40 PST
Comment on attachment 299022 [details]
Patch

LGTM. Land it once the EWSs confirm it's building properly.
Comment 17 WebKit Commit Bot 2017-01-17 03:09:55 PST
Comment on attachment 299022 [details]
Patch

Clearing flags on attachment: 299022

Committed r210800: <http://trac.webkit.org/changeset/210800>
Comment 18 WebKit Commit Bot 2017-01-17 03:10:02 PST
All reviewed patches have been landed.  Closing bug.