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

[Ken Thomases]

On Dec 13, 2019, at 10:57 AM, Chip Davis <cdavis(a)codeweavers.com> 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 <cdavis(a)codeweavers.com>
> ---
> 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