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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

vd_lavc: add Vulkan hardware decoding to autoprobe #14415

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

Conversation

kasper93
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jun 22, 2024

Download the artifacts for this pull request:

Windows
macOS

@Andarwinux
Copy link
Contributor

vulkan hwdec still crashes on nvidia.

@sfan5
Copy link
Member

sfan5 commented Jun 22, 2024

Breaks build on Android:

../video/out/vulkan/context.c:193:9: error: use of undeclared identifier 'VK_KHR_VIDEO_DECODE_QUEUE_EXTENSION_NAME'
        VK_KHR_VIDEO_DECODE_QUEUE_EXTENSION_NAME,
        ^
../video/out/vulkan/context.c:194:9: error: use of undeclared identifier 'VK_KHR_VIDEO_DECODE_H264_EXTENSION_NAME'
        VK_KHR_VIDEO_DECODE_H264_EXTENSION_NAME,
        ^
../video/out/vulkan/context.c:195:9: error: use of undeclared identifier 'VK_KHR_VIDEO_DECODE_H265_EXTENSION_NAME'
        VK_KHR_VIDEO_DECODE_H265_EXTENSION_NAME,
        ^
../video/out/vulkan/context.c:196:9: error: use of undeclared identifier 'VK_KHR_VIDEO_QUEUE_EXTENSION_NAME'
        VK_KHR_VIDEO_QUEUE_EXTENSION_NAME,
        ^
../video/out/vulkan/context.c:231:25: error: use of undeclared identifier 'VK_QUEUE_VIDEO_DECODE_BIT_KHR'
        .extra_queues = VK_QUEUE_VIDEO_DECODE_BIT_KHR,
                        ^
../video/out/vulkan/context.c:233:31: error: invalid application of 'sizeof' to an incomplete type 'const char *[]'
        .num_opt_extensions = MP_ARRAY_SIZE(opt_extensions),
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../common/common.h:48:33: note: expanded from macro 'MP_ARRAY_SIZE'
#define MP_ARRAY_SIZE(s) (sizeof(s) / sizeof((s)[0]))
                                ^~~
6 errors generated.
[...]

The newest android NDK ships with headers for 1.3.237.
Even if they update I'm not sure if Google will be allowing vk video on Android anyway.

@dyphire
Copy link
Contributor

dyphire commented Jun 22, 2024

vulkan hwdec still crashes on nvidia.

This can be confirmed on windows.

@Jules-A
Copy link

Jules-A commented Jun 22, 2024

Vulkan HWDec has a decent bit lower performance on my 6700XT (on Windows 10), not as bad as crashing at least but software performs much better (though irrelevant if they are enabling hwdec) and so does vulkan-copy (slightly) with rather heavy shaders.

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 22, 2024

vulkan hwdec still crashes on nvidia.

It has been know issue for months, there have been patches for ffmpeg, clearly not going to be fixed, so what should we do? Wait till next decade? Noted, though.

The newest android NDK ships with headers for 1.3.237.

Good, so need to check that still. I was wondering what version they have.

Even if they update I'm not sure if Google will be allowing vk video on Android anyway.

No, they won't allow Vulkan Video implementations, but it is runtime limitation, headers will still contain those definitions.

https://source.android.com/docs/compatibility/14/android-14-cdd#7142_vulkan

[C-1-11] MUST NOT enumerate support for the VK_KHR_video_queue, VK_KHR_video_decode_queue, or VK_KHR_video_encode_queue extensions.

EDIT:

Vulkan HWDec has a decent bit lower performance on my 6700XT (on Windows 10), not as bad as crashing at least but software performs much better (though irrelevant if they are enabling hwdec) and so does vulkan-copy (slightly) with rather heavy shaders.

hwdec is often slower, it is not the objective for hardware decoding. Power consumption is. Also I'm not proposing enabling hwdec by default. Your point is moot.

EDIT2:

The newest android NDK ships with headers for 1.3.237.

r27 RC 1 has 1.3.275. So next release we have it.

It is stable and safe to use already.

On platforms where it is unstable/experimental, it is disabled behind
environmental variable, so safe to probe it there too.

Fixes usage of hwdec=yes with gpu-api=vulkan, where hwdec would be not
enabled or copy variant from different API were erroneously used.
@Andarwinux
Copy link
Contributor

Andarwinux commented Jun 22, 2024

It has been know issue for months, there have been patches for ffmpeg, clearly not going to be fixed, so what should we do? Wait till next decade? Noted, though.

vkvideo is currently just cause crashes or build issues on multiple platforms, vaapi has been improved recently and vkvideo doesn't even provide any performance advantage, I'm not sure what the point of allow vkvideo by default is exactly in this case, everyone have to build mpv or ffmpeg with ot patch.

hwdec is often slower, it is not the objective for hardware decoding. Power consumption is. Also I'm not proposing enabling hwdec by default. Your point is moot.

For those who just use hwdec=auto, this is obviously a regression as well.

@kasper93
Copy link
Contributor Author

