mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
28.39k stars 2.91k forks source link

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

Closed m154k1 closed 1 week ago

m154k1 commented 7 months ago

Important Information

Provide following Information:

Reproduction steps

Run mpv with --dither-depth=auto with any SDR video.

Expected behavior

mpv should select 10 bit dither depth.

Actual behavior

mpv selects 8 bit dither depth.

Log file

N/A


Since #13643 mpv selects 8 bit dither depth for SDR content. It might help some gpu contexts, but it's wrong for mac. macOS does internal dithering so the old behavior was correct. --dither-depth=10 does not produce banding, tested with 8 bit external display on https://github.com/mpv-player/mpv/issues/11862#issuecomment-1618340272.

The possible solution is implemented auto dither depth for macvk, similar to #13646.

Akemi commented 7 months ago

i am wondering, back when i looked into possible solutions for opengl i made a proof of concept where i checked if one of the displays is actually driven in 10bit. back then the "System Information" also showed whether a display is in 10bit or 8bit. neither the check works anymore nor does "System Information" show any info anymore (maybe because i am on an ARM mac now). does macOS do anything in 10bit internally anyway now and dithers down if needed?

anyway some things that need to be cleared up:

m154k1 commented 7 months ago

does macOS do anything in 10bit internally anyway now and dithers down if needed?

Yes, this is how it works now.

do we need to detect display bit depth or do we always assume 10bit?

If I understand discussion in #11862 correctly, before https://github.com/mpv-player/mpv/commit/f914947dda421aa5903084cf1813412c8bacb003 the vulkan driver was responsible for selecting color depth, so maybe the best solution would be just let the driver decide again? Looks like MoltenVK does it right.

Akemi commented 7 months ago

with the removal of this, i don't think this is possible to do in the vulkan platform context itself, since we can't just change the swapchain functions or add new ones to the static const. one probably has to readd this and extend ra_vk_ctx_params for platofrm specific functionality, though that will kill the current logic on other platforms again.

kasper93 commented 7 months ago

This is by design, as explicitly done by https://github.com/mpv-player/mpv/commit/f914947dda421aa5903084cf1813412c8bacb003 commit. You don't need more than 8-bits for SDR content and if you do set dither-depth in your config. We could even select 8-bit back buffer and the result for SDR would be the same. You don't need more.

lextra2 commented 7 months ago

Vulkan has no extension to check for the current displays bit depth. So no way to automate this step. As such it is up to the user to manually set the correct dither-depth.

llyyr commented 7 months ago

macOS does internal dithering so the old behavior was correct.

There's no real harm to dithering down to 8 bpc in mpv itself if the content isn't wide-gamut. And if the content is wide gamut, then those changes don't do anything.

However, if there's no banding on Mac at dither-depth=no (which is what happens on vulkan by default due to it picking a 16 bit backbuffer), then we could probably remove this default for Mac. Someone would have to check it first though on a Mac old enough to have 8 bit display.

Use this sample on Mac: https://github.com/mpv-player/mpv/issues/11862#issuecomment-1618340272

Bind cycle-values dither-depth "8" "no" to some key and see if there's any banding with no dithering

Akemi commented 7 months ago

Vulkan has no extension to check for the current displays bit depth. So no way to automate this step. As such it is up to the user to manually set the correct dither-depth.

what vulkan is capable is irrelevant in this case, if the specific platform has a way to check. like with d3d11.

tbh we shouldn't make just assumptions for anything. though in general less dithering is the best dithering. if we end up with 10/8bit (content) > 8 bit (mpv) > 10bit (compositor/whatever) > 10/8bit (display), i would much prefer 10/8bit (content) > 10 bit (mpv) > 10bit (compositor/whatever) > 10/8bit (display).

lextra2 commented 7 months ago

tbh we shouldn't make just assumptions for anything. though in general less dithering is the best dithering

Well I had voted for dither-depth=no as the new default. This is basically the deband discussion all over again. Just let the user handle it.

kasper93 commented 7 months ago

Just let the user handle it.

They can handle it and it is recommended by the docs. But as we can see =auto mode is expected by the users to work, so we cannot remove that.

tbh we shouldn't make just assumptions for anything.

I agree that assuming target dithering based on source content is not best idea, but in the same time assumption is that this covers most of the common case without sacrificing the actual quality much and still providing the auto mode.

I think the best is to have platform specific check and/or logic to infer target depth, on Windows we have it for d3d11, could be extended for other apis with little bit of glue code probably. Little bit less convenient to get DXGI adapter in Vulkan mode though.

On macOS, I have no idea what's possible and on Linux it probably is not really possible to do anything portable.

though in general less dithering is the best dithering

Too little dithering is worse than too much dithering.

m154k1 commented 7 months ago

Using dither-depth=no on macOS is the best way probably. @Akemi what do you think about setting no for dither-depth=auto on mac?

It'd be great if somebody can test this on Intel mac / old macOS versions.

Akemi commented 7 months ago

I think the best is to have platform specific check and/or logic to infer target depth

agreed

Too little dithering is worse than too much dithering.

i didn't say no dithering or too little, though i thought that was obvious from the context, or said differently as little dithering as possible, as much as necessary. that holds true for any kind of filtering.

Akemi commented 3 weeks ago

i am wondering how to proceed here?

to sum it up:

my findings:

this leaves us with two options:

  1. we leave the selection of dither-depth to the user, keep the current behaviour of auto that it selects 8 in the SDR case and possible suboptimal quality, and close this issue
  2. change the behaviour on macOS only to select either no or 10 in the case of dither-depth=auto

for 2. i am not sure where we want to change the code to accomplish that?

lextra2 commented 2 weeks ago

dither-depth=no as the new default, so I can remove it from my config. Just leave it to the user to cinfigure.

Dudemanguy commented 2 weeks ago

mac setting its own value for auto is probably the best way to go here. There's already an existing color_depth in ra_swapchain_fns. You can probably use that.

Akemi commented 2 weeks ago

opened a PR for this #15125.