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

Overlapping outputs on some fractional scales #138

Closed
fberg opened this issue Nov 18, 2023 · 10 comments · Fixed by #145
Closed

Overlapping outputs on some fractional scales #138

fberg opened this issue Nov 18, 2023 · 10 comments · Fixed by #145
Labels
bug Something isn't working
Milestone

Comments

@fberg
Copy link
Contributor

fberg commented Nov 18, 2023

With some fractional scales, way-displays creates output layouts that overlap. For instance, using two 3840x2160 displays, and with

ARRANGE: ROW
ALIGN: TOP
SCALING: true
AUTO_SCALE: false
SCALE:
  - NAME_DESC: '!LG Electronics LG ULTRAFINE .*'
    SCALE: 1.8

a layout is created where the outputs have logical width 2133, but the second monitor is positioned at x=2132, according to wayland-info:

interface: 'zxdg_output_manager_v1',                     version:  3, name:  8
	xdg_output_v1
		output: 58
		name: 'DP-1'
		description: 'LG Electronics LG ULTRAFINE ... (DP-1)'
		logical_x: 2132, logical_y: 0
		logical_width: 2133, logical_height: 1200
	xdg_output_v1
		output: 57
		name: 'DP-2'
		description: 'LG Electronics LG ULTRAFINE ... (DP-2)'
		logical_x: 0, logical_y: 0
		logical_width: 2133, logical_height: 1200

(I'm not sure, but it seems that this might also be preventing some Sway key bindings from working (switch to prev/next output), which is how I noticed. From observation, this works when the scale is set such that the layout is not overlapping. In my case, scales of 1.7 and 1.9 both work here. Positioning the output at x=2133 using wlr-randr also resolves this.)

Setup:

The compositor supports the new fractional scaling protocol, maybe that's the issue here? Afaik, this uses scales in increments of 1/120, while wl_fixed_from_double() which is used in computing the desired scale uses a base of 256.

It's even worse with scale 2.01, where logical_x: 1909 (as set by way-displays) and logical_width: 1912. On the other hand, 120 * 2.01 = 241.2 and 3840 * 120/241 = 1912.03, so that's probably where logical_width comes from.

Using scales that are multiples of 0.125 was fine in my tests (logical_x and logical_width matched exactly), which would make sense since 120 and 256 are both divisible by 8.

@alex-courtis
Copy link
Owner

Thank you so much for the detailed bug report.

With this example it can be definitively reproduced and unit tested.

@alex-courtis alex-courtis added this to the 1.10 milestone Nov 18, 2023
@alex-courtis alex-courtis added the bug Something isn't working label Nov 18, 2023
@alex-courtis
Copy link
Owner

This will be fixed; sanity checks will be applied. I'll likely get to it next monday.

Perhaps introduced by a move from a manually created wl_fixed_t to wl_fixed_from_double.

The compositor supports the new fractional scaling protocol, maybe that's the issue here?

What's this new protocol? Is it sway specific?

Using scales that are multiples of 0.125 was fine in my tests (logical_x and logical_width matched exactly), which would make sense since 120 and 256 are both divisible by 8.

I've not had good results with non-one-eighth and I don't think it's a good user experience. I might change the suggestion into a rule.

@alex-courtis
Copy link
Owner

Could you please send your logs? I'll write a unit test specifically for this case - physical width/height, resolution etc.

@fberg
Copy link
Contributor Author

fberg commented Nov 21, 2023

Thank you for looking into this!

What's this new protocol? Is it sway specific?

I meant fractional-scale-v1 (and here's the MR in the wayland-protocols repo for reference). It's not specific to Sway, and indeed other major compositors like KWin and Mutter have implementations already (Sway doesn't have a release version yet, hence why I built from master). Application/toolkit support has been lagging behind a bit, but FF and Chrome at least have initial implementations, and KDE applications should get it with KDE/Plasma 6, through Qt 6.

I've not had good results with non-one-eighth and I don't think it's a good user experience. I might change the suggestion into a rule.

From my (perhaps limited) understanding, if this protocol is used, the compositor gives clients a buffer and tells them what scale the content should be rendered at. Afterwards, this surface buffer is thrown onto the screen as is, with no additional up or down scaling done by the compositor at all. For instance, the application/client can make sure that fonts and other vector graphics are rendered at the correct (fractional) scale without blurriness. So, if all clients supported this protocol, there should be a perfectly sharp image at any fractional scale.

As for me, restricting to scales that are multiples of 1/8 is fine, and maybe the easiest way to go forward seeing as there seem to be two different ways that outputs may be configured (with Sway at least). But unless I'm wrong, this should no longer be strictly needed with the new protocol. 🙂

Could you please send your logs?

Here you go: way-displays.log. I've set WAYLAND_DEBUG=1 as well.

I start out with scale 2.0, then switch to 1.8, and finally to 2.01. On scale 2.01, way-displays actually enters a loop where it constantly reconfigures the layout, apparently because it thinks the scale jumps between 2.008 and 2.012. At the very end, I return to scale 2.0

@alex-courtis
Copy link
Owner

alex-courtis commented Nov 27, 2023

I meant fractional-scale-v1 (and here's the MR in the wayland-protocols repo for reference).

This is fantastic! Thank you for letting me know... I was looking for a wlroots protocol, however wayland itself is a big win.

Hopefully we can do something about X now...

Here you go: way-displays.log. I've set WAYLAND_DEBUG=1 as well.

I start out with scale 2.0, then switch to 1.8, and finally to 2.01.

Many thanks for all the detail. I'll get this sorted out on mainline and sway master.

On scale 2.01, way-displays actually enters a loop where it constantly reconfigures the layout, apparently because it thinks the scale jumps between 2.008 and 2.012. At the very end, I return to scale 2.0

Great find! This can be hardened against or protected against via discrete scales.

@alex-courtis
Copy link
Owner

Thanks to the info above I was able to replicate with some instrumentation.

It appears that wayland truncates the output width and height when applying the scale, wheras way-displays was rounding it. Changed to truncation.

@fberg I'd be most grateful if you could test a fix for the above, with one of the problematic ones like 1.8

git clone git@github.com:alex-courtis/way-displays.git
cd way-displays
git 138-fix-overlapping-outputs
make
sudo make install

When you are done you can:

sudo make uninstall

I'll add the quantization after that which should prevent all the flipping around.

@fberg
Copy link
Contributor Author

fberg commented Nov 28, 2023

Thank you! I'll make sure to do some tests on the weekend, when I'll have access to that machine again.

@fberg
Copy link
Contributor Author

fberg commented Dec 2, 2023

So I spent some time with this today and unfortunately, 53fef68 doesn't fix the issue for scale 1.8. I still get the second output positioned at x=2132, which is one pixel less than the width of the first output.

I looked into the code myself a bit and found this to work for me: fberg@a716585. That commit fixes both the overlapping outputs as well as the scale flipping for scale 2.01 (and all other tested scales). However, it introduces a similar issue of output overlaps for current release versions of Sway (and probably other wlroots compositors). One can probably detect whether the compositor supports the fractional scaling protocol, but I'm not sure if that's all it takes (and I'm way out of my depth here).

Since you wanted to quantize scales to 1/8ths anyway (and this was tested to be okay without any other modifications), I'm not sure if it's worth it to spend too much time on this. The difficulty is predicting how large the output will be after it was configured. Perhaps the best way would be to configure outputs one-by-one and looking at the sizes of the already configured outputs before doing the next? This would probably need a whole lot of rewriting though.

Btw: not sure if it's of interest, but I noticed that way-displays 1.9.0 with Sway 1.8.1 (i.e. not supporting the fractional-scale-v1 yet) can also generate layouts that are not matching up exactly: with scale 1.9 I get x=2023 for the second output, but widths are only 2022. So there's a one pixel "gap" between the outputs, but as far as I can tell this at least doesn't cause any problems with Sway like overlapping outputs do.

@alex-courtis
Copy link
Owner

alex-courtis commented Dec 3, 2023

I still get the second output positioned at x=2132, which is one pixel less than the width of the first output.

I'm not surprised. The "fix" was... hopeful.

I looked into the code myself a bit and found this to work for me: fberg@a716585. That commit fixes both the overlapping outputs as well as the scale flipping for scale 2.01 (and all other tested scales). However, it introduces a similar issue of output overlaps for current release versions of Sway (and probably other wlroots compositors).

That is fantastic! It looks solid, with good use of fixed rather than double. It looks like the quantization will be "correct" based on observed behaviour.

One can probably detect whether the compositor supports the fractional scaling protocol, but I'm not sure if that's all it takes (and I'm way out of my depth here).

We can most definitely do that: https://github.com/alex-courtis/way-displays/blob/393fdd876646c3e64d5cf018ab28f5ea33d194cc/src/listener_registry.c detects advertised capabilities, allowing us to follow different code paths.

We could also use https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/main/unstable/xdg-output/xdg-output-unstable-v1.xml?ref_type=heads#L100 to retrieve the actual sizes and positions following scaling. That would be a multi step process, like mode setting: set the output scales then position them based on their actual logical size. That may be unnecessary.

@alex-courtis
Copy link
Owner

Proposal: can you please create a PR with your work? We can work on it together to come up with a solution that works for all cases that we can test.

fberg added a commit to fberg/way-displays that referenced this issue Dec 3, 2023
fberg added a commit to fberg/way-displays that referenced this issue Dec 3, 2023
fberg added a commit to fberg/way-displays that referenced this issue Dec 3, 2023
fberg added a commit to fberg/way-displays that referenced this issue Dec 5, 2023
fberg added a commit to fberg/way-displays that referenced this issue Jan 28, 2024
fberg added a commit to fberg/way-displays that referenced this issue Feb 2, 2024
alex-courtis added a commit to fberg/way-displays that referenced this issue Feb 5, 2024
alex-courtis added a commit to fberg/way-displays that referenced this issue Feb 5, 2024
alex-courtis added a commit to fberg/way-displays that referenced this issue Feb 5, 2024
alex-courtis added a commit to fberg/way-displays that referenced this issue Feb 5, 2024
alex-courtis added a commit that referenced this issue Feb 5, 2024
… and other recent compositor changes; use --log-threshold debug to see details (#145)

* #138 truncate scaled dimensions to match wayland

* #138 assert_double_equal is not available on CI cmocka, revert to assert_float_equal

* #138 deal with compositors that support fractional-scale-v1

* #138 add tests

* #138 tests with integral scales

* #138 don't keep a Displ* in Head

* #138 round scales to multiples of 1/8 on fractional scaling compositors

* fix typo

* #138 disable check for fractional-scale-v1 entirely

Now uses a default scaling base of 8.

* #138 more scale rounding debug and test

* #138 iwyu fixes

* #138 tidy fractional scaling message

* #138 remove test harness

---------

Co-authored-by: Alexander Courtis <alex@courtis.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment