Add initial support for wayland to bgfx renderer #11451

belegdol · 2023-07-30T19:43:59Z

This is an initial working prototype of wayland working with bgfx renderer. It only works with -bgfx_backend opengl due to bkaradzic/bgfx#3143 still needing work. Proper conditionalisation is also required so that X11 or Wayland are included and linked only when required.
~~In order to get to this stage, update to the libraries was required:~~
~~- bgfx to 3101a0d93f024e4278caaab29566647cd3e2bf84~~
~~- bx to 198cd120e4941d8aeb677db945ef4291a1ced291~~
~~- bimg to ec02df824a763b2e2ae31e19c674ba0bc88c0695~~
Libs update moved to #11493 for clarity.

While still hacky, it allows for bgfx in a native Wayland window:

belegdol · 2023-08-02T13:56:08Z

Now vulkan renderer is working on wayland as well. I am not sure whether this is ready to be merged due to the following:

with vulkan renderer on wayland the window is blinking with umk3 on my nvidia system. Not sure where the issue is coming from
in comparison with current state, attempting to start mame with -videodriver wayland when it is not built in results in segmentation fault
with wayland support built in, attempting to start mame with -videodriver x11 results in segmentation fault due to -bgfx_backend vulkan being default

Nevertheless, it shows that native wayland is possible.

belegdol · 2023-08-02T15:29:45Z

Blinking seems to be an nvidia specific issue, I do not see this on my amd laptop.

cuavas · 2023-08-02T16:51:37Z

scripts/src/3rdparty.lua

-			MAME_DIR .. "3rdparty/bgfx/src/glcontext_eagl.mm",
-			MAME_DIR .. "3rdparty/bgfx/src/glcontext_nsgl.mm",
 			MAME_DIR .. "3rdparty/bgfx/src/renderer_mtl.mm",


Will this potentially cause issues for older Apple hardware that has poor Metal support?

Probably, but OpenGL support was dropped from upstream bgfx:
bkaradzic/bgfx@928800f

If the PR is interesting but we want to keep OpenGL on Macs, I can see if the necessary commits can be cherry-picked instead.

@rb6502 how many people are going to want my head if bgfx via OpenGL is no longer possible on macOS?

I just got done promising Mac users I wouldn't lift the system requirements until January, so I would strongly prefer to hold off on anything that would affect that until then.

Particularly as Belegdol says this doesn't actually work that well (which describes the totality of Wayland in my experience, to be fair).

Fair enough. Maybe we can leave this PR open in the draft state as a proof-of-concept?

cuavas · 2023-08-02T16:57:30Z

src/osd/modules/render/drawbgfx.cpp

You really should follow the code style of files you’re modifying. Also, why are you casting data pointers through uintptr_t? That’s only necessary when coercing pointer-to-code to pointer-to-data or vice versa.

I dropped the cast. Apologies for the style, I am a C++ newbie at best. The code is more or less a copy-paste from
https://github.com/bkaradzic/bgfx/blob/3101a0d93f024e4278caaab29566647cd3e2bf84/examples/common/entry/entry_sdl.cpp#L53-L64

Do you mind being more specific in terms of what I should fix style-wise?

belegdol · 2023-08-03T09:09:53Z

There are at least two additional points which would be worth clarifying before this is ready for merging

can wayland be detected at runtime? It works for EGL renderer but not for the Vulkan one as it needs a different function on wayland. If this is possible, segfaults would go away. I got it running for native X11 and native Wayland with the help of $XDG_SESSION_TYPE, but this does not cover XWayland unfortunately.
why is egl_window pointer provided by SDL not working: Defer EGLSurface creation to SDL_GL_CreateContext libsdl-org/SDL#5386 (comment)

belegdol · 2023-08-11T08:50:48Z

I have now pushed an update allowing wayland and X11 to be compiled in simultaneously. XWayland is working as well. The patches needed to achieve this are not merged to upstream bgfx yet, but I am putting these here in case someone is curious and would like to test it themselves.
SDL upstream have also clarified that the egl_window pointer being defined the way it was was an accident meaning that creating it with wl_egl_window_create is the way to go.
With nvidia binary driver this is not really usable: with vulkan renderer the window is blinking whereas opengl one falls back to software rendering [1], but I would tend do blame these on generally subpar wayland support with the nvidia drivers.

