Closed Bug 1447270 Opened 2 years ago Closed 2 years ago

[wayland] GLContextGLX::FindVisual undefined when using EGL

Categories

(Core :: Widget: Gtk, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed

People

(Reporter: jhorak, Assigned: stransky)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
../../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.
(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 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 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 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 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.
Attachment #8962634 - Flags: review?(karlt) → review-
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 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.
Blocks: 1401455
Keywords: regression
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 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 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.
Attachment #8962634 - Attachment is obsolete: true
Comment on attachment 8962656 [details]
Bug 1447270 - Use GLContextGLX::FindVisual() on GLX enabled builds only,

https://reviewboard.mozilla.org/r/231516/#review237406
Attachment #8962656 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/2c885f9c0448
Use GLContextGLX::FindVisual() on GLX enabled builds only, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/2c885f9c0448
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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.
Assignee: nobody → stransky
You need to log in before you can comment on or make changes to this bug.