wayland: directly use surface local coords for viewport when possible #17416
wayland: directly use surface local coords for viewport when possible #17416Dudemanguy wants to merge 2 commits intompv-player:masterfrom
Conversation
ab64fa7 to
e8c3d22
Compare
|
I think this has to be fixed in Wayland spec. Any workaround will have lots of issues. xdg_toplevel::state.fullscreen spec says:
There are several issues here:
|
|
I'm not really concerned about a theoretical compositor that enforces a 2 pixel wide border on fullscreen for whatever reason.
Inherently unavoidable. There will potentially be a 1 or 2 pixel compositor-side scale. Nothing we can do about that. Note the status quo is wrong anyway.
I would be concerned about this one however. Can you provide an example of this happening? |
It should be avoidable if the viewporter source size is representable with an integer times the scale rounded halfway away from zero, as specificed by the fractional scale spec. And I thought the rounding error was fixed by #14489.
Suppose the output has a pixel size of 1000 with a fractional scale of 3. If the compositor sends a toplevel size of 334, then you are aware of the 2 pixel excess and can render an image confined within the 1000 pixel region. If you instead use the pixel size of 1000 to calculate the destination size, it will be 333, which only scales to 999 pixels, leaving 1 pixel gap. And it can go the other way too. Suppose the output has a pixel size of 1001 with a fractional scale of 3. If the compositor sends a toplevel size of 333, then it intends to maintain a 2 pixel gap. If you instead use the pixel size of 1001 to calculate the destination size, it will be 334, which violates the spec ("the client cannot resize beyond it"). |
This is actually critical issue to ensure that it works correctly or at least try. I was thinking about this yesterday, but na-na-hi was faster to bring this up. Scaling by 1 or 2 pixels will introduce resampling blur to mpv window. All the "high quality" processing in mpv would be thrown away. We have to make sure that Firefox recently had the same issue, with blur on Wayland. This initial issue [1] was that it's blurry on Wayland and solution was to use "stable rounding" algo suggested in one of the WP PR [2]. While this fixed some targets, it regressed on KWin [3]... and the solution was to not use "stable rounding" for KDE. So... it looks messy, I would rather think of better solution that matching rounding algorithm with compositor implementation. Either way, we have to be careful here to make it display mpv content without unwanted scaling. |
|
We're talking about the specific case that this PR address which is mismatched fullscreen sizes. Being off by one pixel is already bad. You trade one bad thing for another bad thing in this scenario.
The current status quo on this example already is off by 1 pixel.
If we're not using fractional scaling, we could get away with this but that greatly complicates the code and was moved away from. In the non-fractional case, we don't have rounding problems so they shouldn't be an issue. |
|
There are several issues here:
This is about subsurfaces and therefore only affects vo-dmabuf-wayland. Otherwise mpv does not use subsurfaces, AIUI. In vo-dmabuf-wayland, mpv currently uses the following parent -> child relationships:
This will lead to blurriness of the video-surface since the rounding for subsurface is not defined. This is the problem firefox is having. However, for mpv the solution is easy. mpv can simply use this relationship:
making the video-surface the root surface with fully defined rounding. The only other change required is a call to wl_subsurface.place_below to place the background surface below the video surface. Therefore, vo-dmabuf-wayland can be made to behave exactly like every other wayland VO and we can ignore its use of subsurfaces for the purpose of this MR.
That is so. However, the protocol does not define a protocol error for this and therefore it is possible that no compositor actually sends an error in this case.
AIUI, mpv uses this code to set the destination: mpv/video/out/wayland_common.c Lines 4220 to 4224 in aa82213 The logic should always be
However, mpv seems to be doing the following:
This is incorrect because L' != L and B' calculated from L' is not necessarily the same as B. If so, this will lead to blurriness. However, this also means that, if mpv changes B, as this PR does, then L' will automatically be updated as well. Therefore this PR does not make it worse than it already is. If mpv wants the video surface to be blited 1-to-1 to the screen, it must do the following:
Never calculate L from B. My own opinion is that using anything other than the size sent in xdg_toplevel.configure for fullscreen surface is incorrect. If there was a better logical size L than the one sent by the compositor, the compositor would already send it. |
If you are referring to the example in the issue, it's off by 1 pixel too large, not too small. It trades one bad thing for another, but in this case is the issue mentioned guaranteed fixed when Wayland cannot represent in the logical coordinate system a pixel size of 1000 when scale is 3? It is possible that it is "fixed" in KWin for the 1 pixel too large case because of luck and compositor implementation details, while it will cause issues in other compositors. It is not fixing the root cause.
The "mpv calculates buffer size B from L" introduces at most a 0.5 pixel difference ( When converting B to L', L'
And this will actually make the L' incorrect. |
Ah, you are correct. I had thought about this in the past and determined that you cannot calculate L from B. But this only applies to the case where the scale is less than 1. Nevertheless, is there any reason to not handle that case correctly?
In the sense that L' != L. Not in the sense that it will be blurry. As long as L' expands to B, the surface will be blitted 1-to-1. |
In dmabuf-wayland mpv, AIUI, actually depends on the scaling done by the compositor. However there are few special cases when it should be blitted 1-to-1. One of which is when targeting fullscreen and video resolution matches the resolution exactly. Another, more general, case is when someone prescale video to window resolution with a filter, where I would expect it be blitted 1-to-1. If we introduce 1-2 pixel error, it may be hard for user to control the scaling and implicit resampling may be introduced. I don't know the extent of the issue currently, but reading this thread doesn't make me confident in dmabuf-wayland. |
There are two steps:
Currently: The first step is not fulfilled if the mode size of the output is not divisible by the scale. In this case, B will not be the mode size but something smaller. The second step is fulfilled. With this patch: The first step is fulfilled since mpv always sets B equal to the mode size. The second step is not fulfilled because L calculated by mpv will in general not expand to B. Therefore the compositor will scale the buffer. |
I think it's actually can also be bigger. As demonstrated by the original issue reported. Mode width: 3072 round(3072 / 1.85)=1661 It's rounded up twice, by compositor and by mpv. (this is not related to "stable rounding") |
It is possible. But I always round it down so that everything is visible. |
|
The reason we calculate L is because we don't always want to use L from the compositor (custom geometry settings and other things). But for the case of fullscreen, we of course can. |
|
That is fine, but then you either have to calculate B from L or you have to ensure that the calculated L maps to B. As @llyyr showed above, if there is some L' such that L' expands to B, then the L you calculate is correct. However, there are values for B such that there is no such L' and then the L you calculate is wrong. This is because going from L to L+1 sometimes skips a B value and therefore this B has no preimage. I.e. it is impossible for a buffer of size B to ever be displayed 1-to-1. |
|
Added a couple of commits to address the discussion. |
L' will never expand to B if scale factor is > 1, because B and B' differ by more than 1. |
|
Here I meant the buffer size B that mpv is actually attaching. |
|
Forgot about this. I'll try to test on multiple monitors later to make sure nothing weird happens. |
|
You should limit this hack to situations where the video size is the mode of the output. Otherwise the video will be scaled twice: once by mpv and once by the compositor. |
|
Then we're back to off by 1 pixel errors. |
|
Yes, it is a tradeoff between picture quality and being off by 1 px. |
|
You could of course not scale the picture client side at all. That is, a 720p video will be presented via a 720p buffer and the scaling will be left entirely to the compositor via viewporter. Then you would have to put the OSD on a subsurface to ensure that it is sharp. I think you have to put the OSD on a subsurface either way if you go through with this patch since otherwise the compositor will stretch and therefore blur the OSD. |
|
The whole premise here is that not being off by 1 px is better. I mean yes the extra compositor scaling is not really great, but that's not nearly as bad as 1 pixel missing. At least imo. And this is only for fullscreen of course. |
|
instead of it being 1 pixel missing, we could just prefer to waste 1 pixel instead in that scenario no? |
|
I'm not sure what you mean. |
In the standard mpv way, we can have an option to control that. I think it should be fixed on the protocol side, because both of those ways are really suboptimal to say the least. If we could fix this on the protocol side, we could just add big fat warning that fractional scaling v1 may result in unexpected image quality degradation. EDIT: Maybe we should add such warning either way? Users could opt to set 1.0 scale, if they are affected. |
|
The warning can recommend changing the scale. I already reported on IRC 5 months ago that all widescreen videos with a 1.5 scale on a 1440 output are 1 pixel smaller and found that the solution is to use a 1.507812 scale. |
I believe @llyyr is saying that instead of rendering at 1919 pixels you should render at 1921 pixels and accept that one pixel won't be visible. |
I believe this is the way to go. The issue the user in #17415 is experiencing is that 185% does not cleanly divide his 3072 x 1920 screen. However, the user can instead use 223 / 120 = 185.83333333% which is extremely close to his desired scaling and does cleanly divide his screen size (in the sense of the protocol, e.g. (1653 * 223 + 60) / 120 = 3072), leading to no off-by-one errors. I am aware that KDE currently does not allow users to set scalings that are not multiples of 3 * 5 and 223 is a prime number. This is a limitation in KDE. mpv should not hack around this limitation. |
|
The main goal here should be to avoid being scaled by the compositor no matter what. Compositor scaling will result in either a blurry mess or a blocky mess. Most wayland compositors either use bilinear or nearest for scaling, which is fine for regular desktop applications but not for videos or pictures.
I meant we should err on the side of rendering at 1919 instead of 1921, but it seems the compositor will scale mpv window in this case so this might be pointless. I'm also +1 for the warning, if the dimensions after fractional calculations isn't exactly a mode of the output then we should simply warn users about it and do nothing else |
Ah, that makes more sense. I thought mpv was already rendering at 1919 and I was wondering why @Dudemanguy was so concerned about one pixel at the edge of the screen being black when every monitor I've ever owned displays a small border area around the usable area anyway.
Can't you just decrement the logical size until size * scale <= mode? |
Previously, we calculated out the value for simplicity but there can be a mismatch due to round where what we calculate as the viewport width/height is not what surface local coordinates the compositor gave us. Note that we don't always want to use these values directly (e.g. whenever we set our own geometry), so some notion of overriding has to be added.
For rounding reasons, we should have the surface with the video on the top. Rounding subsurfaces is tricky business that we don't need to get into.
45cb7c0 to
7f8311f
Compare
|
Since nobody wants it, I've dropped the first commit. |





Unfortunately due to the way fractional scaling on wayland works, it can lead to rounding errors when calculating the physical size of the buffer. It's convoluted but essentially the compositor will communicate the logical size back to us. Depending on the fractional scale factor, the communicated size can be truncated in a significant enough way that causes final calculated size to be off by a maximum of two (truncation at both steps).
In general, there's nothing we can really do about this but for specifically fullscreen there's an easy enough workaround. Just set the size to be the same as the output. Since we might not have rendered the surface yet (and thus not have an easily accessible wl->current_output), it's easier to just loop through the output list until we have a match.