[1] https://forums.developer.nvidia.com/t/hardware-egl-not-working-on-wayland-libegl-warning-egl-failed-to-create-dri2-screen/262167

belegdol · 2023-08-18T19:56:32Z

I moved the update to the 3rdparty libraries to a separate PR (#11493) for clarity.

cuavas · 2023-08-19T07:01:35Z

I moved the update to the 3rdparty libraries to a separate PR (#11493) for clarity.

It’s still showing as having all those 3rdparty files changed (1063 files changed).

belegdol · 2023-08-19T07:16:45Z

I moved the update to the 3rdparty libraries to a separate PR (#11493) for clarity.

It’s still showing as having all those 3rdparty files changed (1063 files changed).

Apologies. This is either me failing at github or github not being able to create a pr based on another PR. If I understand correctly, it would work if the b* libs update could be temporarily put in a mamedev branch.
Alternatively I could rebase against master but this would make testing slightly more complicated.

belegdol · 2023-08-19T14:02:39Z

Apologies if it is a dumb question, but is the code in

mame/src/osd/modules/render/drawbgfx.cpp

Lines 490 to 528 in 36f5b6e

    
           //============================================================ 
        
           //  helper for getting native platform window 
        
           //============================================================ 
        
           #ifdef OSD_SDL 
        
           static void *sdlNativeWindowHandle(SDL_Window *window) 
        
           { 
        
           	SDL_SysWMinfo wmi; 
        
           	SDL_VERSION(&wmi.version); 
        
           	if (!SDL_GetWindowWMInfo(window, &wmi)) 
        
           		return nullptr; 
        
           	switch (wmi.subsystem) 
        
           	{ 
        
           #if defined(SDL_VIDEO_DRIVER_WINDOWS) 
        
           	case SDL_SYSWM_WINDOWS: 
        
           		return wmi.info.win.window; 
        
           #endif 
        
           #if defined(SDL_VIDEO_DRIVER_X11) 
        
           	case SDL_SYSWM_X11: 
        
           		return (void *)uintptr_t(wmi.info.x11.window); 
        
           #endif 
        
           #if defined(SDL_VIDEO_DRIVER_COCOA) 
        
           	case SDL_SYSWM_COCOA: 
        
           		return wmi.info.cocoa.window; 
        
           #endif 
        
           #if defined(SDL_VIDEO_DRIVER_WAYLAND) && SDL_VERSION_ATLEAST(2, 0, 16) 
        
           	case SDL_SYSWM_WAYLAND: 
        
           		return wmi.info.wl.egl_window; 
        
           #endif 
        
           #if defined(SDL_VIDEO_DRIVER_ANDROID) 
        
           	case SDL_SYSWM_ANDROID: 
        
           		return wmi.info.android.window; 
        
           #endif 
        
           	default: 
        
           		return nullptr; 
        
           	} 
        
           } 
        
           #endif // OSD_SDL

actually used? I put a breakpoint at

mame/src/osd/modules/render/drawbgfx.cpp

Line 497 in 36f5b6e

SDL_SysWMinfo wmi;

but it seems not to get hit.
If this code is not being used, making the change cleaner would be much simpler. Thanks!

…ve Wayland one

belegdol · 2023-08-19T18:56:16Z

I moved the update to the 3rdparty libraries to a separate PR (#11493) for clarity.

It’s still showing as having all those 3rdparty files changed (1063 files changed).

Apologies. This is either me failing at github or github not being able to create a pr based on another PR. If I understand correctly, it would work if the b* libs update could be temporarily put in a mamedev branch. Alternatively I could rebase against master but this would make testing slightly more complicated.

I rebased onto master, will deal with the dependencies locally.

belegdol · 2023-08-20T07:43:34Z

@cuavas, is this better style-wise? Also, if sdlNativeWindowHandle() is actually needed, which namespace would be the best to put create_wl_egl_window() into?

cuavas · 2023-08-21T17:01:32Z

Apologies if it is a dumb question, but is the code in

mame/src/osd/modules/render/drawbgfx.cpp

Lines 490 to 528 in 36f5b6e

//============================================================

// helper for getting native platform window

//============================================================

#ifdef OSD_SDL

static void *sdlNativeWindowHandle(SDL_Window *window)

{

SDL_SysWMinfo wmi;

SDL_VERSION(&wmi.version);

if (!SDL_GetWindowWMInfo(window, &wmi))

return nullptr;

switch (wmi.subsystem)

{

#if defined(SDL_VIDEO_DRIVER_WINDOWS)

case SDL_SYSWM_WINDOWS:

return wmi.info.win.window;

#endif

#if defined(SDL_VIDEO_DRIVER_X11)

case SDL_SYSWM_X11:

return (void *)uintptr_t(wmi.info.x11.window);

#endif

#if defined(SDL_VIDEO_DRIVER_COCOA)

case SDL_SYSWM_COCOA:

return wmi.info.cocoa.window;

#endif

#if defined(SDL_VIDEO_DRIVER_WAYLAND) && SDL_VERSION_ATLEAST(2, 0, 16)

case SDL_SYSWM_WAYLAND:

return wmi.info.wl.egl_window;

#endif

#if defined(SDL_VIDEO_DRIVER_ANDROID)

case SDL_SYSWM_ANDROID:

return wmi.info.android.window;

#endif

default:

return nullptr;

}

}

#endif // OSD_SDL

actually used? I put a breakpoint at

mame/src/osd/modules/render/drawbgfx.cpp

Line 497 in 36f5b6e

SDL_SysWMinfo wmi;

but it seems not to get hit.
If this code is not being used, making the change cleaner would be much simpler. Thanks!

It’s used by renderer_bgfx::create if you have multiple output windows/screens. It definitely is needed.

cuavas · 2023-08-21T17:06:43Z

src/osd/modules/render/drawbgfx.cpp

-		platform_data.nwh = wmi.info.wl.egl_window;
+		platform_data.nwh = (void *)create_wl_egl_window(dynamic_cast<sdl_window_info const &>(window).platform_window(), wmi.info.wl.surface);
+		platform_data.type = bgfx::NativeWindowHandleType::Wayland;


Why are you explicitly casting to void *? You don’t seem to be casting away CV qualifiers, and you aren’t doing a dynamic cast to the canonical object pointer. You should be able to implicitly convert to void * if you just want the pointer value to be preserved.

Implicit conversion seems to be working, will fix it.

cuavas · 2023-08-21T17:12:20Z

src/osd/modules/render/drawbgfx.cpp

+#if defined(SDLMAME_USE_WAYLAND)
+wl_egl_window* create_wl_egl_window(SDL_Window *window, struct wl_surface *surface)
+{
+	wl_egl_window *win_impl = (wl_egl_window*)SDL_GetWindowData(window, "wl_egl_window");
+	if(!win_impl)
+	{
+		int width, height;
+		SDL_GetWindowSize(window, &width, &height);
+		if(!surface)
+			return nullptr;
+		win_impl = wl_egl_window_create(surface, width, height);
+		SDL_SetWindowData(window, "wl_egl_window", win_impl);
+	}
+	return win_impl;
+}
+#endif


Please follow the formatting of the file:

Space before but not after right-associative unary *

Space between flow control keyword and opening parenthesis of controlling expression

Why are you checking surface after getting the window size? You won’t use the window size if it’s null.

Can wl_egl_window_create fail?

Unlike the other cases in the switch (wmi.subsystem) this can fail. However, you aren’t checking for failure and returning false from video_bgfx::set_platform_data when that happens.

Should be fixed now.

cuavas · 2023-08-21T17:12:56Z

src/osd/modules/render/drawbgfx.cpp

+			wl_egl_window *win_impl = (wl_egl_window*)SDL_GetWindowData(window, "wl_egl_window");
+			if(!win_impl)
+			{
+				int width, height;
+				SDL_GetWindowSize(window, &width, &height);
+				struct wl_surface* surface = wmi.info.wl.surface;
+				if(!surface)
+					return nullptr;
+				win_impl = wl_egl_window_create(surface, width, height);
+				SDL_SetWindowData(window, "wl_egl_window", win_impl);
+			}
+			return (void*)win_impl;


This is non-trivial copy/pasta – shouldn’t it be factored out?

- do not cast wayland window pointer implicitly - fix whitespace - add error handling - factor out second instance of wayland window creation

belegdol · 2023-08-22T08:24:48Z

I have attempted to implement your feedback to the best of my abilities. Thanks to your hint about multiple windows, it appears that there is an issue with vulkan on wayland whereby second window is not being shown. It is with the opengl renderer strangely enough. I need to investigate further.
Thank you for your patience so far.

belegdol · 2023-08-22T16:56:45Z

I have attempted to implement your feedback to the best of my abilities. Thanks to your hint about multiple windows, it appears that there is an issue with vulkan on wayland whereby second window is not being shown. It is with the opengl renderer strangely enough. I need to investigate further. Thank you for your patience so far.

Multiple windows needs to be fixed in bgfx:
bkaradzic/bgfx#3162

cuavas · 2023-08-23T18:20:12Z

src/osd/modules/render/drawbgfx.cpp

+wl_egl_window *create_wl_egl_window(SDL_Window *window, struct wl_surface *surface)
+{
+	if (!surface)
+		{
+			osd_printf_error("Wayland surface missing, aborting\n");
+			return nullptr;
+		}
+	wl_egl_window *win_impl = (wl_egl_window *)SDL_GetWindowData(window, "wl_egl_window");
+	if (!win_impl)
+	{
+		int width, height;
+		SDL_GetWindowSize(window, &width, &height);
+		win_impl = wl_egl_window_create(surface, width, height);
+		if (!win_impl)
+			{
+				osd_printf_error("Creating wayland window failed\n");
+				return nullptr;
+			}
+		SDL_SetWindowData(window, "wl_egl_window", win_impl);
+	}
+	return win_impl;
+}


Indentation is off in two places – you’ve bumped two levels for some of the if statements. The braces should be at the level of the if statement itself.

cuavas · 2023-08-23T18:21:16Z

src/osd/modules/render/drawbgfx.cpp

 		platform_data.ndt = wmi.info.wl.display;
-		platform_data.nwh = wmi.info.wl.egl_window;
+		platform_data.nwh = create_wl_egl_window(dynamic_cast<sdl_window_info const &>(window).platform_window(), wmi.info.wl.surface);
+		platform_data.type = bgfx::NativeWindowHandleType::Wayland;


Because create_wl_egl_window can fail (unlike the other branches of this switch statement), you need to check whether it succeeded here, and return false if it didn’t.

Should be fixed now. Is it also needed for sdlNativeWindowHandle()?

Should be fixed now. Is it also needed for sdlNativeWindowHandle()?

It is, but the function would need to be rearranged a bit. I’ll worry about that later.

- more whitespace fixes - more failure handling

cuavas · 2023-09-11T18:25:58Z

src/osd/modules/render/drawbgfx.cpp

-		platform_data.nwh = wmi.info.wl.egl_window;
+		platform_data.nwh = create_wl_egl_window(dynamic_cast<sdl_window_info const &>(window).platform_window(), wmi.info.wl.surface);
+		if (!platform_data.nwh)
+		{
+			osd_printf_error("BGFX: Error creating a Wayland window\n");
+			return false;
+		}
+		platform_data.type = bgfx::NativeWindowHandleType::Wayland;


Should the rest of the branches in this switch statement be setting platform_data.type to bgfx::NativeWindowHandleType::Default to be explicit? The bgfx examples all seem to be doing it now.

platform_data.type is supposed initialised to default in bgfx.cpp:

mame/3rdparty/bgfx/src/bgfx.cpp

Line 3425 in e6a7b3c

, type(NativeWindowHandleType::Default)

The reason why examples are doing it explicitly is because they all go via an intermediate getNativeWindowHandleType() function which has to return something for non-Wayland. mame appears to be setting the parameters directly which would suggest setting the window handle to default would be redundant.

cuavas · 2023-09-28T15:16:22Z

@belegdol can this be marked as ready for review now that the bgfx changes have been merged, or are there other things that would hold it up?

belegdol · 2023-09-28T15:23:10Z

No other things holding up, just wanted to make sure things are merged in the correct order.

belegdol · 2023-09-28T15:27:32Z

Thanks for merging!

belegdol force-pushed the bgfx_wayland branch from 283523b to 3b42c06 Compare August 2, 2023 14:07

cuavas reviewed Aug 2, 2023

View reviewed changes

belegdol mentioned this pull request Aug 18, 2023

Update bgfx, bx and bimg to current upstream versions #11493

Merged

belegdol force-pushed the bgfx_wayland branch from e295aed to 3be21ef Compare August 18, 2023 19:51

belegdol changed the title ~~Update BGFX, BX and BIMG and add initial support for wayland to bgfx renderer~~ Add initial support for wayland to bgfx renderer Aug 18, 2023

belegdol force-pushed the bgfx_wayland branch from 3be21ef to 0414e06 Compare August 18, 2023 20:25

belegdol added 5 commits August 19, 2023 20:54

Allow bgfx on wayland

51ba341

Initial working prototype for bgfx on wayland

7a0a5aa

Only build wayland support when requested

63f382f

Do not cast win_impl through uintptr_t

8542812

Use platform_data.type to relay information about window being a nati…

0db5852

…ve Wayland one

belegdol force-pushed the bgfx_wayland branch from 0414e06 to 0db5852 Compare August 19, 2023 18:55

Move wayland window creation to a separate function

2affa46

cuavas reviewed Aug 21, 2023

View reviewed changes

Implement PR feedback:

5474221

- do not cast wayland window pointer implicitly - fix whitespace - add error handling - factor out second instance of wayland window creation

cuavas reviewed Aug 23, 2023

View reviewed changes

Implement PR feedback:

8d08909

- more whitespace fixes - more failure handling

belegdol force-pushed the bgfx_wayland branch from 0c387e1 to 8d08909 Compare September 10, 2023 13:51

belegdol mentioned this pull request Sep 10, 2023

Cherry pick upstream bgfx commits needed to get wayland working #11539

Merged

cuavas reviewed Sep 11, 2023

View reviewed changes

belegdol marked this pull request as ready for review September 28, 2023 15:22

cuavas merged commit 117c384 into mamedev:master Sep 28, 2023
5 checks passed

belegdol deleted the bgfx_wayland branch September 28, 2023 15:26

belegdol mentioned this pull request Sep 29, 2023

crash when using bgfx and wayland #5693

Closed

Add initial support for wayland to bgfx renderer #11451

Add initial support for wayland to bgfx renderer #11451

Conversation

belegdol commented Jul 30, 2023 • edited Loading

belegdol commented Aug 2, 2023

belegdol commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belegdol commented Aug 3, 2023 • edited Loading

belegdol commented Aug 11, 2023 • edited Loading

belegdol commented Aug 18, 2023

cuavas commented Aug 19, 2023

belegdol commented Aug 19, 2023

belegdol commented Aug 19, 2023 • edited Loading

belegdol commented Aug 19, 2023

belegdol commented Aug 20, 2023 • edited Loading

cuavas commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belegdol commented Aug 22, 2023

belegdol commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuavas Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuavas commented Sep 28, 2023

belegdol commented Sep 28, 2023

belegdol commented Sep 28, 2023

belegdol commented Jul 30, 2023 •

edited

Loading

belegdol commented Aug 3, 2023 •

edited

Loading

belegdol commented Aug 11, 2023 •

edited

Loading

belegdol commented Aug 19, 2023 •

edited

Loading

belegdol commented Aug 20, 2023 •

edited

Loading

cuavas Aug 23, 2023 •

edited

Loading