GPU API Feedback

Hi! I saw the GPU API push and have a bit of feedback to provide: ## Unsupported vertex formats ```cpp SDL_GPUVERTFMT_CHAR SDL_GPUVERTFMT_UCHAR SDL_GPUVERTFMT_UCHAR3 SDL_GPUVERTFMT_CHAR3 SDL_GPUVERTFMT_USHORT3 SDL_GPUVERTFMT_SHORT3 SDL_GPUVERTFMT_HALF3 ``` Get rid of them (including the NORMALIZED variants). GPU support is flaky for them (or non-existent) and requires expensive emulation, which is hard to get right due to alignment issues. ## Merging format enums? ```cpp typedef enum SDL_GpuVertexFormat typedef enum SDL_GpuPixelFormat ``` The trend is to unify these two; since supported `GpuVertexFormat` is just a subset of `GpuPixelFormat` But personally I'm not decided for either option. They have pros and cons. From a low API perspective, unification makes sense. At a higher level? I don't know. ## Format enum [Vulkan](https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkFormat.html), [DXGI](https://docs.microsoft.com/en-us/windows/win32/api/dxgiformat/ne-dxgiformat-dxgi_format) and [MTLPixelFormat](https://developer.apple.com/documentation/metal/mtlpixelformat) formats are awfully similar. In my experience this has been the best way to encode formats and intention (compared to alternatives like e.g. storing bits and shifts like DDS did back in the D3D7/DirectDraw days) Personally I prefer the short versions, e.g. RGBA8_UNORM is just better than RGBA8888_UNORM when all formats have the same bit. Likewise RGB10A2 over RGBA1010102. DXGI uses `TYPELESS` to indicate the format can be reinterpreted. Vulkan & Metal have no such format. In OgreNext we approach that by having a `getFamily` function: `SDL_GpuPixelFormat getFamily( SDL_GpuPixelFormat f );` This format returns the same format for all the variations, in order to simplify many operations (e.g. RGBA8_UNORM and RGBA8 are exactly the same in all ways except in how the data is returned when viewed by a shader). ## More flags ```cpp typedef enum SDL_GpuTextureUsage { SDL_GPUTEXUSAGE_SHADERREAD, SDL_GPUTEXUSAGE_SHADERWRITE, SDL_GPUTEXUSAGE_RENDERTARGET } SDL_GpuTextureUsage; ``` You'll want to include a few more flags such as: - `SDL_GPUTEXUSAGE_MIPMAPGEN`. When user wants SDL to generate Mipmaps, on D3D11 this means `RENDERTARGET` flag must be added so D3D11's `GenerateMipmaps` can be used. But on D3D12, it means `SHADERWRITE` must be added so a compute shader can generate the mipmaps. - `SDL_GPUTEXUSAGE_NOT_SAMPLED` Almost all textures want to be sampled, but there's a few where the user intends to use only read and write operations, without sampling. Hence a flag with negation to indicate sampling is not necessary. Or alternatively add `SDL_GPUTEXUSAGE_SAMPLED` and have the user almost always add this flag. Although generally flags should express positive terms instead of negation; In terms of user friendliness I think this is an exception, i.e. `SDL_GPUTEXUSAGE_NOT_SAMPLED` makes sense over `SDL_GPUTEXUSAGE_SAMPLED`. - `SDL_GPUTEXUSAGE_CUBE_ARRAY_2D` Indicates the CubeMap or CubemapArray wants to also be used as ARRAY_2D. - `SDL_GPUTEXUSAGE_REINTERPRETABLE` Indicates the texture will later be reinterpreted as another format. ## More Texture types ```cpp SDL_GPUTEXTYPE_1D, SDL_GPUTEXTYPE_2D, SDL_GPUTEXTYPE_CUBE, SDL_GPUTEXTYPE_3D ``` There's a few types missing, like `CUBE_ARRAY`, `2D_ARRAY`, etc. ## Being very clear about 'depth' ```cpp typedef struct SDL_GpuTextureDescription { // ... Uint32 width; Uint32 height; Uint32 depth; Uint32 mipmap_levels; // ... } ``` Documentation needs to be very clear about how `depth` is interpreted: - When `SDL_GPUTEXTYPE_CUBE`, depth must be 6 or 1? - Personal recommendation: Depth must be 6 (see next item) - When `SDL_GPUTEXTYPE_CUBE_ARRAY`, depth must be multiple of 6? or should depth be 1, 2, 3 and is internally multiplied by 6? - Personal recommendation: Depth must be multiple of 6 (see next item) - When `SDL_GPUTEXTYPE_CUBE_ARRAY` but the texture is reinterpreted to `SDL_GPUTEXTYPE_2D_ARRAY` and depth was 1 but internally multiplied by 6, is depth now 6x on the reinterpreted texture? - This is why I recommend to be multiple of 6 for `SDL_GPUTEXTYPE_CUBE_*`. When reinterpreted to `SDL_GPUTEXTYPE_2D_ARRAY`, all code is consistent and there's no surprise. There's no need for special handling. I also recommend to rename `depth` to `depthOrSlices`. Its interpretation depends on whether the texture is of type `SDL_GPUTEXTYPE_3D` or not. ### What's the difference between depth and slice? - depth is halved when going to the next mip. - slice is constant for all mips. ## OpenGL OpenGL will by far give you the biggest headaches and constraints in your API: - OpenGL does not support samplers being separate from textures at the shader level - This is hard to emulate. You have to analyze the shader and find all pair combinations of \<texture, sampler\> and then bind them all potentially binding the same texture multiple times. Then generate a GLSL shader that remaps all of that. So if the original shader uses 5 textures and 3 samplers, at best you will have to bind 5 textures (w/ 5 samplers) and at worst case 15 textures (w/ samplers) - OpenGL threading support is bad. Really bad. The biggest problem in this area is when trying to map a GPU buffer from multiple threads. The best solution is to just disallow this. - One possible solution is to run GL in its own thread (aka RHI thread as Unreal likes to call it) - All API calls from any thread are submitted/redirected to the RHI thread - Some API calls like Draw commands can return immediately, other API calls will stall until the RHI sees it and returns data (such as when mapping buffers) - For performance this may mean some calls end up returning "OK" but later fail when the RHI sees it and tries to execute it. SDL could have a debug flag to wait for every RHI calls, and should be off in Release. ## One-time mipmaps There's mainly 2 cases for generating mipmaps on the fly: - We're rendering to a texture, and need mipmaps for it. - We'll be calling this often (e.g. once per frame or more) - Textures are loading from disk. They were not bundled with mipmaps. - We'll be generating mipmaps once. Then never again. The first case is not an issue. The second case is. The problem is that the regular code: 1. Create texture with `SDL_GPUTEXUSAGE_MIPMAPGEN` 2. Load texture from disk 3. Call `SDL_GpuGenerateMipmaps` is inefficient. Why? Because the `SDL_GPUTEXUSAGE_MIPMAPGEN` flag will stay forever which means the texture is internally either RENDERTARGET or SHADERWRITE when it doesn't have to, disabling lots of internal GPU optimizations. The easiest way to achieve this (cross platform): - Generate tmp texture - Load data into tmp texture - Generate mipmaps into tmp - Copy results into final texture - Destroy tmp (or keep it cached for reuse) An API that is easy to use could provide this convenience, by handling this TMP texture internally when loading data from disk. ## SDL_GpuPipelineDescription's attachments `SDL_GpuPipelineDescription` contains `SDL_GpuColorAttachmentDescription`, which contains a `SDL_GpuTexture`. This is wrong. What's needed in the Pipeline desc is its metadata (format, type, msaa settings, etc; except resolution). A structure should contain this data, and could be obtained via API: `SDL_GpuTextureMetadata *metadata = SDL_GetMetadata( SDL_GpuTexture *texture )` Actually `SDL_GpuTextureMetadata` = `SDL_GpuTextureDescription` but without the `char *name`. or alternatively (perhaps this is even better): `SDL_GpuColorAttachmentDescriptionMeta *metaDescriptor = SDL_GetMeta( SDL_GpuColorAttachmentDescription *descriptor )`