vkvideo is currently just cause crashes or build issues on multiple platforms, vaapi has been improved recently and vkvideo doesn't even provide any performance advantage, I'm not sure what the point of allow vkvideo by default is exactly in this case, everyone have to build mpv or ffmpeg with ot patch.

In this case Vulkan Video is bloat and should be removed from mpv. In other case it should be consistent and selected as documentation says, not hidden implicitly, because it was not added to autoprobe.

For those who just use hwdec=auto, this is obviously a regression as well.

What do you mean? Do you think d3d11va-copy is faster than native vulkan without copy?

@Dudemanguy
Copy link
Member

Adding it to the autoprobe is fine, but it seems premature to put it in the safe whitelist and I don't see a reason it should be first in the list. Maybe before or after vaapi imo.

@kasper93
Copy link
Contributor Author

Adding it to the autoprobe is fine, but it seems premature to put it in the safe whitelist and I don't see a reason it should be first in the list.

Not sure I agree, if it is unstable, it is unstable and shouldn't be used at all. I don't see any development around this topic lately, it really stabilized in the sense things are implemented. So unless someone put pressure to fix remaining bugs it will just stagnate.

I don't see a reason it should be first in the list.

Unlike all other APIs, and similar to d3d11va and dxva, vulkan video is tied to gpu-api, at least to some degree. So it should be probed first, to select it. Else it will never be used, because we will fallback to some -copy variant or something else. On Windows we would use d3d11va-copy which is always available, on *nix we likely will just use vaapi, so whatever.

Either way, I don't push for this. But it seems weird to have high visibility feature is intentionally hidden, it is not our fault it has problems and it highly depends on platform/hardware. We don't have any driver/vendor specific exclusions, so here it shouldn't matter either. I understand there were transition period until things are implemented/integrated, stable version of software is released, but we are past that point.

hwdec always get push back.

@kasper93
Copy link
Contributor Author

/remindme in 5 years

@kasper93 kasper93 marked this pull request as draft June 22, 2024 19:00
@norinoriko
Copy link

I think it's worth considering that most proprietary gpu vendors aren't going to bother updating or fixing vulkan video if the feature isn't visible in the first place. Do vendors care about dedicated media players more than hardware decoding on browsers? Probably not, but it's still worth thinking about.

@Dudemanguy
Copy link
Member

I can't speak for the situation on windows but on *nix it's entirely possible to autoprobe vulkan. In fact, my own card would autoprobe vulkan as long as I am using --gpu-api=vulkan because it doesn't have the modifier support necessary for vaapi to work on vulkan. --hwdec=vulkan works just fine though.

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 22, 2024

To explain myself, because I may be ranting too much. Vulkan Video suppose to be this one API that rule them all, the one that is cross platform, support everything and doesn't need additional libraries, except Vulkan itself. That's why I think it is good candidate to be positioned as this one API to use.

standards

I might update this just without safe flag, to keep it for --hwdec=auto.

because it doesn't have the modifier support necessary for vaapi to work on vulkan

So it will fallback to vaapi-copy and that's all.

@Dudemanguy
Copy link
Member

There's no reason it couldn't be before vaapi-copy.

@philipl
Copy link
Member

philipl commented Jun 23, 2024

It is certainly reasonable to make it work for auto. We already caveat that mode enough that none of these issues are blockers.

Is there any value in moving the minimum vulkan headers up to 1.3.237? I forget if we have any other conditions that would simplify.

@kasper93
Copy link
Contributor Author

Is there any value in moving the minimum vulkan headers up to 1.3.237? I forget if we have any other conditions that would simplify.

I don't think so. We can wait for next NDK, it will have 275.

@na-na-hi
Copy link
Contributor

Unlike all other APIs, and similar to d3d11va and dxva, vulkan video is tied to gpu-api, at least to some degree. So it should be probed first, to select it.

The problem is that with d3d11, there are no other usable hwdec other than d3d11va and dxva, so it's fine to place it first. But for vulkan there are other hwdec usable. I see no reason why it should be placed above nvdec for example which works perfectly with vulkan.

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 23, 2024

The problem is that with d3d11, there are no other usable hwdec other than d3d11va and dxva, so it's fine to place it first. But for vulkan there are other hwdec usable. I see no reason why it should be placed above nvdec for example which works perfectly with vulkan.

I see no reason why we should prefer one vendor (nvidia) specific codec over more open and free alternatives.

@na-na-hi
Copy link
Contributor

na-na-hi commented Jun 23, 2024

I think vendor-specific codecs should be priortized over vendor-neutral alternatives since these are usually implemented as a less efficient wrapper over the vendor-specific ones. There is no reason not to use them since these code already exist.

The only benefit I see with vendor-neutral alternatives is being able to remove vendor-specific codecs. Is this in the plan?

@kasper93
Copy link
Contributor Author

vendor-neutral alternatives since these are usually implemented as a less efficient wrapper over the vendor-specific ones

I don't think this is valid, generally they are implemented in driver anyway, but the thing is vendor specific may have more features, better support, in case things are not standardized or standardized in different ways.

The only benefit I see with vendor-neutral alternatives is being able to remove vendor-specific codecs. Is this in the plan?

