Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Video Extension functions to support Vulkan #1022

Merged
merged 1 commit into from
Aug 12, 2023

Conversation

Rosalie241
Copy link
Contributor

@Rosalie241 Rosalie241 commented Jul 31, 2023

This introduces the following video extension functions:

VidExt_InitWithRenderMode()
Which allows a front-end to initialize based on a specific render mode (Vulkan or OpenGL),
I left the original VidExt_Init() alone for backwards compatability, and a front-end can easily just call their own VidExt_Init() when the render mode is OpenGL.

VidExt_VK_GetSurface()
Which allows a plugin to retrieve a vulkan surface to render to, note that it's a pointer to void*, that's due to the fact that VkSurfaceKHR is a opaque handle, but since including the vulkan headers seems wasteful, It makes sense to use a void* instead, the plugin and front-end can do the casting themselves.

VidExt_VK_GetInstanceExtensions()
Which allows a plugin to retrieve the supported vulkan extensions, note that it's upto the vidext implementation to create a const char* array and pass that back with it's size.

Ping @loganmc10 because these changes are currently untested, but I feel like this is enough for a front-end to support both OpenGL and Vulkan plugins.

@Rosalie241 Rosalie241 force-pushed the vidext_vulkan branch 2 times, most recently from c05b4ee to 875c88b Compare July 31, 2023 22:45
@loganmc10
Copy link
Member

Yes, this is very similar to what I currently do in simple64, but I would say this:

  • Rather than using uint64_t* for the VkSurface, I would just use void* (this is what I do in simple64). Rather than try to approximate the pointer, since it's not the proper type anyway, it's better to just make it void*, it's going to get cast anyway on the frontend side.
  • For VidExt_VK_GetInstanceExtensions, I use const char** Extensions [], not const char** Extensions. You can see how I use it with parallel-rdp here and here

@loganmc10
Copy link
Member

The difference between const char** Extensions and const char** Extensions[] is a matter of ownership.

const char** Extensions is "a list of strings", whereas const char** Extensions[] is "a pointer to a list of strings".

const char** Extensions would mean that the video plugin would need to allocate the memory for that list, and the frontend (vidext) would just populate it.

const char** Extensions[] would mean that the frontend (vidext) needs to allocate the memory, and the video plugin is just getting a pointer to the list.

Because the video plugin doesn't know how big the list will be (it doesn't know the number of extensions), it can't initialize the array/list. It makes sense to pass a pointer, and then have the frontend (vidext) create and populate the list, passing back the length of the list in the NumExtensions variable

@Rosalie241
Copy link
Contributor Author

Rosalie241 commented Aug 1, 2023

Rather than using uint64_t* for the VkSurface, I would just use void* (this is what I do in simple64). Rather than try to approximate the pointer, since it's not the proper type anyway, it's better to just make it void*, it's going to get cast anyway on the frontend side.

The function is used differently than what you have in simple64,
i.e in my proposal it'll be used like

VkSurfaceKHR surface;
VkInstance instance;
VidExt_VK_GetSurface((uint64_t*)&surface, (uint64_t)instance);

Instead of the code you have now:

VkInstance instance;
VkSurfaceKHR surface = (VkSurfaceKHR)VidExt_GetVkSurface((void*)instance);

For VidExt_VK_GetInstanceExtensions, I use const char** Extensions [], not const char** Extensions. You can see how I use it with parallel-rdp here and here

That makes sense, I'll change it.

@loganmc10
Copy link
Member

Oh, I see, I mean to me it makes sense that the vksurface is the return value for the function, since it is created by the function, whereas the instance is an input for the function. But either way would work.

But regardless, they should both be void* (I guess void** for the surface if you pass it in), since they are both opaque handles. The only reason they are uint64_t on your system is because pointers on your system are 64 bits. On a 32 bit system they would be 32 bit pointers.

@Rosalie241
Copy link
Contributor Author

makes sense, changed it to void** and void*

@loganmc10
Copy link
Member

LGTM

@Narann
Copy link
Member

Narann commented Aug 1, 2023

LGTM too. Would wait for @richard42 to give final inputs.

@Mastergatto
Copy link
Contributor

If I may suggest...I would just rename VidExt_Init2() to something like VidExt_InitVk(), so you get a more descriptive name of the function.

@loganmc10
Copy link
Member

loganmc10 commented Aug 1, 2023

I think it makes sense, it's not Vulkan specific, it allows you to choose the rendering mode (currently OpenGL or Vulkan, but it leaves it open for others in the future)

It could maybe be something like VidExt_InitWithRenderMode or VidExt_InitWithMode just like there is currently VidExtFuncSetMode and VidExtFuncSetModeWithRate

@Mastergatto
Copy link
Contributor

Ah right, it should be better this way.

@Rosalie241 Rosalie241 force-pushed the vidext_vulkan branch 4 times, most recently from 37fe043 to cf7781c Compare August 2, 2023 20:35
@Rosalie241
Copy link
Contributor Author

I've renamed VidExt_Init2 to VidExt_InitWithRenderMode as suggested, and I've implemented the vulkan video extension functions for the default SDL implementation, which I've tested and it works, it requires SDL 2.0.6 at the very least and isn't supported on macos due to mac not supporting vulkan.

@loganmc10
Copy link
Member

You've gone above and beyond @Rosalie241 , good work!

@Rosalie241 Rosalie241 force-pushed the vidext_vulkan branch 3 times, most recently from bbaf548 to af287ee Compare August 3, 2023 10:32
@richard42 richard42 merged commit 15f38a1 into mupen64plus:master Aug 12, 2023
9 checks passed
cmitu added a commit to cmitu/RetroPie-Setup that referenced this pull request Oct 19, 2023
The Vulkan API can be used by video plugins and front-end, but we don't currently ship any such components.
Added upstream in mupen64plus/mupen64plus-core#1022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants