mac/vulkan: always return 10bit color depth for dithering #15125

Akemi · 2024-10-18T18:31:07Z

@Dudemanguy is that what you had in mind?

should i optimise the sw->fns->color_depth(sw) call so it isn't called twice in the worst case?

Dudemanguy · 2024-10-18T18:42:40Z

video/out/vo_gpu_next.c

@@ -842,7 +842,7 @@ static void apply_target_options(struct priv *p, struct pl_frame *target)
    int dither_depth = opts->dither_depth;
    if (dither_depth == 0) {
        struct ra_swapchain *sw = p->ra_ctx->swapchain;
-        if (sw->fns->color_depth) {
+        if (sw->fns->color_depth && sw->fns->color_depth(sw) != 0) {


Shouldn't need to check that. Anybody implementing this but returning 0 or some other bad value is just wrong.

0 is also perfectly valid value default value for auto dither.

if we only check for sw->fns->color_depth current behaviour will be changed. see also #13643 where we removed the color_depth(struct ra_swapchain *sw) stub in video/out/vulkan/context.c.

now that i added the 'stub' back for vulkan, at least it always returns 0 on all other contexts, the if (sw->fns->color_depth) would always be true and the else if (!pl_color_transfer_is_hdr(target->color.transfer)) condition could never be hit in any case. making it basically dead code.

since dither_depth == 0 is checked before, dither_depth is already 0 in that case. i decided to only change it if the actual function call would change it, and the else if can still be hit.

would you prefer me to change the if/else if to something like?

struct ra_swapchain *sw = p->ra_ctx->swapchain; if (dither_depth == 0 && sw->fns->color_depth) dither_depth = sw->fns->color_depth(sw); if (dither_depth == 0 && !pl_color_transfer_is_hdr(target->color.transfer)) dither_depth = 8;

now that i added the 'stub' back for vulkan, at least it always returns 0 on all other contexts

Return -1 for unsupported. 0 is currently used as "auto" value.

kasper93 · 2024-10-18T18:39:44Z

video/out/vulkan/context_mac.m

@@ -50,6 +50,11 @@ static void mac_vk_get_vsync(struct ra_ctx *ctx, struct vo_vsync_info *info)
    [p->vo_mac fillVsyncWithInfo:info];
 }

+static int mac_vk_color_depth(struct ra_ctx *ctx)
+{
+    return 10;


Can we at least check backbuffer type before returning this? This will be wrong if we have swapchain configured to ARGB8888

there is probably some way. though i am not sure if this would conflict with how moltenvk works, since we would need to retrieve the current drawable most likely, which moltenvk would to too for drawing.

video/out/vo_gpu_next.c

github-actions · 2024-10-18T18:46:25Z

Download the artifacts for this pull request:

Windows

macOS

kasper93 · 2024-10-18T18:48:35Z

Correct me if I'm wrong, but this whole change looks like a hack to disable another hack?

Specifically this part

        } else if (!pl_color_transfer_is_hdr(target->color.transfer)) {
            dither_depth = 8;

which is just wrong and if it causes problems it should be reworked to do this only on affected platforms.

EDIT: I don't think we should add a workaround to platforms that works as expected and instead contain focus on others that are affected.

EDIT2: After reading the code again, I think for now we can implement

static int mac_vk_color_depth(struct ra_ctx *ctx)
{
    return 0;
}

to use default behavior of dithering to output swap chain. And avoid duplicating format checks.

Akemi · 2024-10-18T19:11:29Z

yeah i think the pl_color_transfer_is_hdr() else if case is problematic. deciding the colour depth by source isn't optimal. anyway this is also the reason why i changed the if like i did, see above.

[edit]

which is just wrong and if it causes problems it should be reworked to do this only on affected platforms.

maybe i misunderstand, but that's what this does atm. previously this was always the case for auto on vulkan. with the changes of this PR it will only hit on platforms/contexts that don't implement color_depth(), so as before but without macvk.

kasper93 · 2024-10-18T19:20:27Z

this is also the reason why i changed the if like i did, see above.

Yes, I understand, I was confused. See my suggested changes, should make auto work fine for mac_vk, but not the rest vulkan.

kasper93 · 2024-10-18T19:18:43Z

video/out/vulkan/context.c

+    if (p->params.color_depth)
+        return p->params.color_depth(sw->ctx);
+
+    return 0;


Suggested change

return 0;

return -1;

kasper93 · 2024-10-18T19:18:55Z

video/out/vo_gpu_next.c

@@ -842,7 +842,7 @@ static void apply_target_options(struct priv *p, struct pl_frame *target)
    int dither_depth = opts->dither_depth;
    if (dither_depth == 0) {
        struct ra_swapchain *sw = p->ra_ctx->swapchain;
-        if (sw->fns->color_depth) {
+        if (sw->fns->color_depth && sw->fns->color_depth(sw) != 0) {


Suggested change

if (sw->fns->color_depth && sw->fns->color_depth(sw) != 0) {

if (sw->fns->color_depth && sw->fns->color_depth(sw) != -1) {

kasper93 · 2024-10-18T19:19:10Z

video/out/vulkan/context_mac.m

@@ -50,6 +50,11 @@ static void mac_vk_get_vsync(struct ra_ctx *ctx, struct vo_vsync_info *info)
    [p->vo_mac fillVsyncWithInfo:info];
 }

+static int mac_vk_color_depth(struct ra_ctx *ctx)
+{
+    return 10;


Suggested change

return 10;

return 0;

macOS apparently always operates at 10bit internally regardless of the display's bit depth. this changed at some point in the more recent macOS versions, where previously macOS operated at a frame buffer depth corresponding to the display. either 30-Bit Color (ARGB2101010) for 10bit or 24-Bit Color (ARGB8888) for 8bit. Fixes 13767

Akemi mentioned this pull request Oct 18, 2024

mac/vulkan: --dither-depth=auto should select 10 bit #13767

Open

Dudemanguy reviewed Oct 18, 2024

View reviewed changes

kasper93 reviewed Oct 18, 2024

View reviewed changes

Akemi added 2 commits October 19, 2024 16:36

vo_gpu_next/vulkan: add mechanism to retrieve color depth in gpu context

836f864

Akemi force-pushed the mac_vulkan_color_depth branch from 4d0703d to 1daf108 Compare October 19, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mac/vulkan: always return 10bit color depth for dithering #15125

mac/vulkan: always return 10bit color depth for dithering #15125

Akemi commented Oct 18, 2024

Dudemanguy Oct 18, 2024

kasper93 Oct 18, 2024

Akemi Oct 18, 2024

kasper93 Oct 18, 2024

kasper93 Oct 18, 2024

Akemi Oct 18, 2024

github-actions bot commented Oct 18, 2024

kasper93 commented Oct 18, 2024 •

edited

Loading

Akemi commented Oct 18, 2024 •

edited

Loading

kasper93 commented Oct 18, 2024

kasper93 Oct 18, 2024

kasper93 Oct 18, 2024

kasper93 Oct 18, 2024

	if (sw->fns->color_depth && sw->fns->color_depth(sw) != 0) {
	if (sw->fns->color_depth && sw->fns->color_depth(sw) != -1) {

mac/vulkan: always return 10bit color depth for dithering #15125

Are you sure you want to change the base?

mac/vulkan: always return 10bit color depth for dithering #15125

Conversation

Akemi commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024

kasper93 commented Oct 18, 2024 • edited Loading

Akemi commented Oct 18, 2024 • edited Loading

kasper93 commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasper93 commented Oct 18, 2024 •

edited

Loading

Akemi commented Oct 18, 2024 •

edited

Loading