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

scroll paint corruption #2757

Closed
totaam opened this issue May 7, 2020 · 16 comments
Closed

scroll paint corruption #2757

totaam opened this issue May 7, 2020 · 16 comments
Labels

Comments

@totaam
Copy link
Collaborator

totaam commented May 7, 2020

Issue migrated from trac ticket # 2757

component: core | priority: critical | resolution: fixed

2020-05-07 05:24:14: antoine created the issue


Trying to reproduce the memleak (#2730), I modified the [/browser/xpra/trunk/src/tests/xpra/test_apps/simulate_console_user.py simulate_console_user.py] script and ran it in an xterm: windows.1.geometry=(1147, 229, 1423, 1408).
Semi reliably, the scroll paints end up corrupting the screen during the last part of the test (after ps -ef).

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 05:37:58: antoine uploaded file scroll-corruption.png (34.7 KiB)

screenshot of the paint corruption
scroll-corruption.png

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 05:45:14: antoine changed status from new to assigned

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 05:45:14: antoine commented


[[Image(scroll-corruption.png)]]
(ignore the black box that cuts the xterm near sleep(0.5) - that's an artifact of using scrot for the screenshot)

Could be caused by:

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 05:59:18: antoine commented


Looks like this is caused by the switch to non-opengl windows for 'text' content added in r26145.
r26274 makes it possible to force opengl on for all windows that can support opengl, including 'text' windows (xpra attach --opengl=force) and the corruption disappears.

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 13:32:23: antoine commented


Simply turning on paint debugging with XPRA_PAINT_BOX=1 xpra attach made the bug "disappear", so it looks like a race condition of some sort.
I'm not sure if r26276 makes it harder to hit the bug, should not hurt to have it?
r26277 seems to make it easier to reproduce the bug?

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 15:21:05: antoine commented


r26281 improves the cairo scroll paint code slightly.

Testing with the changes from r26277, the corruption seems to occur where the edge of the scroll bar on the left hand side aligns with the terminal output.
The scroll list ends up divided there (the page scrolls up, the scroll bar moves in the opposite direction).

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 16:36:51: antoine uploaded file replay-scrolls.patch (5.5 KiB)

patch to save and replay scrolls later

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 17:19:30: antoine uploaded file cairo_replayscroll2.py (4.1 KiB)

replay in two separate windows (original and copy)

@totaam
Copy link
Collaborator Author

totaam commented May 7, 2020

2020-05-07 17:21:09: antoine commented


Bug could be server-side?

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 09:12:06: antoine uploaded file test_scrolling.py (1.5 KiB)

script to generate scrolling data from 2 pictures

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 09:12:41: antoine uploaded file scroll-27.png (129.8 KiB)

pic1
scroll-27.png

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 09:12:54: antoine uploaded file scroll-28.png (126.9 KiB)

pic2
scroll-28.png

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 09:25:19: antoine commented


Running the test script to generate scroll data:

$ test_scrolling.py ./scroll-27.png ./scroll-28.png
0, 28, 1339, 1181, 0, -26
0, 1235, 1339, 80, 0, -26
0, 1172, 1339, 37, 0, 26
0, 1237, 1339, 26, 0, 52

This matches the data captured from the client, and replaying the scroll changes produces an image that's wrong. Can be seen using:

$ cairo_replayscroll2.py ./scroll-28.png scroll-28.txt 

The 3rd chunk is wrong and overlaps with data from the 2nd chunk (omitting horizontal coordinates which are always the same):

  • 2nd chunk moves up by 26: from 1235, 80 pixels high -> rectangle from 1209 to 1289
  • 3rd chunk moves down by 26: from 1172, 37 pixels high -> rectangle from 1198 to 1235
    Overlaps should never occur!

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 15:09:11: antoine changed status from assigned to closed

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 15:09:11: antoine set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented May 8, 2020

2020-05-08 15:09:11: antoine commented


Fixed in r26287. (also avoid harmless debug overflow in r26284)

Explanation: comment:6 is wrong. Overlaps can and do happen, we should not try to avoid them at all costs. The cost of splitting one large scroll region can be higher.
We have to keep track of which lines have been sent using 'scroll', that was also used to shortcut out and skip sending the same line again... but that was done badly: when we're in the loop because we're counting how many lines to include in a scroll region, we need to keep counting, not skip it.

Backport to v3.0.x in 26288.

For those stuck on v3.0.9 and earlier, you can start your server with:

XPRA_SCROLL_ENCODING=0 xpra start ...

To disable scroll encoding completely. The loss of performance with some workloads can be pretty significant.

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

No branches or pull requests

1 participant