1447270 - [wayland] GLContextGLX::FindVisual undefined when using EGL

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jan Horak [:jhorak]]

Since fix for bug 1401455 landed, building with wayland support ends by:
/run/build/firefox/widget/gtk/nsWindow.cpp:3659: error: undefined reference to 'mozilla::gl::GLContextGLX::FindVisual(_XDisplay*, int, bool, bool, int*)'

Placing FindVisual only to the GLContextGLX breaks the wayland builds ATM because it's not build with using EGL:
https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/gfx/gl/moz.build#17

Comment 1

[Jan Horak [:jhorak]]

../../build/unix/gold/ld: error: /run/build/firefox/obj-x86_64-pc-linux-gnu/toolkit/library/../../widget/g
../../build/unix/gold/ld: error: /run/build/firefox/obj-x86_64-pc-linux-gnu/toolkit/library/../../widget/gtk/nsWindow.o: requires dynamic R_X86_64_PC32 reloc against '_ZN7mozilla2gl12GLContextGLX10FindVisualEP9_XDisplayibbPi' which may overflow at runtime; recompile with -fPIC
../../build/unix/gold/ld: error: read-only segment has dynamic relocations
/run/build/firefox/widget/gtk/nsWindow.cpp:3658: error: undefined reference to 'mozilla::gl::GLContextGLX::FindVisual(_XDisplay*, int, bool, bool, int*)'

Hm, full error output suggests adding -fPIC.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jan Horak [:jhorak] from comment #1)
> Hm, full error output suggests adding -fPIC.

I've seen that error for undefined symbols before.  I think what's going on here is that it's declared with hidden visibility (see config/gcc_hidden.h), so the call site gets a plain PC-relative relocation (instead of PLT) because that's normally fine for intra-libxul calls, but then when it winds up being undefined it looks like a text relocation due to not using -fPIC, even though we *are* using -fPIC.

Comment 3

[Takuro Ashie [:ashie]]

Attachment: Bug 1447270 - Fix build failure of cairo-gtk3-wayland

On cairo-gtk3-wayland, GLContextProviderEGL is used by default
instead of GLContextProviderGLX, so GLContextGLX::FindVisual()
isn't defined.

Review commit: https://reviewboard.mozilla.org/r/231490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231490/

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8962634 [details]
Bug 1447270 - Fix build failure of cairo-gtk3-wayland

https://reviewboard.mozilla.org/r/231490/#review236998

