You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reported in version: HG 2.1 Reported for operating system, platform: Other, x86
Comments on the original bug report:
On 2017-02-26 12:33:52 +0000, Mark Callow wrote:
I planned to post this on the forum but the forum is broken. Both searches and clicking "submit" after completing the registration form result in an empty white screen. I'm using Firefox 51.0.1 on macOS 10.12.3.
I want to add support on macOS and iOS for creating a window that can use Metal. This needs an NSView object backed by a CALayer instance of type CAMetalLayer. So the change would involve a new flag to be set prior to creating a window and modifying the window creation code to respect the flag.
Is this something you would consider including in standard SDL? I realize Metal is not portable but, actually, the reason I want to do this is portability for my Vulkan applications. I would use MoltenVK which is Vulkan on Metal. Using Vulkan would require the new flag and, a possibly modified, SDL_vulkan.[ch] as found on GitHub.
MoltenVK is not FOSS but anyone can download the trial version and use it to build stuff. A license is required only if you want to distribute applications built with it. This is enforced by it drawing a watermark on the output.
On 2017-03-05 17:33:06 +0000, Alex Szpakowski wrote:
Thanks for the example. Is there a similar one for Mac?
This is great. It means the support is much less intrusive to SDL than I thought. I hadn't realized it was possible to add the CAMetalLayer backed view after window creation.
This is useful code to include in SDL together with a modified https://github.com/corngood/SDL_vulkan/blob/master/src/SDL_vulkan.c that supports Linux, Windows, Mac, iOS & Android. Currently it only supports Linux. I have a modified version that I will be sending a pull request to @corngood about in the near future.
Ultimately I can post a patch here, if there is interest in including it in standard SDL.
On 2017-03-08 01:35:09 +0000, Ryan C. Gordon wrote:
(We're having forum issues, we're working on it, sorry!)
I had an initial shot at a Metal target for the SDL_Render* APIs, posted here:
It needs work. I didn't fully understand how Objective-C wants to retain/release objects at the time, so there are obvious bugs in there, but it sort of works!
If nothing else, it shows how to plug this into an SDL window.
--ryan.
On 2017-03-12 08:49:35 +0000, Mark Callow wrote:
Thanks to Alex and Ryan I now have code for both iOS (UIKit) and macOS (Cocoa) to create Vulkan surfaces using MoltenVK on Metal.
The iOS version appears to be working. Initialization is successful but MoltenVK aborts while loading a texture so I can't yet do any thorough testing. I am stymied for testing on Mac because my MacBook is too old to support Metal.
Once I have more confidence that everything is working, I'll integrate it into my clone of SDL and send a patch.
On 2017-03-15 10:49:08 +0000, Mark Callow wrote:
I'm trying to get this working. I have 2 problems.
The SDL window resize event, which come from SDL_uikitviewcontroller, reports the size in points rather than actual size. For GL contexts one can call SDL_GL_GetDrawableSize to get the actual size (only needed when ALLOW_HIGH_DPI is set). I am still trying to find a way to deal with this when supporting Metal via the simple subview approach from Alex.
I don't seem to be getting the animation callback once the subview has been added. I'm using SDL_iPhoneSetAnimationCallback.
Any hints are welcome.
On 2017-03-16 10:25:21 +0000, Mark Callow wrote:
I resolved issue # 1 in comment # 5 by not setting the contentScaleFactor to nativeScale on the Metal view so the drawable size is now also in points. I've actual added code to set the size according to the setting of SDL_WINDOW_ALLOW_HIGHDPI as in SDL_uikitopenglview. I've also filed bug # 3607. If the conclusion to that bug is that events and SDL_WindowSize should always report the size in points, I'm going to have to add an API call for Vulkan/Metal uses to get either the scale or drawable size.
For issue # 2, I'm currently stuck as I know nothing about UIKit at this level. I am using the same application core as in my GL apps (it's a library) where the animation callback works. Even if I don't [view addSubview:metalview]; the CAMetalLayer backed subview to the SDL window's SDL_uikitview, the animation callback is not called. SDL_iPhoneSetAnimationCallback is not called until long after the subview has been created and added so this is not surprising but I wanted to try.
I am wondering if there is something in SDL_uikitopenglview that enables animation callbacks to work that I also need in my SDL_metalview.
For now I've changed to using a loop to call my drawFrame() and everything is drawing correctly which is pretty awesome. Vulkan API->MoltenVK->Metal in an SDL_Window. I have the same Vulkan app running with SDL on Linux too.
On 2017-03-16 10:52:08 +0000, Mark Callow wrote:
Very red face. I have fixed issue # 2. I was calling SDL_GL_GetCurrentWindow(), to get the window I was passing to SDL_iPhoneSetAnimationCallback, which was of course returning NULL.
Now I'm ready to integrate my work into libSDL2. Hope we can quickly resolve bug # 3607. I should be able to send a preliminary patch soon.
On 2017-03-17 02:58:35 +0000, Alex Szpakowski wrote:
(In reply to Mark Callow from comment # 5)
I'm trying to get this working. I have 2 problems.
The SDL window resize event, which come from SDL_uikitviewcontroller,
reports the size in points rather than actual size. For GL contexts one can
call SDL_GL_GetDrawableSize to get the actual size (only needed when
ALLOW_HIGH_DPI is set). I am still trying to find a way to deal with this
when supporting Metal via the simple subview approach from Alex.
The subview approach already solves this by using the layoutSubviews callback to resize its contents based on the bounds multiplied by the native scale factor. The result is dimensions in pixels.
On 2017-03-17 03:00:50 +0000, Alex Szpakowski wrote:
In other words, no modifications to SDL are needed as all the required information is readily available by using the proper callbacks (as seen in my example). :)
On 2017-03-20 20:08:52 +0000, wrote:
I've made a more extensive implementation for vulkan that mirrors tizen's implementation. Modification of SDL is necessary because we want to disable the creation of egl and opengl es contexts because they conflict with vulkan on android and possibly wayland/mir. I've added the platform independent portion and an implementation of the platform dependant portion for x11 using both xcb and xlib. I've also implemented a rudimentary test program for vulkan on SDL -- it just clears the screen to a color that changes with time. I put the patches in a github gist because they are too big for the mailing list: https://gist.github.com/programmerjake/71ae02e439ff20dca3b1bca6779211f8
I also have a backported version for sdl 2.0.5: https://github.com/programmerjake/SDL/tree/vulkan-support-2.0.5
On 2017-04-07 04:47:37 +0000, Mark Callow wrote:
Created attachment 2712
Patch adding Vulkan surface creation for all platforms supporting Vulkan
Adds SDL_Vulkan_CreateSurface and support functions for all video drivers that support Vulkan: Android, UIKit, Cocoa, X11, Mir, Wayland, Windows. For Cocoa & UIKit you will need to install MoltenVK (Vulkan on Metal), https://moltengl.com/moltenvk/.
You will need a Vulkan library or MoltenVK installed in order to build shared library versions of SDL and, of course, applications using Vulkan.
For Cocoa & UIKit it is necessary to create Metal-backed layers. That code is included but I've made no attempt to expose it outside SDL as such platform specific code is against the spirit.
I've tested Linux/X11, Cocoa and UIKit. It builds on Windows but I do not have suitable hardware to run my Vulkan app. I have not tested Android, Mir or Wayland builds but the code is trivial, half-a-dozen lines each.
I have integrated this code into my fork of SDL at https://github.com/msc-/SDL. See the README there for sample code and a link to a working example.
This code will have to be integrated with the efforts to provide an SDL renderer based on Vulkan.
On 2017-04-07 04:53:27 +0000, Mark Callow wrote:
(In reply to Alex Szpakowski from comment # 8)
(In reply to Mark Callow from comment # 5)
The subview approach already solves this by using the layoutSubviews
callback to resize its contents based on the bounds multiplied by the native
scale factor. The result is dimensions in pixels.
I found that the size reported by the SDL's window resized event and by GetWindowSize is still in points. Also the response to # 3607 indicates this is intentional. So I added an SDL_Vulkan_GetDrawableSize() function.
On 2017-04-07 04:58:05 +0000, Mark Callow wrote:
(In reply to Mark Callow from comment # 11)
You will need a Vulkan library or MoltenVK installed in order to build
shared library versions of SDL and, of course, applications using Vulkan.
You also need vulkan.h to build any version of the library, if the Vulkan surface support is configured in the build. Applications do not need it, unless they are using Vulkan.
BTW, the new functions are documented in include/SDL_vulkan.h.
On 2017-04-07 05:02:54 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 10)
Modification of SDL is necessary because we want to disable
the creation of egl and opengl es contexts because they conflict with vulkan
on android and possibly wayland/mir.
Be careful not to make this a blanket disable. Some implementations, most notably NVIDIA, support inter-operation of OpenGL & Vulkan.
In my case modification to SDL is necessary because of the necessary Metal support for Cocoa and UIKit.
On 2017-04-07 05:12:13 +0000, Mark Callow wrote:
Created attachment 2713
Revised patch fixing a typo in Wayland code.
On 2017-04-07 09:20:23 +0000, wrote:
(In reply to Mark Callow from comment # 14)
(In reply to programmerjake from comment # 10)
Modification of SDL is necessary because we want to disable
the creation of egl and opengl es contexts because they conflict with vulkan
on android and possibly wayland/mir.
Be careful not to make this a blanket disable. Some implementations, most
notably NVIDIA, support inter-operation of OpenGL & Vulkan.
My code doesn't currently support opengl and vulkan in the same window, however it does not need to have the user-visible api changed to accomodate that, you could just pass both the opengl and vulkan flags to SDL_CreateWindow.
Also, I looked at some of your code, I think it would be good to duplicate the api used in tizen's fork of libsdl, as I have done.
On 2017-04-07 12:01:26 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 16)
Also, I looked at some of your code, I think it would be good to duplicate
the api used in tizen's fork of libsdl, as I have done.
What API are you talking about? We both have SDL_Vulkan_GetInstanceExtensions and SDL_Vulkan_CreateSurface. There are slight differences in signature of the former but nothing significant.
I knew nothing about the Tizen work until today. Where is this fork?
On 2017-04-07 12:16:34 +0000, Mark Callow wrote:
@programmerjake why is it necessary to create SDL clones of standard Vulkan types such as VkInstance and VkSurfaceKHR? My code is successfully working across 4 platforms without them.
Note that Xlib Vulkan surfaces are not widely supported. That is why my code always uses xcb surfaces, via X11-xcb.
I don't like having to call GetProcAddr for all the functions I am using. It is a lot of extra work for the application compared with #include <vulkan/vulkan.h> and truly is not necessary. libvulkan on all platforms exports all the standard functions. Only actual extensions need to have their addresses queried.
I wrote some more extensive documentation based off of reading tizen's code in the doxygen comments in my SDL_video.h
I wrote the code to load everything dynamically so you can have an executable be able to fall back to a different rendering system when you don't have the vulkan loader shared object installed.
The code I have written doesn't need any extra headers or anything to be able to build.
Note that I changed the type used for SDL_vulkanSurface because the type used in tizen is not always 64 bits. It doesn't matter that much because it will always be casted to a VkSurfaceKHR before being accessed.
On 2017-04-08 09:03:29 +0000, wrote:
(In reply to Mark Callow from comment # 18)
Note that Xlib Vulkan surfaces are not widely supported. That is why my code
always uses xcb surfaces, via X11-xcb.
Not all versions of Xlib support xcb, so, assuming someone wants to use a vulkan driver with an old linux distribution, like RHEL 4 (not sure about this, was initially released before xlib-xcb), we should use Xlib if it's available, otherwise fallback to xcb and xlib-xcb.
On 2017-04-10 04:30:50 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 19)
I wrote some more extensive documentation based off of reading tizen's code
in the doxygen comments in my SDL_video.h
Yes I know. I read your documentation. The only differences between your API and mine are:
Your SDL_Vulkan_GetInstanceExtensions returns a list that
includes VK_KHR_Surface.
The return types of this function are different. Yours
returns SDL_bool mine returns the count of extensions
or 0 for error, so does not need the int* parameter.
Your SDL_Vulkan_CreateSurface returns SDL_bool mine returns
0 on success or -1 on error like several other API functions
in SDL_video.
Mine does not add an SDL_VULKAN_WINDOW flag. The same errors can
be checked for. It's just done in the SDL_Vulkan functions.
That is why I am not sure I understand your comment about duplicating the Tizen API, because I basically do have the same API.
I have no issues with following you on no 1. I prefer my no. 2. I could go either way on no. 3. I don't see the need for no. 4.
I wrote the code to load everything dynamically so you can have an
executable be able to fall back to a different rendering system when you
don't have the vulkan loader shared object installed.
That makes sense if there is a version of the SDL renderer that uses Vulkan and for providing binaries of an SDL library that will work regardless of whether or not Vulkan is installed.
I put my API functions their own source file so that applications linking with a static library version of SDL but not using Vulkan do not need to link with Vulkan. Dynamic loading has the same result.
The only issue I have with this is your note in the dynamic loader documentation:
If you do this, you need to retrieve all of the Vulkan functions
used in your program from the dynamic library using \c
SDL_Vulkan_GetVkGetInstanceProcAddr().
I believe it is important that applications be able to link with libvulkan and resolve the symbols at link time.
BTW, SDL_Vulkan_GetVkGetInstanceProcAddr() provides no added value as VkGetInstanceProcAddress is part of the core Vulkan API. This is unlike OpenGL where each platform is different making it a nightmare from which SDL_GL_GetProcAddr() rescues you.
The code I have written doesn't need any extra headers or anything to be
able to build.
Mine doesn't require them for applications. It doesn't seem overly burdensome
to require a vulkan.h be available when building the library. The SDK is readily available. If you don't use it, there is a possibility of errors in the extension name strings that you are duplicating. Plus your code seems a lot more complex than mine in part because you are re-declaring a lot of stuff from vulkan.h.
Note that I changed the type used for SDL_vulkanSurface because the type
used in tizen is not always 64 bits. It doesn't matter that much because it
will always be casted to a VkSurfaceKHR before being accessed.
Neither is it in Vulkan. vulkan.h supports compilation for both 32- & 64-bit systems and VkSurfaceKHR will be defined appropriately.
Not all versions of Xlib support xcb, so, assuming someone wants to
use a vulkan driver with an old linux distribution, like RHEL 4 ...
It is extremely unlikely that Vulkan drivers will ever be available for such old distributions. The earliest Ubuntu for which they are available is Xenial and they were not in the distribution. They are in Yakkety.
Nevertheless it is not much of a burden to try both.
On 2017-04-10 04:36:33 +0000, Mark Callow wrote:
I based my work on that of David McFarland which I linked to in comment # 2. I don't know who was first, him or Tizen.
On 2017-04-10 07:59:49 +0000, Mark Callow wrote:
(In reply to Mark Callow from comment # 21)
That makes sense if there is a version of the SDL renderer that uses Vulkan
and for providing binaries of an SDL library that will work regardless of
whether or not Vulkan is installed.
I have to make a small correction. An SDL so,dylib,dll linked with libvulkan would be okay regardless of whether a Vulkan driver is installed on the system. VkCreateInstance will simply fail with an error indicating an incompatible driver.
On 2017-04-10 09:09:49 +0000, wrote:
(In reply to Mark Callow from comment # 21)
This is a partial reply for now, will reply in more detail later.
Mine does not add an SDL_VULKAN_WINDOW flag. The same errors can
be checked for. It's just done in the SDL_Vulkan functions.
SDL_VULKAN_WINDOW is needed because on android, and possibly other systems, the opengl/egl context is made when you create the window, not when you call SDL_GL_createcontext.
That is why I am not sure I understand your comment about duplicating the
Tizen API, because I basically do have the same API.
Your api is similar but not identical. I wanted to have an identical api for those porting from tizen's fork to sdl mainline.
The only issue I have with this is your note in the dynamic loader
documentation:
If you do this, you need to retrieve all of the Vulkan functions
used in your program from the dynamic library using \c
SDL_Vulkan_GetVkGetInstanceProcAddr().
I should correct the documentation for that, it's the equivalent to using SDL_LoadObject. As far as I know, you can both link dynamically and use the get function pointer functions simultaneously. the getvkgetinstanceprocaddr function returns the address from the shared object for when you don't want to link to libvulkan.so in order to run the program on a system that doesn't have libvulkan.so installed.
BTW, SDL_Vulkan_GetVkGetInstanceProcAddr() provides no added value as
VkGetInstanceProcAddress is part of the core Vulkan API. This is unlike
OpenGL where each platform is different making it a nightmare from which
SDL_GL_GetProcAddr() rescues you.
it returns the fn ptr which would not be otherwise available when loading libvulkan.so programmatically instead of linking to it.
Note that I changed the type used for SDL_vulkanSurface because the type
used in tizen is not always 64 bits. It doesn't matter that much because it
will always be casted to a VkSurfaceKHR before being accessed.
Neither is it in Vulkan. vulkan.h supports compilation for both 32- & 64-bit
systems and VkSurfaceKHR will be defined appropriately.
check again, notice that VkSurfaceKHR is only a pointer on 64-bit systems, otherwise it's a uint64_t.
On 2017-04-10 09:56:59 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 24)
(In reply to Mark Callow from comment # 21)
Neither is it in Vulkan. vulkan.h supports compilation for both 32- & 64-bit
systems and VkSurfaceKHR will be defined appropriately.
check again, notice that VkSurfaceKHR is only a pointer on 64-bit systems,
otherwise it's a uint64_t.
That's true for all non-dispatchable handles. It is still an opaque handle. And it does not invalidate what I wrote, unless you disagree with "appropriately". But uint64_t is what the Vulkan working group decided it should be.
If Tizen is returning something other than a 64-bit quantity for a VkSurfaceKHR they are not conformant and should not be calling their API Vulkan.
On 2017-04-12 23:03:58 +0000, Mark Callow wrote:
I want to point out again that my patch supports UIKit and Cocoa as well as Linux, etc. I am worried that someone may just look at the list I wrote in comment 21 and miss this. That list is about API not functional differences. Whatever shape Sam decides for the API, I want the UIKit & Cocoa functionality included in main line.
(In reply to programmerjake from comment # 24)
SDL_VULKAN_WINDOW is needed because on android, and possibly other systems,
the opengl/egl context is made when you create the window, not when you call
SDL_GL_createcontext.
This just changes where you can report an error that a Window can't be both OpenGL & Vulkan. With the flag you can report it at Window creation. Without it, you report it at Vulkan surface creation.
Your api is similar but not identical. I wanted to have an identical api for
those porting from tizen's fork to sdl mainline.
I see. As I said, I was unaware of the Tizen work. I tried to follow similar existing SDL API functions.
On 2017-04-12 23:31:49 +0000, wrote:
(In reply to Mark Callow from comment # 26)
(In reply to programmerjake from comment # 24)
SDL_VULKAN_WINDOW is needed because on android, and possibly other systems,
the opengl/egl context is made when you create the window, not when you call
SDL_GL_createcontext.
This just changes where you can report an error that a Window can't be both
OpenGL & Vulkan. With the flag you can report it at Window creation. Without
it, you report it at Vulkan surface creation.
It's more than just that: currently, if you don't pass the SDL_OPENGL_WINDOW flag, it's added automatically on android. The opengl context is created before you tell SDL that you're using or not using opengl. We need the SDL_VULKAN_WINDOW flag to disable the creation of the opengl context at window creation time.
On 2017-04-12 23:41:53 +0000, wrote:
(In reply to Mark Callow from comment # 26)
If you add the SDL_VULKAN_WINDOW flag so we don't have to change the API again later and document somewhere that your code probably won't work on android as it is, I'd be fine with adding your code to libsdl.
On 2017-04-13 10:02:16 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 27)
It's more than just that: currently, if you don't pass the SDL_OPENGL_WINDOW
flag, it's added automatically on android.
Oh! I didn't know that. Thanks.
If you add the SDL_VULKAN_WINDOW flag so we don't have to change
the API again later and document somewhere that your code probably
won't work on android as it is, I'd be fine with adding your code
to libsdl.
I'll do that and I'll also change my SDL_Vulkan_GetInstanceExtensions to include VK_KHR_Surface. Don't you have a modified Android CreateWindow already that can be included so Android will work?
Give me a few days.
On 2017-04-13 16:26:53 +0000, wrote:
(In reply to Mark Callow from comment # 29)
I'll do that and I'll also change my SDL_Vulkan_GetInstanceExtensions to
include VK_KHR_Surface. Don't you have a modified Android CreateWindow
already that can be included so Android will work?
No, I don't have a way to test vulkan on android, so I've mostly given up on implementing the code for android as it's nontrivial.
Sorry.
On 2017-04-18 07:14:37 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 30)
No, I don't have a way to test vulkan on android, so I've mostly given up on
implementing the code for android as it's nontrivial.
Sorry.
I'm in the same boat. Neither my Android or Windows devices can run Vulkan, the latter due to Intel choosing not to rather than device incapability. I can run their open source Vulkan driver under Linux on the same device.
@programmerjake, we should work together on this. For example we should use your dynamic loading code because that is just the way SDL does things. Even with this, applications will be able link to the Vulkan library, if they choose. My OpenGL apps link with OpenGL and work fine. Note that on Windows GetProcAddr has to be used because the OpenGL library there only has OpenGL 2 symbols. Also it is probably good to divide up the SDL_Vulkan_* function implementations across the different drivers rather than the the single ifdef laden file I am using. That was convenient while I was developing the technology.
Do you have a GitHub(?) fork against which I can send pull requests? Or would you like to do send me pull requests against my fork?
On 2017-04-18 07:17:37 +0000, Mark Callow wrote:
About the SDL_VULKAN_WINDOW flag, it looks to me like this is only required because the Android platform has a faulty implementation of SDL_OPENGL_WINDOW. Why does it always create an OpenGL context even if the flag is not specified? What would break if it was changed?
On 2017-04-19 05:35:06 +0000, wrote:
(In reply to Mark Callow from comment # 31)
(In reply to programmerjake from comment # 30)
No, I don't have a way to test vulkan on android, so I've mostly given up on
implementing the code for android as it's nontrivial.
Sorry.
I'm in the same boat. Neither my Android or Windows devices can run Vulkan,
the latter due to Intel choosing not to rather than device incapability. I
can run their open source Vulkan driver under Linux on the same device.
Note that wine-staging has support for vulkan, allowing you to test vulkan on windows from linux.
Do you have a GitHub(?) fork against which I can send pull requests? Or
would you like to do send me pull requests against my fork?
Note that I have some useful utility functions in src/video/SDL_vulkan.h
From what I can tell, on android, the opengl context needs to be created and destroyed in specific places in callbacks from the android framework; I forgot why. It's done in the java part of sdl's implementation.
On 2017-05-20 12:59:54 +0000, Mark Callow wrote:
Mostly for @programmerjake but could use a wider audience so posting here.
In working on merging programmerjake's and my efforts I have recalled one of the reasons why I designed the API I did for SDL_Vulkan_GetInstanceExtensions: robustness.
int SDL_Vulkan_GetInstanceExtensions(SDL_Window* window, unsigned int length, const char** names);
The difference is the length parameter that clearly tells the function how long the names array is. Since you're passing an int, the compiler can tell you if you're passing an uninitialized variable plus it makes you think about the length of the names array you pass. The return value is the extension count.
With the Tizen API, count could be made to do double duty but use of it to limit the writes is not obvious, the compiler won't warn if it's uninitialized and there is a comment in the @programmerjake's code that says Tizen expects the count value when names is not null to be the same value as it returned when called with names == NULL.
I'd prefer a more robust API. How important is it to maintain compatibility with what Tizen did?
I have no idea how many Tizen Vulkan apps there are nor how many will be ported to other SDL platforms.
On 2017-05-20 15:37:18 +0000, Ryan C. Gordon wrote:
Just as a heads up: we added a SDL_WINDOW_VULKAN flag to revision control:
This was to fix an immediate need (Android couldn't use Vulkan at all with SDL, since SDL would set up an EGL context unconditionally). This patch isn't intended to step on anyone working away here.
(I'll take some time and review the work here soon and give some feedback; we've all been so busy in recent times, and I apologize for the neglect!)
--ryan.
On 2017-05-20 19:29:21 +0000, wrote:
(In reply to Mark Callow from comment # 34)
Mostly for @programmerjake but could use a wider audience so posting here.
In working on merging programmerjake's and my efforts I have recalled one
of the reasons why I designed the API I did for
SDL_Vulkan_GetInstanceExtensions: robustness.
int SDL_Vulkan_GetInstanceExtensions(SDL_Window* window, unsigned int
length, const char** names);
The difference is the length parameter that clearly tells the function how
long the names array is. Since you're passing an int, the compiler can tell
you if you're passing an uninitialized variable plus it makes you think
about the length of the names array you pass. The return value is the
extension count.
With the Tizen API, count could be made to do double duty but use of it to
limit the writes is not obvious, the compiler won't warn if it's
uninitialized and there is a comment in the @programmerjake's code that says
Tizen expects the count value when names is not null to be the same value as
it returned when called with names == NULL.
I will note that the tizen source allows you to pass a larger count when names != nullptr, however if count == 0 then it acts as if names == nullptr and modifies count. If count is not 0 and less than returned previously, it writes past the end of the names array.
see https://review.tizen.org/git/?p=platform/upstream/SDL.git;a=blob;f=src/video/tizen/SDL_tizenvulkan.c;h=8aed19440cf708b8171424f77889e17b6d1fbf7e;hb=HEAD#l47
This was to fix an immediate need (Android couldn't use Vulkan at all with
SDL, since SDL would set up an EGL context unconditionally). This patch
isn't intended to step on anyone working away here.
(I'll take some time and review the work here soon and give some feedback;
we've all been so busy in recent times, and I apologize for the neglect!)
Mostly for @programmerjake but could use a wider audience so posting here.
In working on merging programmerjake's and my efforts I have recalled one
of the reasons why I designed the API I did for
SDL_Vulkan_GetInstanceExtensions: robustness.
int SDL_Vulkan_GetInstanceExtensions(SDL_Window* window, unsigned int
length, const char** names);
The difference is the length parameter that clearly tells the function how
long the names array is. Since you're passing an int, the compiler can tell
you if you're passing an uninitialized variable plus it makes you think
about the length of the names array you pass. The return value is the
extension count.
With the Tizen API, count could be made to do double duty but use of it to
limit the writes is not obvious, the compiler won't warn if it's
uninitialized and there is a comment in the @programmerjake's code that says
Tizen expects the count value when names is not null to be the same value as
it returned when called with names == NULL.
I'd prefer a more robust API. How important is it to maintain compatibility
with what Tizen did?
I have no idea how many Tizen Vulkan apps there are nor how many will be
ported to other SDL platforms.
also, I'd expect most users of this code to dynamically allocate an array and to pass the same count variable both times the function is called. Since the variable is initialized by the first call, I'd expect to not really have any uninitialized count variables passed in the second call.
I'd speculate that the tizen api is designed to mirror the vulkan apis for getting a list of things, they all have a pointer to the array of items and a pointer to the count of items. when the items array pointer is null, they write the number of items available to *count. when the items array pointer is not null, they write the lesser of *count or the number of items available to the items array, then assign the number of items written to *count. see vkEnumerateInstanceExtensionProperties in the vulkan spec.
On 2017-05-21 09:48:47 +0000, Mark Callow wrote:
(In reply to programmerjake from comment # 38)
I'd speculate that the tizen api is designed to mirror the vulkan apis for
getting a list of things, they all have a pointer to the array of items and
a pointer to the count of items. when the items array pointer is null, they
write the number of items available to *count. when the items array pointer
is not null, they write the lesser of *count or the number of items
available to the items array, then assign the number of items written to
*count. see vkEnumerateInstanceExtensionProperties in the vulkan spec.
Okay. I will document that this is the way GetInstanceExtensions works and make sure that it does.
On 2017-05-21 09:50:30 +0000, Mark Callow wrote:
(In reply to Mark Callow from comment # 39)
(In reply to programmerjake from comment # 38)
Okay. I will document that this is the way GetInstanceExtensions works and
make sure that it does.
Unless SDL has a pattern for this type of API. If so, should we follow that?
On 2017-05-21 09:53:31 +0000, Mark Callow wrote:
(In reply to Ryan C. Gordon from comment # 35)
Just as a heads up: we added a SDL_WINDOW_VULKAN flag to revision control:
I already pulled this into my fork and working tree where I am doing the merge. Let me know if I should change the value as requested by @programmerjake.
(I'll take some time and review the work here soon and give some feedback;
we've all been so busy in recent times, and I apologize for the neglect!)
Feedback would be welcome but probably best to wait until I send a pull request to @programmerjake for the merged effort.
On 2017-05-21 10:44:58 +0000, wrote:
(In reply to Mark Callow from comment # 39)
(In reply to programmerjake from comment # 38)
I'd speculate that the tizen api is designed to mirror the vulkan apis for
getting a list of things, they all have a pointer to the array of items and
a pointer to the count of items. when the items array pointer is null, they
write the number of items available to *count. when the items array pointer
is not null, they write the lesser of *count or the number of items
available to the items array, then assign the number of items written to
*count. see vkEnumerateInstanceExtensionProperties in the vulkan spec.
Okay. I will document that this is the way GetInstanceExtensions works and
make sure that it does.
You may want to talk to talk to the original author of tizen's implementation about how the api is intended to work: Wonsik, Jung
On 2017-05-21 16:17:27 +0000, Ryan C. Gordon wrote:
Would you be willing to change the value of SDL_WINDOW_VULKAN to 0x10000000
for abi compatibility in tizen:
It's extremely unlikely we'll end up with something ABI compatible when we're done; but if this is the only thing, yes, we'll change it.
--ryan.
On 2017-05-27 09:54:16 +0000, wrote:
(In reply to Ryan C. Gordon from comment # 43)
Would you be willing to change the value of SDL_WINDOW_VULKAN to 0x10000000
for abi compatibility in tizen:
It's extremely unlikely we'll end up with something ABI compatible when
we're done; but if this is the only thing, yes, we'll change it.
On 2017-06-14 12:12:33 +0000, Turo Lamminen wrote:
Your branch fails to compile for me (GCC 4.9.2 on Debian stable 64-bit):
src/video/x11/SDL_x11vulkan.c:73:5: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
for(Uint32 i = 0; i < extensionCount; i++)
On 2017-06-14 20:42:00 +0000, wrote:
(In reply to Turo Lamminen from comment # 46)
Your branch fails to compile for me (GCC 4.9.2 on Debian stable 64-bit):
src/video/x11/SDL_x11vulkan.c:73:5: error: 'for' loop initial declarations
are only allowed in C99 or C11 mode
for(Uint32 i = 0; i < extensionCount; i++)
It should work now.
On 2017-06-22 20:24:49 +0000, wrote:
Did anyone get a chance to review the code yet, or are you planning on waiting till after releasing SDL 2.0.6?
On 2017-07-27 13:15:27 +0000, Turo Lamminen wrote:
It doesn't compile with the Visual Studio 2010 project (as the readme says it should). It's missing files "src/video/SDL_vulkan_utils.c" and "src/video/windows/SDL_windowsvulkan.c"
On 2017-07-27 15:17:40 +0000, Mark Callow wrote:
(In reply to Turo Lamminen from comment # 49)
It doesn't compile with the Visual Studio 2010 project (as the readme says
it should). It's missing files "src/video/SDL_vulkan_utils.c" and
"src/video/windows/SDL_windowsvulkan.c"
I do not have VisualStudio 2010 so was unable to add the new files to that project. Please send us a PR.
The structure of a VS2010 XML project makes it very difficult to add new files by hand editing whereas doing it via VS2010 is a snap.
On 2017-08-09 05:25:34 +0000, Ryan C. Gordon wrote:
(Sorry if you get a lot of copies of this email, we're touching dozens of bug reports right now.)
Tagging a bunch of bugs as target-2.0.6.
This means we're in the final stretch for an official SDL 2.0.6 release! These are the bugs we really want to fix before shipping if humanly possible.
That being said, we don't promise to fix them because of this tag, we just want to make sure we don't forget to deal with them before we bless a final 2.0.6 release, and generally be organized about what we're aiming to ship. After some debate, we might just remove this tag again and deal with it for a later release.
Hopefully you'll hear more about this bug soon. If you have more information (including "this got fixed at some point, nevermind"), we would love to have you come add more information to the bug report when you have a moment.
Thanks!
--ryan.
On 2017-08-11 19:35:38 +0000, Alex Szpakowski wrote:
Regarding the Metal code in the patch, I don't think SDL should internally create a MTLDevice. Doing so should be up to the user of SDL, especially in macOS where there can be multiple MTLDevices corresponding to multiple GPUs, and creating an instance of one can activate the discrete GPU even if the user of SDL wants to use the integrated GPU for their app when there's the option.
On 2017-08-12 01:35:10 +0000, Mark Callow wrote:
(In reply to Alex Szpakowski from comment # 52)
Regarding the Metal code in the patch, I don't think SDL should internally
create a MTLDevice. Doing so should be up to the user of SDL, especially in
macOS where there can be multiple MTLDevices corresponding to multiple GPUs,
and creating an instance of one can activate the discrete GPU even if the
user of SDL wants to use the integrated GPU for their app when there's the
option.
The current code calls MTLCreateSystemDefaultDevice() which "returns a reference to the system preferred device". I have not been able to find any explanation of "system preferred." On multi-GPU systems it is very likely to pick the low-power device, if that supports Metal, especially if the Mac's user has checked automatic graphics switching in system preferences. If it doesn't support Metal, then the Vulkan app will have to use the high-power GPU anyway.
W.r.t Metal, only macOS supports multiple GPUs. On iOS what benefit would be provided by forcing the application to create the Metal device itself?
I do not want people porting their Vulkan apps to be forced to create Mac specific code especially if it means using Obj-C.
How does SDL provide cross-platform support for picking the GPU for OpenGL? Perhaps we can do something similar, if we have to do something.
I will talk to the author of MoltenVK about this. Perhaps we can obtain the selected Metal device from the VkInstance passed to SDL_Vulkan_CreateSurface.
On 2017-08-12 01:49:20 +0000, Alex Szpakowski wrote:
(In reply to Mark Callow from comment # 53)
The current code calls MTLCreateSystemDefaultDevice() which "returns a
reference to the system preferred device". I have not been able to find any
explanation of "system preferred." On multi-GPU systems it is very likely to
pick the low-power device, if that supports Metal, especially if the Mac's
user has checked automatic graphics switching in system preferences. If it
doesn't support Metal, then the Vulkan app will have to use the high-power
GPU anyway.
It picks the high power GPU on my laptop with automatic switching enabled.
I do not want people porting their Vulkan apps to be forced to create Mac
specific code especially if it means using Obj-C.
Ah my bad, I didn't look closely enough and I thought the Metal code in the patch was meant to be more generic than just for MoltenVK.
I'm not really familiar with MoltenVK, but I glanced at GLFW's code (which has MoltenVK support) and didn't see MtlCreateSystemDefaultDevice anywhere. Shouldn't MoltenVK be abstracting that into vkCreateDevice or something?
On 2017-08-12 02:04:37 +0000, Mark Callow wrote:
(In reply to Alex Szpakowski from comment # 54)
It picks the high power GPU on my laptop with automatic switching enabled.
That probably means your low-power GPU doesn't support Metal.
On 2017-08-12 02:07:00 +0000, Alex Szpakowski wrote:
Both my GPUs support Metal. I have an Intel HD 530 and a Radeon Pro 460.
On 2017-08-12 02:35:45 +0000, Alex Szpakowski wrote:
I took a quick look in MoltenVK's sample code and it doesn't explicitly create a MTLDevice anywhere either, so I assume MoltenVK does it via vkCreateDevice or other wrapped Vulkan functions.
On 2017-08-12 15:29:34 +0000, Mark Callow wrote:
(In reply to Alex Szpakowski from comment # 56)
Both my GPUs support Metal. I have an Intel HD 530 and a Radeon Pro 460.
Which GPU do the MoltenVK demos use?
On 2017-08-13 02:32:56 +0000, Alex Szpakowski wrote:
(In reply to Mark Callow from comment # 58)
(In reply to Alex Szpakowski from comment # 56)
Both my GPUs support Metal. I have an Intel HD 530 and a Radeon Pro 460.
Which GPU do the MoltenVK demos use?
The GLFW Vulkan test (using MoltenVK as a Vulkan backend) uses the first device from vkEnumeratePhysicalDevices. That's my Radeon Pro 460. MoltenVK exposes both of my GPUs using that function (the second is my Intel HD 530). I can still run GLFW's demo code if I change it to use the second GPU (and I can verify that it's using the correct GPU by using vulkan's device query APIs).
GLFW's Vulkan backend code and test code doesn't have any MTLDevice code in it at all, it's all handled by the MoltenVK library.
The header documentation for MTLCreateSystemDefaultDevice and MTLCopyAllDevices mentions what I described in my previous posts as well.
On 2017-08-15 16:42:11 +0000, Mark Callow wrote:
I have confirmed with the author of MoltenVK that you do not need to set the device property of CAMetalLayer prior to submitting the UIView/NSView to vkCreateIOSSurfaceMVK() or vkCreateMacOSSurfaceMVK(). The MTLDevice is assigned to the CAMetalLayer within MoltenVK when the swapchain is created.
vkEnumeratePhysicalDevices() enumerates the equivalent MTLDevices on each platform, and an app can use the deviceType element of the properties returned by vkGetPhysicalDeviceProperties() to distinguish the GPU’s on a multi-GPU platform and choose which one to use when creating the VkDevice.
I have submitted a PR, programmerjake/SDL#6, to programmerjake to remove specification of the Metal device. In all it is a very nice simplification.
Thanks to Alex Szpakowski for spotting this problem.
On 2017-08-30 06:55:22 +0000, Sam Lantinga wrote:
This is in for 2.0.6, thanks everyone!
The text was updated successfully, but these errors were encountered:
This bug report was migrated from our old Bugzilla tracker.
These attachments are available in the static archive:
Patch adding Vulkan surface creation for all platforms supporting Vulkan (bug3591.patch, text/plain, 2017-04-07 04:47:37 +0000, 62415 bytes)Reported in version: HG 2.1
Reported for operating system, platform: Other, x86
Comments on the original bug report:
On 2017-02-26 12:33:52 +0000, Mark Callow wrote:
On 2017-03-05 17:33:06 +0000, Alex Szpakowski wrote:
On 2017-03-06 05:06:42 +0000, Mark Callow wrote:
On 2017-03-08 01:35:09 +0000, Ryan C. Gordon wrote:
On 2017-03-12 08:49:35 +0000, Mark Callow wrote:
On 2017-03-15 10:49:08 +0000, Mark Callow wrote:
On 2017-03-16 10:25:21 +0000, Mark Callow wrote:
On 2017-03-16 10:52:08 +0000, Mark Callow wrote:
On 2017-03-17 02:58:35 +0000, Alex Szpakowski wrote:
On 2017-03-17 03:00:50 +0000, Alex Szpakowski wrote:
On 2017-03-20 20:08:52 +0000, wrote:
On 2017-04-07 04:47:37 +0000, Mark Callow wrote:
On 2017-04-07 04:53:27 +0000, Mark Callow wrote:
On 2017-04-07 04:58:05 +0000, Mark Callow wrote:
On 2017-04-07 05:02:54 +0000, Mark Callow wrote:
On 2017-04-07 05:12:13 +0000, Mark Callow wrote:
On 2017-04-07 09:20:23 +0000, wrote:
On 2017-04-07 12:01:26 +0000, Mark Callow wrote:
On 2017-04-07 12:16:34 +0000, Mark Callow wrote:
On 2017-04-08 08:53:55 +0000, wrote:
On 2017-04-08 09:03:29 +0000, wrote:
On 2017-04-10 04:30:50 +0000, Mark Callow wrote:
On 2017-04-10 04:36:33 +0000, Mark Callow wrote:
On 2017-04-10 07:59:49 +0000, Mark Callow wrote:
On 2017-04-10 09:09:49 +0000, wrote:
On 2017-04-10 09:56:59 +0000, Mark Callow wrote:
On 2017-04-12 23:03:58 +0000, Mark Callow wrote:
On 2017-04-12 23:31:49 +0000, wrote:
On 2017-04-12 23:41:53 +0000, wrote:
On 2017-04-13 10:02:16 +0000, Mark Callow wrote:
On 2017-04-13 16:26:53 +0000, wrote:
On 2017-04-18 07:14:37 +0000, Mark Callow wrote:
On 2017-04-18 07:17:37 +0000, Mark Callow wrote:
On 2017-04-19 05:35:06 +0000, wrote:
On 2017-05-20 12:59:54 +0000, Mark Callow wrote:
On 2017-05-20 15:37:18 +0000, Ryan C. Gordon wrote:
On 2017-05-20 19:29:21 +0000, wrote:
On 2017-05-20 19:35:34 +0000, wrote:
On 2017-05-20 19:59:16 +0000, wrote:
On 2017-05-21 09:48:47 +0000, Mark Callow wrote:
On 2017-05-21 09:50:30 +0000, Mark Callow wrote:
On 2017-05-21 09:53:31 +0000, Mark Callow wrote:
On 2017-05-21 10:44:58 +0000, wrote:
On 2017-05-21 16:17:27 +0000, Ryan C. Gordon wrote:
On 2017-05-27 09:54:16 +0000, wrote:
On 2017-06-11 23:13:27 +0000, wrote:
On 2017-06-14 12:12:33 +0000, Turo Lamminen wrote:
On 2017-06-14 20:42:00 +0000, wrote:
On 2017-06-22 20:24:49 +0000, wrote:
On 2017-07-27 13:15:27 +0000, Turo Lamminen wrote:
On 2017-07-27 15:17:40 +0000, Mark Callow wrote:
On 2017-08-09 05:25:34 +0000, Ryan C. Gordon wrote:
On 2017-08-11 19:35:38 +0000, Alex Szpakowski wrote:
On 2017-08-12 01:35:10 +0000, Mark Callow wrote:
On 2017-08-12 01:49:20 +0000, Alex Szpakowski wrote:
On 2017-08-12 02:04:37 +0000, Mark Callow wrote:
On 2017-08-12 02:07:00 +0000, Alex Szpakowski wrote:
On 2017-08-12 02:35:45 +0000, Alex Szpakowski wrote:
On 2017-08-12 15:29:34 +0000, Mark Callow wrote:
On 2017-08-13 02:32:56 +0000, Alex Szpakowski wrote:
On 2017-08-15 16:42:11 +0000, Mark Callow wrote:
On 2017-08-30 06:55:22 +0000, Sam Lantinga wrote:
The text was updated successfully, but these errors were encountered: