Bug 103757 - eglGetDisplay() is broken for Wayland
Summary: eglGetDisplay() is broken for Wayland
Status: RESOLVED MOVED
Alias: None
Product: Mesa
Classification: Unclassified
Component: EGL/Wayland (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Wayland bug list
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-15 12:03 UTC by Michael Olbrich
Modified: 2019-09-18 18:09 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
egldisplay-try-interface-name-for-wl-display.patch (1.13 KB, patch)
2017-11-15 13:39 UTC, Daniel Stone
Details | Splinter Review

Description Michael Olbrich 2017-11-15 12:03:13 UTC
_eglNativePlatformDetectNativeDisplay() (called by eglGetDisplay()) detects Wayland by checking if the native display object starts with a pointer to wl_display_interface.

Unfortunately wl_display_interface is defined in both libwayland-server.so and libwayland-client.so. libEGL links to both and in all my tests it always picks the one from libwayland-server.so.
The wl_display object passed as native display however contains the version from libwayland-client.so so the check fails.

There is an old thread about this[1] on the wayland-devel list, but it seems nothing happened since then.

[1] https://lists.freedesktop.org/archives/wayland-devel/2014-October/017783.html
Comment 1 Pekka Paalanen 2017-11-15 12:34:54 UTC
eglGetDisplay() is by definition broken on multi-platform implementations, yes.

Any reason you cannot use EGL_KHR_platform_wayland?
Comment 2 Daniel Stone 2017-11-15 13:38:40 UTC
(In reply to Pekka Paalanen from comment #1)
> eglGetDisplay() is by definition broken on multi-platform implementations,
> yes.

Ugh. It is broken by definition, but Mesa does go to great lengths to try to make it work by auto-detecting: see _eglNativePlatformDetectNativeDisplay() inside src/egl/main/egldisplay.c.

Michael, I'll attach a compiled but untested patch; can you please let me know if it helps?
Comment 3 Daniel Stone 2017-11-15 13:39:34 UTC
Created attachment 135490 [details] [review]
egldisplay-try-interface-name-for-wl-display.patch
Comment 4 Emil Velikov 2017-11-15 15:15:41 UTC
<cheeky>
I do recall mentioning that exposing the same symbol(s) will lead to bad experiences. Followed by some ideas...
</cheeky>

WRT eglGetDisplay - I'd recommend moving towards the EGL_*_platform_* extensions ASAP - EGL_EXT_platform_wayland or EGL_KHR_platform_wayland in this case.

A better heuristic would be to use dladdr() for eglGetDisplay. See the analogous GBM patch [1].


https://patchwork.freedesktop.org/patch/187918/
Comment 5 Daniel Stone 2017-11-15 15:31:03 UTC
(In reply to Emil Velikov from comment #4)
> <cheeky>
> I do recall mentioning that exposing the same symbol(s) will lead to bad
> experiences. Followed by some ideas...
> </cheeky>

Yep, as per the discussion on inlining wayland_drm_buffer_get. :)

It certainly hurts, but then again breaking ABI and forcing every single Wayland user to rebuild - probably having to change every symbol name in the API to make sure there was no clash between the two when loaded - would be uncharted depths of pain.

On the other hand, the solution is the same: look at the interface name, rather than ever compare pointers equally. Unfortunately, we can't use the same solution since the wayland-drm one only works for wl_resource in libwayland-server rather than a plain wl_interface or a wl_proxy ... oh well.

> A better heuristic would be to use dladdr() for eglGetDisplay. See the
> analogous GBM patch [1].

I don't think that would necessarily fix it, particularly if the wayland-client.so.1 being loaded differed, e.g. due to people linking their own copies?
Comment 6 Michael Olbrich 2017-11-15 16:30:11 UTC
(In reply to Daniel Stone from comment #2)

> Michael, I'll attach a compiled but untested patch; can you please let me
> know if it helps?

Looks good. It does the right thing for wayland and drm at least.
is _eglPointerIsDereferencable() sufficient to ensure that the whole string is accessible?
Comment 7 Daniel Stone 2017-11-15 16:37:40 UTC
(In reply to Michael Olbrich from comment #6)
> (In reply to Daniel Stone from comment #2)
> 
> > Michael, I'll attach a compiled but untested patch; can you please let me
> > know if it helps?
> 
> Looks good. It does the right thing for wayland and drm at least.
> is _eglPointerIsDereferencable() sufficient to ensure that the whole string
> is accessible?

Good point - I guess it'd have to be if (_eglPointerIsDereferencable(child_pointer) && _eglPointerIsDereferencable(((char *) child_pointer) + strlen("wl_display") + 1) && strcmp(interface->name, "wl_display") == 0) ... ugh. Does that work for you?
Comment 8 Michael Olbrich 2017-11-15 16:49:08 UTC
(In reply to Daniel Stone from comment #7)
> Good point - I guess it'd have to be if
> (_eglPointerIsDereferencable(child_pointer) &&
> _eglPointerIsDereferencable(((char *) child_pointer) + strlen("wl_display")
> + 1) && strcmp(interface->name, "wl_display") == 0) ... ugh. Does that work
> for you?

Yes that works.
Comment 9 Michael Olbrich 2017-11-15 16:57:13 UTC
(In reply to Pekka Paalanen from comment #1)
> Any reason you cannot use EGL_KHR_platform_wayland?

It's not my code. It's somewhere in gstreamer-vaapi. I was just debugging why my application was crashing.
Comment 10 Pekka Paalanen 2017-11-16 07:24:20 UTC
(In reply to Michael Olbrich from comment #9)
> (In reply to Pekka Paalanen from comment #1)
> > Any reason you cannot use EGL_KHR_platform_wayland?
> 
> It's not my code. It's somewhere in gstreamer-vaapi. I was just debugging
> why my application was crashing.

Ok, while this issue is being worked on in Mesa, I would very much recommend filing a bug against gstreamer-vaapi to start using the EGL platform extensions whenever available. It's the only reliable way forward.
Comment 11 GitLab Migration User 2019-09-18 18:09:06 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/171.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.