::: widget/gtk/nsWindow.cpp:3657
(Diff revision 1)
>  
>          // If using WebRender on X11, we need to select a visual with a depth buffer,
>          // as well as an alpha channel if transparency is requested. This must be done
>          // before the widget is realized.
>          if (mIsX11Display && useWebRender) {
> +#ifdef GL_CONTEXT_PROVIDER_IS_GLX

That fails for wayland windows with useAlphaVisual set.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

Review commit: https://reviewboard.mozilla.org/r/231516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231516/

Comment 6

[Jan Horak [:jhorak]]

Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

https://reviewboard.mozilla.org/r/231516/#review237074

::: widget/gtk/nsWindow.cpp:3644
(Diff revision 1)
>          // mozilla.widget.use-argb-visuals is a hidden pref defaulting to false
>          // to allow experimentation
>          if (Preferences::GetBool("mozilla.widget.use-argb-visuals", false))
>              useAlphaVisual = true;
>  
> +#ifndef MOZ_WAYLAND

Hm, what happens to the wayland build when only the x11 backend will be available?

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

https://reviewboard.mozilla.org/r/231516/#review237074

> Hm, what happens to the wayland build when only the x11 backend will be available?

A Wayland build uses EGL no matter what actual backend is used, because GL backend is choosed at build time. This means that Wayland builds running on X11 backend do not have working OpenGL (Bug 1438144).

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8962634 [details]
Bug 1447270 - Fix build failure of cairo-gtk3-wayland

https://reviewboard.mozilla.org/r/231490/#review237264

Testing a define directly related to the context provider is the right thing to do, but it needs to be in the right place so that the useAlphaVisual code is not skipped.

::: gfx/gl/GLContextProvider.h:49
(Diff revision 1)
>  #if defined(MOZ_X11) && !defined(MOZ_WAYLAND)
>    #define GL_CONTEXT_PROVIDER_NAME GLContextProviderGLX
>    #include "GLContextProviderImpl.h"
>    #undef GL_CONTEXT_PROVIDER_NAME
>    #define GL_CONTEXT_PROVIDER_DEFAULT GLContextProviderGLX
> +  #define GL_CONTEXT_PROVIDER_IS_GLX 1

The default provider can be overridden by MOZ_GL_PROVIDER below and so it is not right to say _IS_GLX here.

The intention of changes for bug 575032 was that multiple GL providers could
be co-exist.  I don't know whether that still works, but I wonder why the
!defined(MOZ_WAYLAND) was added here (instead of around
GL_CONTEXT_PROVIDER_DEFAULT).

moz.configure defines GL_PROVIDER_GLX=1 for the default provider, and so I
suggest using that.  I assume it would make sense to also define this when the
default provider is EGL but GLX is built, but I don't know whether that works,
and it is a separate issue.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

https://reviewboard.mozilla.org/r/231516/#review237274

::: widget/gtk/nsWindow.cpp:3644
(Diff revision 1)
>          // mozilla.widget.use-argb-visuals is a hidden pref defaulting to false
>          // to allow experimentation
>          if (Preferences::GetBool("mozilla.widget.use-argb-visuals", false))
>              useAlphaVisual = true;
>  
> +#ifndef MOZ_WAYLAND

Test GL_PROVIDER_GLX for a more directly related define.

It is theoretically possible to have a GLX provider in a Wayland build.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8962634 [details]
Bug 1447270 - Fix build failure of cairo-gtk3-wayland

https://reviewboard.mozilla.org/r/231490/#review237282

::: widget/gtk/nsWindow.cpp:144
(Diff revision 1)
>  using namespace mozilla;
>  using namespace mozilla::gfx;
>  using namespace mozilla::widget;
>  using namespace mozilla::layers;
>  using mozilla::gl::GLContext;
> +#ifdef GL_CONTEXT_PROVIDER_IS_GLX

Gecko doesn't usually conditionally include its own header files based on features available, unless this is necessary to build for some reason.

Comment 11

[Jan Alexander Steffens [:heftig]]

Hm, I think that in order not to break WR you still need to do an X visual selection (using eglChooseConfig before the window is realized) if your context provider is EGL and your backend is X11.

Comment 12

[Takuro Ashie [:ashie]]

Comment on attachment 8962634 [details]
Bug 1447270 - Fix build failure of cairo-gtk3-wayland

https://reviewboard.mozilla.org/r/231490/#review237264

> The default provider can be overridden by MOZ_GL_PROVIDER below and so it is not right to say _IS_GLX here.
> 
> The intention of changes for bug 575032 was that multiple GL providers could
> be co-exist.  I don't know whether that still works, but I wonder why the
> !defined(MOZ_WAYLAND) was added here (instead of around
> GL_CONTEXT_PROVIDER_DEFAULT).
> 
> moz.configure defines GL_PROVIDER_GLX=1 for the default provider, and so I
> suggest using that.  I assume it would make sense to also define this when the
> default provider is EGL but GLX is built, but I don't know whether that works,
> and it is a separate issue.

At first, I agree with using `GL_PROVIDER_GLX`.
Martin's patch (https://reviewboard.mozilla.org/r/231516/#review237274) with `GL_PROVIDER_GLX` seems fine for me.
So I retract my patch.

> The intention of changes for bug 575032 was that multiple GL providers could
> be co-exist.

I think that the ideal solution for this issue is that make GLContextextProvider switchable (GLX & EGL) at runtime.
It seems that Google Chrome can do it.

> The default provider can be overridden by MOZ_GL_PROVIDER below and so it is not right to say _IS_GLX here.

I don't think so.
It never be overriden in the current code becuase the following condition exists:

```
#ifndef MOZ_GL_PROVIDER
  ...
  #define GL_CONTEXT_PROVIDER_DEFAULT GLContextProviderGLX
  ...
#endif

#ifdef MOZ_GL_PROVIDER
  ...
  #define GL_CONTEXT_PROVIDER_DEFAULT MOZ_GL_PROVIDER
  ...
#endif
```

The current code is too confusing.
It should be:

```
#ifdef MOZ_GL_PROVIDER
  ...
  #define GL_CONTEXT_PROVIDER_DEFAULT MOZ_GL_PROVIDER
  ...
#else
  ...
  #define GL_CONTEXT_PROVIDER_DEFAULT GLContextProviderGLX
  ...
#endif
```

Comment 13

[Takuro Ashie [:ashie]]

Comment on attachment 8962634 [details]
Bug 1447270 - Fix build failure of cairo-gtk3-wayland

https://reviewboard.mozilla.org/r/231490/#review237264

> At first, I agree with using `GL_PROVIDER_GLX`.
> Martin's patch (https://reviewboard.mozilla.org/r/231516/#review237274) with `GL_PROVIDER_GLX` seems fine for me.
> So I retract my patch.
> 
> > The intention of changes for bug 575032 was that multiple GL providers could
> > be co-exist.
> 
> I think that the ideal solution for this issue is that make GLContextextProvider switchable (GLX & EGL) at runtime.
> It seems that Google Chrome can do it.
> 
> > The default provider can be overridden by MOZ_GL_PROVIDER below and so it is not right to say _IS_GLX here.
> 
> I don't think so.
> It never be overriden in the current code becuase the following condition exists:
> 
> ```
> #ifndef MOZ_GL_PROVIDER
>   ...
>   #define GL_CONTEXT_PROVIDER_DEFAULT GLContextProviderGLX
>   ...
> #endif
> 
> #ifdef MOZ_GL_PROVIDER
>   ...
>   #define GL_CONTEXT_PROVIDER_DEFAULT MOZ_GL_PROVIDER
>   ...
> #endif
> ```
> 
> The current code is too confusing.
> It should be:
> 
> ```
> #ifdef MOZ_GL_PROVIDER
>   ...
>   #define GL_CONTEXT_PROVIDER_DEFAULT MOZ_GL_PROVIDER
>   ...
> #else
>   ...
>   #define GL_CONTEXT_PROVIDER_DEFAULT GLContextProviderGLX
>   ...
> #endif
> ```

> I don't think so.
> It never be overriden in the current code becuase the following condition exists:

Oops, sorry it's not correct.
I applied https://reviewboard.mozilla.org/r/216718/diff/2#index_header locally.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231516/diff/1-2/

Comment 15

[Jan Horak [:jhorak]]

Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

https://reviewboard.mozilla.org/r/231516/#review237406

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c885f9c0448
Use GLContextGLX::FindVisual() on GLX enabled builds only, r=jhorak

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2c885f9c0448

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to Jan Alexander Steffens [:heftig] from comment #11)
> Hm, I think that in order not to break WR you still need to do an X visual
> selection (using eglChooseConfig before the window is realized) if your
> context provider is EGL and your backend is X11.

Yes, I assume that would be the right thing to do, but AFAIK this change
is not actually "break"ing WR in any cases.  It is more that bug 1401455
was addressed for only one of multiple possible situations that would
trigger it.  i.e. WR was already broken.