vo_dmabuf_wayland: reject formats not supported by the GPU #14815
Conversation
|
Download the artifacts for this pull request: |
Dudemanguy
force-pushed
the
dmabuf-autoconvert
branch
from
804ab76 to
3552e6b
Compare
|
I didn't read the code yet, but from the commit message I couldn't grasp what exactly is the problem with current hwupload implementation. Could you explain what conversion is not currently possible and/or doesn't work? On example, will be better to understand for me. Just to reiterate how hwupload currently work. (I've debugged some d3d11 upload issue recently). We start from the list of hw formats, for each of this formats we find software format that is supported to be uploaded and filter hwformat with VO supported ones. If hwupload fails, because we cannot upload current swformat to hwformat we fallback to swscale and upload supported format. In your case there is no possible swscale conversion that would be compatible with the hwupload? For example in d3d11 case, we can only upload nv12. So it first convert yuv420p to nv12 with swscale and upload that. |
|
Are you referring to vf_d3d11? If so, that works because the vf_d3d11 filter creates another autoconvert filter under the hood via |
|
But on some further thought, a VO optionally requesting another user filter that gets created in |
The normal autoconvert filter is not relevant or used. vf_d3d11vpp requires hardware frames and the frame is fully uploaded before it. |
|
Huh I guess the refqueue handles it somehow (can't really test). One autoconvert alone is not sufficient. i.e. I get this: When what I really want is this: |
Looks like the hwupload were successful, if this upload paths doesn't work, you need to exclude Take a look at You could try adding skip in this loop Line 492 in f92d5da Line 501 in f92d5da exclude yuv420p from adding to supported formats. |
|
It is already being excluded no? |
|
There is no standard way to filter formats based on what the vo supports today, so in that sense, you are trying to fix a valid limitation. We do support filtering based on supported ra formats, although we discussed this for dmabuf-Wayland before and there probably isn't a sane way to express the supported formats in terms of ra formats. The issue here, I think, is vaapi lying about what formats it supports. It claims yuv420p and then the hwupload fails. If that was not claimed, it would probably auto convert correctly? I tried to make all of this smarter but it still relies on an accurate evaluation of vaapi capabilities, |
Indeed it looks correct. So why does it go through and do Could you step through
Yeah, that's why I'm grilling DMG to figure out why it fails for him :) |
Dudemanguy
force-pushed
the
dmabuf-autoconvert
branch
from
3552e6b to
347d7ab
Compare
Dudemanguy
changed the title
|
Edit: bug is real but the fix is not correct. So WIP still. |
|
To summarize my findings and thoughts from IRC: I was able to reproduce issue with Hyprland, which are fixed by wayland changes in this PR. On my test system sway/plasma seem to support nv12, so it just works. Hyprland on the other hand, is not supporting nv12 and before this PR, it would still use it and produce wrong colors. After this PR it correctly reject nv12 and tries to use bgr. But the master (f6d9313)hyprland (wrong colors, red): hyprland (wrong colors, red): sway/plasma: sway/plasma: PR #14815hyprland hyprland sway/plasma sway/plasma PR #14815 (w/o f_hwtransfer change)hyprland hyprland sway/plasma sway/plasma |
Dudemanguy
force-pushed
the
dmabuf-autoconvert
branch
from
347d7ab to
f2b0d7e
Compare
|
We could probably merge |
|
In theory, the hwupload probing should work after I unbreak my crap soon here. I just had to push the branch to test on another machine hence the draft (so nobody wastes times looking at obviously wrong code). Edit: Should be functional now. |
Dudemanguy
force-pushed
the
dmabuf-autoconvert
branch
5 times, most recently
from
d2ffa23 to
48001c5
Compare
Dudemanguy
force-pushed
the
dmabuf-autoconvert
branch
3 times, most recently
from
8f0fa63 to
16da43c
Compare
|
After this PR I found that it inserts an extra conversion, because nv12 isn't supported by the drm primary plane on Haswell igpu, even though it also worked before this PR without wrong colors. Both the compositor and vaExportSurfaceHandle support nv12. Before: After: |
|
What does |
|
Primary: Overlay: Cursor: |
|
I'm not sure what we can do then unless someone has a better way of getting what formats will actually work via dmabuf. |
|
@na-na-hi: Do you know which part of code is rejecting nv12 as supported now? |
|
It's the format probing via DRM for him since NV12 isn't listed in any of the planes. Note that vaExportSurfaceHandle on NV12 works just fine here on my machine but will end up with broken colors which was the whole motivation behind this PR in the first place. |
|
What if we use tranche_formats? Are they working for you without GPU format check or both were needed? |
|
Needs both. tranche_formats only sends what works on the compositor's side. |
|
Ok... @na-na-hi you are on your own in this case ¯\(ツ)/¯ |
|
I did not personally pinpoint where it happens but my assumption is that something under the hood on the driver level goes wrong when |
|
But if the planes don't declare native support, the compositor must be converting NV12, right? It can't be going through directly. |
|
In my case, the "red colors bug" happened on all compositors no matter what so I thought it wasn't compositor-dependent but probably something in drm. Since mpv does specifically create the buffer with the NV12 drm format in this case I presume that's what all compositors treat it as. Of course, there is nothing stopping a compositor from doing some other conversion if it wanted. |
|
TBH I'm not sure why it works on my hardware without conversion by mpv. It might be that the gpu driver/vaapi are doing some magic conversion behind the scene but I haven't looked at the code to confirm this. I have reverted locally for now, but it would be mostly placebo if this PR just moves that conversion to mpv and there isn't really an extra conversion overall. |
Yeah, that's why I asked. If compositor accepts nv12 in |
Yeah. Given that you're looking at old Haswell hardware, it's much more likely that the conversion is happening further down the pipeline than that there is some native support that is somehow missing or worse in newer hardware generations. That said, you would almost certainly want to use |
Well the protocol here only states that it is formats "the compositor supports". Of course, nothing is stopping a compositor from saying that formats on a certain device is not supported and thus not send the format in question but in practice it doesn't seem anyone implements that way. But if they suddenly did, nanahi would still see the same change in behavior. To be honest, it's probably better that compositors don't touch the buffer. The entire point of this VO is so mpv and the compositor can share the dmabuf and ideally it can be directly scanned out in the end so it can be zero-copy. The only thing I'd expect a compositor to possibly do is maybe handle tonemapping for hdr one day but that's still up in the air of course. |
|
I agree. On this point maybe it would be good to revise our
To double down on this I think we should also use scale_vaapi to scale video to output size, to avoid report about low quality scaling done by compositors. Settings those options https://github.com/FFmpeg/FFmpeg/blob/d103b61cd84990b0dfce52315b18fdeaffa64f05/libavfilter/vf_scale_vaapi.c#L220-L236 it even has quality levels, that could be exposed in config. This would likely fix issues like this #13745 |
Thanks for your valuable input. I guess this points out some inconsistencies between driver implementations for this feature. (Side note: I've been playing with vaapi filters recently to enhance
From my experience with the "hq" quality setting for vaapi, this would be a valuable thing to have. It looks similar to lanczos for upscaling and downscaling has much less aliasing than naive bilinear, while at the same time it performs much better (avoids framedrops at large resolutions) and uses much less power on my setup compared to mpv's shader scalers. (~0.5W vs ~5W in GPU power according to |
|
We could definitely integrate the vaapi filter some more into |
That would be consistent what can be observed on Windows too. If that mode would be available, I would switch to dmabuf myself, because it would be complete, soon we will get HDR support too. |
|
|
|
Yes, the entire point of this change was to reject formats that weren't supported by drm. You got a format conversion and that's what we want. Note that your problem is hdr which is broken and has always been broken in |
|
As I explained before, mpv looking at primary planes is fundamentally broken if there is a compositor between mpv and the primary plane. The situation reported by @na-na-hi is one that I mentioned when you proposed this logic the first time. mpv has no idea how the compositor is using the buffer. The compositor might want to put the buffer on an overlay plane or a primary plane. It might want to scan the buffer out on GPU A or GPU B. When the user has multiple GPUs this property will change at runtime. The question, what formats are "supported by the GPU" is not well-posed in the first place since there are many different ways in which a GPU can support a format and mpv has no idea which one of those is most relevant in what situation or which is the most efficient in what situation. This situation can change depending on whether the mpv window is fullscreen or being scanned out directly. It can also change when the compositor moves the mpv window between displays. If the purpose of vo_dmabuf_wayland is to be the most efficient, then the only way to do this is to use dmabuf_feedback at runtime. If the feedback changes at runtime, mpv must change its output format accordingly to guarantee presentation being optimal. Otherwise there is no way for mpv to implement this logic since mpv lacks critical information.
If the purpose of this PR is to work around bugs in the graphics driver for your GPU, report that bug to the driver developers instead. If the purpose of this PR is to work around bugs in your compositor, report that bug to the compositor developers instead. |
Every compositor implements it that way. Every compositor contains a hard-coded list of formats it supports 1 2. This is required because different formats required different logic. At runtime, it intersects that list with the formats supported by the graphics driver 3 4. For direct scanout, it intersects that list additionally with the formats supported by hardware planes 5 6. As you can see, this also involves intersecting modifiers. AFAIK, at least on Intel, only a subset of modifiers are supported for scanout and this subset might change depending on the number of connected displays due to bandwidth limitations. It is up to the compositor to detect this and tell the client to use different modifiers if it wants to support direct scanout in such situations. As far as I can tell, this PR does not look at modifiers at all. |
I get exactly the same list of formats across multiple devices in wlroots with the default renderer and weston. There are different modifiers (which we do check against) but I never thought it actually filtered out formats or was supposed to. Now if looking at drm formats on plane is fundamentally wrong then I'm at a bit of a loss what to do. There are several formats I get from tranche_formats definitely don't work when uploading to hardware. My understanding of the protocol and the behavior seen by the compositors never lead me to conclude that the compositor was filtering these things for us. I had thought support on the planes was necessary for that. As for multi-gpu/runtime stuff, the limitations are known and have always existed. Indeed it's on the todo list one day to make this better. |
Report such bugs to the components that sit between you and the display. If this PR fixed the "monochrome video" issue on your setup, then your hardware likely does not support direct scanout for the affected format. In that case, the compositor is compositing and the bug is either that the compositor does not implement the required logic or the OpenGL driver does not do the required conversion correctly. In OpenGL, seeing an all-red video is a classic symptom of the shader not using If you are using old hardware, it is possible that driver bugs are never going to get fixed and it might also be necessary to add an mpv flag to blacklist such formats. Or tell users not to use the VO on such hardware. For a wayland client, using dmabuf_feedback is the one and only way to go when selecting formats and modifiers. PS: When I had support for nv12 in my OpenGL renderer, mpv worked fine on my RX580 with vo_dmabuf_wayland. I've since removed support for such formats since I did not want to support them in my Vulkan renderer. |
|
I don't think anyone was doing anything wrong here except mpv. The compositor gives us format + modifier pairs, but it cannot know that none of them are going to work correctly for our purpose since we are drawing the image in a special way without using the usual graphics APIs. The drivers/hardware isn't bugged. It just lacked support for what we were trying to do. The old format selection code was not correct because it ignored this completely. My mistake with this PR was that I filtered them too much. This has since been corrected so that planar formats are only chosen as a fallback if the compositor supports the format and if there are no valid format + modifier pairs. |
You need bad hardware to actually test this. If you don't have bad drivers with poor format support, nothing should change except you might technically do an extra autoconvert step. Couldn't avoid that sorry but just use
--hwdecanyway.Basically my card is unable to correctly display vaapi frames from nv12 and other several common formats. On master, the behavior I get is red frames with broken colors (e.g.: #13098). This can be worked around by manually specifying a format version beforehand (e.g.
--vf=format=bgr0in my case) which will then display the correct colors since my GPU does do bgr0.Edit: removed some dated stuff since this changed alot.