Re: [PATCH 2/2] winemac.drv: Support VK_EXT_metal_surface.

[Ken Thomases]

On Dec 13, 2019, at 10:57 AM, Chip Davis  wrote:
> Prefer it to VK_MVK_macos_surface when present.
>
> MoltenVK has deprecated VK_MVK_macos_surface in favor of
> VK_EXT_metal_surface. It's likely that this extension will vanish at
> some point.
> Signed-off-by: Chip Davis
>
> dlls/winemac.drv/vulkan.c | 66 +++++++++++++++++++++++++++++++++------
> 1 file changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/dlls/winemac.drv/vulkan.c b/dlls/winemac.drv/vulkan.c
> index 21e93827c15..60f3dcda6af 100644
> --- a/dlls/winemac.drv/vulkan.c
> +++ b/dlls/winemac.drv/vulkan.c
> […]
> @@ -341,7 +367,7 @@ static void macdrv_vkDestroySwapchainKHR(VkDevice device, VkSwapchainKHR swapcha
> static VkResult macdrv_vkEnumerateInstanceExtensionProperties(const char *layer_name,
>         uint32_t *count, VkExtensionProperties* properties)
> {
> - unsigned int i;
> -
>
> unsigned int i, mvk_surface = -1, ext_surface = -1;
> VkResult res;
>
> TRACE("layer_name %s, count %p, properties %p\n", debugstr_a(layer_name), count, properties);
>
> @@ -364,14 +390,36 @@ static VkResult macdrv_vkEnumerateInstanceExtensionProperties(const char *layer_
> for (i = 0; i < *count; i++)
> {
> -
>    /* For now the only MoltenVK extension we need to fixup. Long-term we may need an array. */
> -
>    /* For now the only MoltenVK extensions we need to fixup. Long-term we may need an array. */
>   if (!strcmp(properties[i].extensionName, "VK_MVK_macos_surface"))
>   {
> -
>        if (ext_surface != -1)
> -
>        {
> -
>            /* If we've already seen EXT_metal_surface, just hide this one. */
> -
>            memcpy(properties + i, properties + i + 1, (--(*count) - i) * sizeof(VkExtensionProperties));

This and the memcpy() below should be memmove(), strictly speaking.

Use sizeof(*properties) instead of sizeof(VkExtensionProperties).

Also, please decrement *count in a separate statement.  And you need to decrement i, too, or you skip the extension that was moved to properties[i].
> -
>            continue;
> -
>        }
>       TRACE("Substituting VK_MVK_macos_surface for VK_KHR_win32_surface\n");
>
>       snprintf(properties[i].extensionName, sizeof(properties[i].extensionName),
>               VK_KHR_WIN32_SURFACE_EXTENSION_NAME);
>       properties[i].specVersion = VK_KHR_WIN32_SURFACE_SPEC_VERSION;
> -
>        mvk_surface = i;
> -
>    }
> -
>    if (!strcmp(properties[i].extensionName, "VK_EXT_metal_surface"))
> -
>    {
> -
>        if (mvk_surface != -1)
> -
>        {
> -
>            /* If we've already seen MVK_macos_surface, just hide this one. */
> -
>            memcpy(properties + i, properties + i + 1, (--(*count) - i) * sizeof(VkExtensionProperties));
> -
>            continue;
> -
>        }
> -
>        TRACE("Substituting VK_EXT_metal_surface for VK_KHR_win32_surface\n");
> -
> -
>        snprintf(properties[i].extensionName, sizeof(properties[i].extensionName),
> -
>                VK_KHR_WIN32_SURFACE_EXTENSION_NAME);
> -
>        properties[i].specVersion = VK_KHR_WIN32_SURFACE_SPEC_VERSION;
> -
>        ext_surface = i;
>   }

These two "if" statements are so similar they can and should be combined.  There can be a single flag for "seen either surface extension".  (No need to track the index.)

Finally, there's a comment above the call to the host vkEnumerateInstanceExtensionProperties() that asserts this function will return the same count.  That's no longer true.

-Ken