mpv icon indicating copy to clipboard operation
mpv copied to clipboard

vo: move vo_gpu_next above vo_gpu in probe order

Open llyyr opened this issue 11 months ago • 18 comments

Opening a PR to track discussion on this instead of retreading the same topics repeatedly each time we discuss it.

vo_gpu_next is already much better than the default vo_gpu, and the latter doesn't receive many bug fixes or testing from maintainers making changes elsewhere in the codebase. It doesn't make much sense to keep a default that doesn't receive much or any testing at all due to almost none of the mpv contributors using it.

There are multiple steps to doing this:

  1. Just move gpu-next above gpu in the probe order and change nothing else as this PR is doing. Receive user testing and feedback before the next step.

  2. Rename gpu-next to gpu, rename gpu to gpu-legacy or gpu-old, alias gpu-next to gpu. And deprecate gpu-legacy.

  3. Move certain Vulkan contexts above OpenGL contexts in the probe order as well, since libplacebo Vulkan is better and some of the potential show stoppers only exist on OpenGL.

We could do this in multiple iterations, or all at once, or not do (3) at all.

Potential show stoppers:

High Priority: https://github.com/mpv-player/mpv/issues/13108, https://github.com/mpv-player/mpv/issues/11499

Medium Priority: https://github.com/mpv-player/mpv/issues/11925, https://github.com/mpv-player/mpv/issues/9551 (~~https://github.com/mpv-player/mpv/issues/13303~~ / ~~https://github.com/mpv-player/mpv/issues/12517~~)

Low Priority: https://github.com/mpv-player/mpv/issues/12462

Various other issues

llyyr avatar Mar 03 '24 07:03 llyyr

Download the artifacts for this pull request:

Windows
macOS

github-actions[bot] avatar Mar 03 '24 08:03 github-actions[bot]

throwing this one into the ring: https://github.com/mpv-android/mpv-android/issues/855 (not android-specific but unlikely to apply on desktop) I have intentions to work on a fix.

sfan5 avatar Mar 03 '24 10:03 sfan5

Another thing that gpu-next is currently missing over gpu is support for blend-subtitles=video (or something equivalent to it). Without this option or something similar to it, it's hard to watch content with complex subtitles on 4k (or bigger) screens without lag.

arch1t3cht avatar Mar 04 '24 14:03 arch1t3cht

mpv-player/mpv/issues/11862 still appears to be a problem, so many users may end up with color banding on default configuration even though the intended default is to have dithering.

ghost avatar Mar 04 '24 22:03 ghost

mpv-player/mpv/issues/11862 still appears to be a problem, so many users may end up with color banding on default configuration even though the intended default is to have dithering.

fwiw the issue you linked (or at least, in the form the OP originally described it) is more or less a non-issue. With --gpu-api=vulkan libplacebo selects an 8-bit surface format and inserts an 8-bit dither shader.

[   0.063][v][vo/gpu-next/libplacebo] Available surface configurations:
[   0.063][v][vo/gpu-next/libplacebo]     0: VK_FORMAT_R8G8B8A8_UNORM                 VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
[   0.063][v][vo/gpu-next/libplacebo]     1: VK_FORMAT_B8G8R8A8_UNORM                 VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
[   0.063][v][vo/gpu-next/libplacebo]     2: VK_FORMAT_R8G8B8A8_SRGB                  VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
[   0.063][v][vo/gpu-next/libplacebo]     3: VK_FORMAT_B8G8R8A8_SRGB                  VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
[   0.063][v][vo/gpu-next/libplacebo] Picked surface configuration 0: VK_FORMAT_R8G8B8A8_UNORM + VK_COLOR_SPACE_SRGB_NONLINEAR_KHR
...
[   0.197][v][vo/gpu-next/libplacebo] Dithering to 8 bit depth

As for d3d11, it selects the incorrect bit-depth because d3d11 always uses a 10-bit surface, but the driver already applies its own dithering correctly on d3d11, so you're not going to see banding regardless.

But yes, if you disable dithering on vulkan, you're going to see banding.

ghost avatar Mar 05 '24 00:03 ghost

Rename gpu-next to gpu

If so, firstly you should rewrite the manual.

hooke007 avatar Mar 05 '24 04:03 hooke007

With --gpu-api=vulkan libplacebo selects an 8-bit surface format and inserts an 8-bit dither shader.

I cannot reproduce this, for me it selects 10bit and I get banding.

but the driver already applies its own dithering correctly on d3d11, so you're not going to see banding regardless.

Not true, I tested this and d3d11 also produces banding without manually setting output depth to the correct value.

Fwiw I tested across Nvidia, AMD, and Intel on Windows - all of them had the output bitdepth set erroneously to 10bit resulting in banding regardless of gpu-api.

ghost avatar Mar 05 '24 06:03 ghost

Huh, I was under the impression that gpu-next stopped accepting bogus surface formats after https://github.com/haasn/libplacebo/commit/4d20c8b6b1ad86e18ec79c5e69b754314373810b. At the very least, it stopped doing so on my system (Windows+AMD+8bit-depth, identical to the OP). I guess this is still an issue that vo_gpu handles more consistently.

ghost avatar Mar 05 '24 07:03 ghost

Huh, I was under the impression that gpu-next stopped accepting bogus surface formats after haasn/libplacebo@4d20c8b. At the very least, it stopped doing so on my system (Windows+AMD+8bit-depth, identical to the OP). I guess this is still an issue that vo_gpu handles more consistently.

Only the d3d11 swapchain uses it at the moment and that too only when the right d3d11-output-format is passed, so it shouldn't affect vulkan. https://github.com/mpv-player/mpv/commit/41259db9523bede8c656c7a8664484e0e4befa02

This was never actually hooked up to mpv, so it doesn't do anything here.

llyyr avatar Mar 05 '24 09:03 llyyr

My bad. I retried an older build from when the issue was first created, and libplacebo doesn't choose a 16bit format anymore, it chooses an 8bit format and dithers with vulkan. I must've conflated a system/driver update with that commit and unknowingly pulled a "werks on my machine".

In which case, I agree with @wiwaz that #11862 is probably worthy of being a "show stopper" if this is still an issue on certain system configurations.

ghost avatar Mar 05 '24 17:03 ghost

#11862 is probably worthy of being a "show stopper"

I agree but I don't see any actionable prescription for this issue except not using 10/16 bit backbuffers for SDR content.

llyyr avatar Mar 05 '24 18:03 llyyr

As gpu-next does not currently support the render API (issue #10810), would prioritizing gpu-next for probing cause a problem for applications using the render API?

low-batt avatar Apr 05 '24 21:04 low-batt

no, since you explicitly set vo=libmpv.

Akemi avatar Apr 05 '24 22:04 Akemi

We should also prioritize Vulkan over OpenGL in this case.

kasper93 avatar Apr 18 '24 16:04 kasper93

Vulkan has its own set of problems, several Mesa related: #11578, #11236, #13811 (probably)

na-na-hi avatar Apr 18 '24 17:04 na-na-hi

Though in general Vulkan works better on gpu-next than OpenGL does.

kasper93 avatar Apr 18 '24 17:04 kasper93

Small suggestion that isn't really worth making a new issue for...

Unlike vo_gpu, vo_gpu_next currently defaults to --scale-radius=3 for the Sinc filter. This is perfectly fine (I highly doubt anyone uses Sinc by itself), but it breaks the current default scale-wparam for the Kaiser window, which is optimized for the two lobe variant of the Sinc filter.

To keep the visuals consistent, the default scale-wparam should be increased to something like ~9.5 ((6.33/2)*3). This gives a pretty typical tapered Sinc filter unlike the current status quo, though as usual, there's probably a more optimized value: https://www.imagemagick.org/discourse-server/viewtopic.php?t=20944.

norinoriko avatar Jul 14 '24 01:07 norinoriko

Based on irc discussion, this also moves vulkan contexts above opengl contexts in the probe order now.

llyyr avatar Sep 27 '24 21:09 llyyr