In perfect world, but as we can see Vulkan is unusable and this will likely not changes. So no, there is no plans like that.

@philipl
Copy link
Member

philipl commented Jun 23, 2024

I think vendor-specific codecs should be priortized over vendor-neutral alternatives since these are usually implemented as a less efficient wrapper over the vendor-specific ones. There is no reason not to use them since these code already exist.

The only benefit I see with vendor-neutral alternatives is being able to remove vendor-specific codecs. Is this in the plan?

The exact implementation is going to vary by vendor, but we know that in Mesa, all the vulkan video implementations are not wrappers around vaapi, and we know that nvidia's implementation is directly against the low level nvdec api - meaning that is more efficient than the public cuda based nvdec api.

In practice, the way vulkan video works makes it incredibly difficult to implement as a wrapper over other implementations because of the way resource management is done.

@Andarwinux
Copy link
Contributor

What do you mean? Do you think d3d11va-copy is faster than native vulkan without copy?

This is a fact, see 13309.

I see no reason why we should prefer one vendor (nvidia) specific codec over more open and free alternatives.

At least this prevents crashes on nvidia. Of course, intel windows will still have problems.

@sfan5
Copy link
Member

sfan5 commented Jun 23, 2024

I don't think so. We can wait for next NDK, it will have 275.

That's still not a reason to do it.
I think the result of weighing "we can save 4 LOC" against "users potentially can't build mpv without annoying workaround" should be obvious.

another counterpoint: libplacebo itself only requires vk1.2 and then mpv should force the user to install vk1.3 headers for no reason?

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 23, 2024

I think the result of weighing "we can save 4 LOC" against "users potentially can't build mpv without annoying workaround" should be obvious.

I have already explained. Even Debian stable has the correct headers. NDK is the last feasible target that doesn't have them, and we can wait for it. If you are concerned about any specific platform or user, feel free to let me know, but I won't respond to mythical cases that don't exist in the real world.

It is not about saving 4 LOC, it is about shipping mpv binaries that are not crippled, by disabling features that only require build time headers to work.

another counterpoint: libplacebo itself only requires vk1.2 and then mpv should force the user to install vk1.3 headers for no reason?

libplacebo requires 1.3 headers. FFmpeg requires 1.3.277.

@sfan5
Copy link
Member

sfan5 commented Jun 23, 2024

I don't have a concrete platform in mind. But by merging these features what you are saying is "if you don't have vk1.3 headers then you can't use even just the vulkan rendering", for which there is no technical basis and which I think is unnecessary.

It is not about saving 4 LOC, it is about shipping mpv binaries that are not crippled, by disabling features that only require build time headers to work.

Well, where are these crippled mpv builds in the real world?

libplacebo required 1.3 headers.

libplacebo has a transparent way of using vendored headers in case the OS headers are too old. This would alleviate the concerns but do you want to port that to mpv?

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 23, 2024

if you don't have vk1.3 headers then you can't use even just the vulkan rendering

Exactly, same as in FFmpeg case.

libplacebo has a transparent way of using vendored headers in case the OS headers are too old. This would alleviate the concerns but do you want to port that to mpv?

I don't think this is necessary, as we don't even know what platform this solution should target. Like I said, let me know, I will add it to CI, so we can track the availability and think about solutions.

@sfan5
Copy link
Member

sfan5 commented Jun 24, 2024

Exactly, same as in FFmpeg case.

Not really. The thing is: excluding more fringe cases like -vf hwupload,yadif_cuda,hwdownload ffmpeg has exactly one use case for being compiled with vulkan and that is Vulkan Video Decoding.
Meanwhile mpv has a big and obvious use for vulkan, which works perfectly fine completely independent from hwdec (which isn't even default). And hwdec is what requires headers >=1.3.227.

And again: Where are these real world crippled mpv builds that you want to avoid?
Basing changes on real world situation rather than theory is reasonable, but this also applies to the argument for doing it in the first place.

@kasper93
Copy link
Contributor Author

kasper93 commented Jun 24, 2024

ffmpeg has exactly one use case for being compiled with vulkan and that is Vulkan Video Decoding.

Not really, ffmpeg has 13 Vulkan filters, which are directly comparable to our rendering. https://ffmpeg.org/ffmpeg-filters.html#Vulkan-Video-Filters Those does not require Vulkan Video.

Yet, they are not splitting it into each use-case, simply because the cost of acquiring the correct headers is almost nothing and doesn't depend on anything that may block such update. And maintaining support for old headers is only a burden. Sure in mpv case those are "4 LOC" but that's not the point.

And again: Where are these real world crippled mpv builds that you want to avoid?

I though it is rhetorical question. I already told that almost everywhere the correct headers are available, so the check is not needed. But yes, I think it is badly hidden, that you can enable Vulkan in mpv, have both ffmpeg and libplacebo compiled with Vulkan Video support and yet, mpv can be not functional in this area. There is no reason to do so.

I already agreed that we should wait for next NDK release, even if Vulkan Video doesn't matter there. But apart from that I just see no reason to keep our workaround.

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

Successfully merging this pull request may close these issues.

None yet

9 participants