Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vo: move vo_gpu_next above vo_gpu in probe order #13620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llyyr
Copy link
Contributor

@llyyr llyyr commented Mar 3, 2024

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.

Potential show stoppers:

High Priority: #13108, #11499

Medium Priority: #11925, #9551 (#13303 / #12517)

Low Priority: #12462

Various other issues

Copy link

github-actions bot commented Mar 3, 2024

Download the artifacts for this pull request:

Windows
macOS

@sfan5
Copy link
Member

sfan5 commented Mar 3, 2024

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

@arch1t3cht
Copy link

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.

@ghost
Copy link

ghost commented Mar 4, 2024

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
Copy link

ghost commented Mar 5, 2024

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.

@hooke007
Copy link
Contributor

hooke007 commented Mar 5, 2024

Rename gpu-next to gpu

If so, firstly you should rewrite the manual.

@ghost
Copy link

ghost commented Mar 5, 2024

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
Copy link

ghost commented Mar 5, 2024

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.

@llyyr
Copy link
Contributor Author

llyyr commented Mar 5, 2024

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. 41259db

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

@ghost
Copy link

ghost commented Mar 5, 2024

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.

@llyyr
Copy link
Contributor Author

llyyr commented Mar 5, 2024

#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.

@low-batt
Copy link
Contributor

low-batt commented Apr 5, 2024

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?

@Akemi
Copy link
Member

Akemi commented Apr 5, 2024

no, since you explicitly set vo=libmpv.

@kasper93
Copy link
Contributor

We should also prioritize Vulkan over OpenGL in this case.

@na-na-hi
Copy link
Contributor

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

@kasper93
Copy link
Contributor

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

@norinoriko
Copy link
Contributor

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.

@llyyr llyyr changed the title [RFC] vo: move vo_gpu_next above vo_gpu in probe order vo: move vo_gpu_next above vo_gpu in probe order Sep 27, 2024
@llyyr
Copy link
Contributor Author

llyyr commented Sep 27, 2024

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

@llyyr
Copy link
Contributor Author

llyyr commented Nov 23, 2024

Updated documentation

@llyyr
Copy link
Contributor Author

llyyr commented Nov 23, 2024

The first issue is, once again, Nvidia specific :\

All the other issues I can't reproduce, and specifically for the third item I recall there being a fix somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants