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

Reimplement Xinerama tiling code #682

Open
wants to merge 17 commits into
base: 3.6.x
Choose a base branch
from

Conversation

Ionic
Copy link
Member

@Ionic Ionic commented Mar 19, 2018

The current Xinerama implementation suffers from multiple problems, including it resizing based upon invisible areas if the display configuration is a bit odd.

This PR tries to fix these shortcomings by implementing a new algorithm for tiling the session window into multiple CRTCs.

The algorithm behaves like this:

  1. Generate a list of split points based upon the (remote) client's display configuration. For each coordinate space (x and y), check each display edge if it is within the agent's window. If true, add a split point.
  2. Tile the agent window based upon split points. At the end of this step, each session window should be split into smaller boxes if there were split points.
  3. Magic! At this point, we will have too many tiles for the session window - we do not want to have tiles that do not intersect with any display (i.e., would only intersect with empty space), so go through all tiles and merge those that do not intersect with at least one display with other tiles based upon a policy. Continue doing that until there are no tiles left that without a display intersection or until only one tile is left.

The mentioned "policy" for us will be hardcoded to "prefer vertical splits" (i.e., splits along the X axis) since for most people displays are placed in a horizontal fashion. That means, that when merging tiles, we will first look for neighbors in the north or south and only if there are none to the east and west.

Potentially the policy could be changed and expanded at a later point, if needed.

This PR is still WIP. The label will be removed once it's fully implemented and I've tested it for a bit.

@Ionic Ionic requested review from sunweaver and uli42 March 19, 2018 01:01
@Ionic Ionic force-pushed the bugfix/xinerama-crtcs branch 11 times, most recently from 90d94d0 to 0080a02 Compare March 20, 2018 03:09
@uli42
Copy link
Member

uli42 commented Mar 20, 2018

I have not yet looked at the algo or tested anything. But I have seen you are still using the intersect() function. I think we should try switching to Xorg region/box functions instead. See e.g. =nxagentHandleExposeEvent()= in =Events.c=

@Ionic
Copy link
Member Author

Ionic commented Mar 20, 2018

I haven't changed the actual code there yet.

Yes, for some data I'm using BoxRec. Regions are not appropriate in this case because they are... special. I don't know of any intersection function that would work with BoxRecs (well, short of converting them to Regions and intersecting these...)

I'm currently hanging at the box merging code, which is way more complicated than I have thought and might even be an NP-hard problem.

@uli42
Copy link
Member

uli42 commented Mar 20, 2018

What exactly is the problem? Deciding adjacency (is this he right spelling?)?

@Ionic
Copy link
Member Author

Ionic commented Mar 20, 2018

I've been playing around with different display placements and came up with such a situation:

.-----------------.
| 1 | 1,2 | 2 |   |
|---|-----|---|---|
| 1 |  1  |   |   |
|---|-----|---|---|
| 3 |     |   |   |
|---|-----|---|---|
|   |     |   |   |
·-----------------·

The numbers in there denote the display on which a box is displayed (and this also includes a special case in which a box is displayed on two heads since the CRTCs overlap).

In this scenario, I do not even know how to tile the window manually so that all boxes have an intersection with a display.

To make matters worse, I thought about legal merge operations and think that merging a box with a neighbor on the south should be forbidden, since that could move a panel that is displayed at the bottom of a display into invisible space. I.e., merging the box marked with 3 with its bottom neighbor should be prevented. Merging with upper neighbors should be fine, though.

In such a case, a possible tiling could be:

.---------------------.
| 1 | 1,2 | 1,2 | 1,2 |
|---|-----|-----|-----|
| 1 |  1  |  1  |  1  |
|---|-----|-----|-----|
| 3 |  3  |  3  |  3  |
|---|-----|-----|-----|
|   |     |     |     |
·---------------------·

That would expand all displays horizontally and leave a band not intersecting with any display at the bottom. I'm not sure this is a good solution, though...

@uli42
Copy link
Member

uli42 commented Mar 20, 2018

This is not what we want. It would mean the Desktop Environment would not have a crtc for the lower strip. Which would make in unusable. It must be merged with 3

@Ionic
Copy link
Member Author

Ionic commented Mar 20, 2018

It would have a fake CRTCs for the lower band not mapping to a real client-side CRTCs.

@Ionic
Copy link
Member Author

Ionic commented Mar 20, 2018

If the lower band and 3 would be merged, a panel at the bottom of 3 would move out of the visible area, which is probably worse, since users would have no way to get it back other than to drag the window around again.

@uli42
Copy link
Member

uli42 commented Mar 20, 2018

Do not make assumptions about the screen contents. The user might also use Unity with the control panel on the left or gnome or kde with the the control panel configured at the top. The point is: if you create a cut just below 3 the desktop will be resized on every movement of the nx window.The whole stoy is that we want to avoid just that!

@Ionic
Copy link
Member Author

Ionic commented Mar 20, 2018

Hm, probably a good argument. Panels at the top would be cut off, too... I've been thinking about those left and right, but naturally if merging invisible areas that would happen there, too.

So no constraints on this.

Finding an algorithm for this optimization problem will still be difficult, though...

@Ionic Ionic force-pushed the bugfix/xinerama-crtcs branch 5 times, most recently from 0917bfc to d3d3b8e Compare March 23, 2018 21:57
@Ionic Ionic force-pushed the bugfix/xinerama-crtcs branch 3 times, most recently from fa71e29 to fe3f753 Compare June 4, 2018 07:48
Ionic added 17 commits July 13, 2018 07:19
…sect_bb static since they are only used in this file.
…ULL pointers in case we're only interested in the boolean return value.
…rating a solution for extending a single box.
…oxes extending algorithm, generating a list of solutions.
…o generate and select a solution for a given base screen boxes list and remote Xinerama information.
…dR adjustment function based on the new screen tiling functions.
@sunweaver
Copy link
Member

@Ionic: Please close #673 with this PR. Is it mergable, yet?

@uli42
Copy link
Member

uli42 commented Oct 30, 2018

I have tested a bit:

  1. If you move the nxagent window to the lower edge so that only the window decoration bar is visible all outputs get disconnected. This feels wrong, but maybe there are good reasons to do it like this.

  2. I have this setup on :0

$ DISPLAY=:0 xrandr | grep conn
LVDS1 connected 1920x1200+0+0 (normal left inverted right x axis y axis) 331mm x 207mm
DP1 disconnected (normal left inverted right x axis y axis)
DP2 disconnected (normal left inverted right x axis y axis)
DP3 disconnected (normal left inverted right x axis y axis)
HDMI1 disconnected (normal left inverted right x axis y axis)
HDMI2 connected primary 1920x1200+1920+0 (normal left inverted right x axis y axis) 518mm x 291mm
VGA1 disconnected (normal left inverted right x axis y axis)
VIRTUAL1 disconnected (normal left inverted right x axis y axis)

The LVDS1 is left and the HDMI2 is right, HMDI2 is the primary output. Now running nxagent on that setup results in the order of the two outputs being swapped. Moving the nxagent window completely to the left output (LVDS1, which is first in the list above) results in the second output inside nxagent being in use:

$ DISPLAY=:55 xrandr | grep conn
NX1 disconnected
NX2 connected 1014x900+0+0 0mm x 0mm

I think we should try to keep the order as reported by the real display, if possible.

(side note: So the primary screen is shown as NX1 here, but it has no "primary" marker. Not sure if we need to clone that, as there might be situations where the primary screen is not visible)

Now, running xrandr -noprimary -display :0 makes the order the nxagent reports matching the one of :0. So I suppose if primary is active that one is put on top of the list that nxagent gets from the real display and we might not be able to sort that out. What do you think?

  1. nxagent -ac :55 opens a window on the left of my outputs (as described above). nxagent reports a size of 1914x900:
$ DISPLAY=:55 xrandr 
Screen 0: minimum 320 x 240, current 1914 x 900, maximum 3840 x 1200
NX1 disconnected
NX2 connected 1914x900+0+0 0mm x 0mm
   nx_1914x900   60.00* 

Now, simply moving the window a few pixels to the right leads to the window widening and I get this:

$ DISPLAY=:55 xrandr 
Screen 0: minimum 320 x 240, current 2880 x 900, maximum 3840 x 1200
NX1 connected 984x900+1896+0 0mm x 0mm
   nx_984x900    60.00* 
NX2 connected 1896x900+0+0 0mm x 0mm
   nx_1896x900   60.00* 

I am running xfwm4.

  1. Left output (LVDS1): 1920x1200, right output (HDMI2) 1920x1080 (the top row is aligned). Now moving the nxagent window downwards leads to the left part keeping the full height, while the right part has its height reduced with every movement:
$ DISPLAY=:55 xrandr 
Screen 0: minimum 320 x 240, current 2880 x 900, maximum 3840 x 1200
NX1 connected 1288x261+1592+0 0mm x 0mm
   nx_1288x261   60.00* 
NX2 connected 1592x900+0+0 0mm x 0mm
   nx_1592x900   60.00* 

after moving down a bit:

$ DISPLAY=:55 xrandr
Screen 0: minimum 320 x 240, current 2880 x 900, maximum 3840 x 1200
NX1 connected 1287x245+1593+0 0mm x 0mm
   nx_1287x245   60.00* 
NX2 connected 1593x900+0+0 0mm x 0mm
   nx_1593x900   60.00* 

As you can see the height of NX1 is not matching the height of NX2. This clearly a bug, as solving situations like this was original reason for the reimplementation.

@sunweaver
Copy link
Member

This PR branch has been stalling for a while. @Ionic: from what I got last time we spoke, you are not 100% satisfied with it, @uli42: from what I got last time we spoke is that this tiling code rewrite is pretty awesome and ready to merge.

@Ionic: @uli42: Please give feedback!

@sunweaver
Copy link
Member

As a side note, Ubuntu LTS is 8 months from now, so we should get this into 19.10 and let people test and complain. The time has never been better to merge this.

@Ionic
Copy link
Member Author

Ionic commented Aug 27, 2019

I haven't touched this code in a year, but there were multiple issues with it.

I do remember that there was one very hard to debug crash, but I don't know if I ever managed to fix it.

Besides that, some WMs really didn't like the new layouts (probably due to max size restrictions), complained about invalid sizes and completely disabled Xinerama sometimes, with the virtual displays set to an even weirder layout without the possibly to reset this but by detaching and reattaching the session.

Then, there's all the other cases uli described that behave weirdly (for instance disconnecting all outputs when it really should leave one output for the whole screen as a fallback, which likely is an edge case problem I haven't thought about or tested).

@sunweaver
Copy link
Member

sunweaver commented Aug 27, 2019 via email

@Ionic
Copy link
Member Author

Ionic commented Aug 27, 2019

I'd like to, but no idea right now. I'm in deep trouble with the X2Go infrastructure again and have to update dnf* packages to the most recent release. 1 week of effort so far and still counting...

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.

3 participants