scrolling improvements

[totaam]

Issue migrated from trac ticket # 1426

component: encodings | priority: major | resolution: fixed | keywords: scroll

2017-02-02 11:32:51: antoine created the issue

---

follow up from #1232.

Issues:

• it can take a lot of CPU to figure out the scroll encoding (figures given here are made worse by the instrumentation and profiling tools enabled):

scroll encoding total time: 89ms

So high that we might has well not bother!

• we should add the ability to enable it even when there are no video encodings available (ie: for html5 client)
• we make a copy of the xshm backing for each subimage we send to the non-scroll compression - these could be handled as memoryviews

[totaam]

2017-02-02 11:35:36: antoine uploaded file scroll-timing.patch (2.2 KiB)

add logging for scroll timing within encode_scrolling function

[totaam]

2017-02-02 11:40:41: antoine commented

---

Using the patch above, we can see that there are 2 main areas that take too long to complete:

2017-02-02 02:01:12,113 updated scroll data, previously set: True
2017-02-02 02:01:12,116 best scroll guess took 4ms, matches 76% of 1485 lines: -80
2017-02-02 02:01:12,118 encode_scrolling(XShmImageWrapper(BGRX: 0, 0, 1923, 1485), {..}, [], [], {'scroll': True}) window-dimensions=(1923, 1485)
2017-02-02 02:01:12,175  will send scroll packets for yscroll=[-80, -79, -81, -78, -82, -77, -83, -76, -84, -75, -85, -74, -86, -73, -87, -72, -88, -71, 0, -70, -89, -69, -90, -68, -67, -91, -66, -92, -65, -64, -93, -63, -62, -94, -61, -95, -60, -96, -97, -59, -97, -59, -98, -58, -98, -58, 11, -57, 11, -57], elapsed=56ms
2017-02-02 02:01:12,180  scroll groups for distance=-80 : [(71, 429), (599, 167), (865, 540)]
2017-02-02 02:01:12,183  scroll groups for distance=-79 : [(1405, 1)]
2017-02-02 02:01:12,186  scroll groups for distance=-78 : [(1406, 1)]
2017-02-02 02:01:12,189  scroll groups for distance=-77 : [(1407, 1)]
2017-02-02 02:01:12,192  scroll groups for distance=-76 : [(1408, 1)]
2017-02-02 02:01:12,195  scroll groups for distance=-75 : [(1409, 1)]
2017-02-02 02:01:12,198  scroll groups for distance=-74 : [(1410, 1)]
2017-02-02 02:01:12,201  scroll groups for distance=-73 : [(1411, 1)]
2017-02-02 02:01:12,204  scroll groups for distance=-72 : [(1412, 1)]
2017-02-02 02:01:12,208  scroll groups for distance=-71 : [(1413, 1)]
2017-02-02 02:01:12,211  87 lines have not changed
2017-02-02 02:01:12,215  scroll groups for distance=-70 : [(1414, 1)]
2017-02-02 02:01:12,218  scroll groups for distance=-69 : [(1415, 1)]
2017-02-02 02:01:12,221  scroll groups for distance=-68 : [(1416, 1)]
2017-02-02 02:01:12,224  scroll groups for distance=-67 : [(1417, 1)]
2017-02-02 02:01:12,226  scroll groups for distance=-66 : [(1418, 1)]
2017-02-02 02:01:12,229  scroll groups for distance=-65 : [(1419, 1)]
2017-02-02 02:01:12,232  scroll groups for distance=-64 : [(1420, 1)]
2017-02-02 02:01:12,234  scroll groups for distance=-63 : [(1421, 1)]
2017-02-02 02:01:12,237  scroll groups for distance=-62 : [(1422, 1)]
2017-02-02 02:01:12,240  scroll groups for distance=-61 : [(1423, 1)]
2017-02-02 02:01:12,243  scroll groups for distance=-60 : [(1424, 1)]
2017-02-02 02:01:12,248  scroll groups for distance=11 : [(784, 9), (1450, 2)]
2017-02-02 02:01:12,251  distance matching took 73ms
2017-02-02 02:01:12,253  non scroll: 6 packets: [(500, 99), (766, 18), (793, 25), (820, 45), (1439, 11), (1452, 33)]
2017-02-02 02:01:12,257 compress: 137.4ms for 1923x1485 pixels at    0,0    for wid=1     using scroll as  25 rectangles  (11154KB)           , sequence    90, client_options={'scroll': True, 'flush': 6}
2017-02-02 02:01:12,260  splitting took 6ms
2017-02-02 02:01:12,262 sub_image 190377 pixels: 0.1ms
2017-02-02 02:01:12,268 compress:   6.0ms for 1923x99   pixels at    0,500  for wid=1     using   jpeg with ratio   5.4%  (  743KB to    40KB), sequence    91, client_options={'scroll': True, 'flush': 5}
2017-02-02 02:01:12,271 sub_image 34614 pixels: 0.1ms
2017-02-02 02:01:12,276 compress:  14.1ms for 1923x18   pixels at    0,766  for wid=1     using   jpeg with ratio   6.1%  (  135KB to     8KB), sequence    92, client_options={'scroll': True, 'flush': 4}
2017-02-02 02:01:12,279 sub_image 48075 pixels: 0.1ms
2017-02-02 02:01:12,285 compress:  22.5ms for 1923x25   pixels at    0,793  for wid=1     using   jpeg with ratio   1.3%  (  187KB to     2KB), sequence    93, client_options={'scroll': True, 'flush': 3}
2017-02-02 02:01:12,288 sub_image 86535 pixels: 0.1ms
2017-02-02 02:01:12,296 compress:  33.8ms for 1923x45   pixels at    0,820  for wid=1     using   jpeg with ratio   5.0%  (  338KB to    17KB), sequence    94, client_options={'scroll': True, 'flush': 2}
2017-02-02 02:01:12,299 sub_image 21153 pixels: 0.0ms
2017-02-02 02:01:12,304 compress:  41.5ms for 1923x11   pixels at    0,1439 for wid=1     using   jpeg with ratio   1.8%  (   82KB to     1KB), sequence    95, client_options={'scroll': True, 'flush': 1}
2017-02-02 02:01:12,306 sub_image 63459 pixels: 0.1ms
2017-02-02 02:01:12,313 compress:  50.4ms for 1923x33   pixels at    0,1452 for wid=1     using   jpeg with ratio   4.1%  (  247KB to    10KB), sequence    96, client_options={'scroll': True}
2017-02-02 02:01:12,315 non-scroll encoding using jpeg (quality=42, speed=90) took 52ms for 6 rectangles
2017-02-02 02:01:12,318 scroll encoding total time: 199ms

• elapsed=56ms is the time spent figuring out which scroll values to send
• distance matching took 73ms
• the rest is the region encoding (already improved in r14952 by using jpeg instead of png - quality / speed settings permitting)

The whole thing should be cythonized.
For the v1.0.x branch, maybe we can just limit the size of the search space (<1K vertical?)

[totaam]

2017-02-02 13:03:11: antoine changed status from new to assigned

[totaam]

2017-02-02 13:03:11: antoine edited the issue description

[totaam]

2017-02-02 15:07:18: antoine uploaded file scroll-cythonized.patch (27.9 KiB)

this cythonized version of the code is super fast, but also not visually correct (yet)

[totaam]

2017-02-03 14:44:31: antoine uploaded file scroll-cythonized-v3.patch (34.5 KiB)

almost done - almost renders correctly

[totaam]

2017-02-03 14:45:46: antoine commented

---

r14965 avoids the unnecessary memory copy with the X11 image wrapper class.
The patch above is almost complete (just needs an off-by-one fixed)

Other improvements planned:

• CRC Image function should be added to the cython class so we can allocate the checksum list directly in C code (no need to go back and forth between integers and cython numbers)
• we could probably split each line in chunks of ~512bytes (128 pixels) so that we could send partial scroll packets: only the right edge (less than 128 pixels) would be sent as a picture in most cases. We can do this search if we have found a scroll distance already (fast - worth the extra hash cost)

[totaam]

2017-02-04 08:31:19: antoine commented

---

After much time spent debugging, the new scroll code is just fine, the real problem comes from the new jpeg encoder instead: #1423.

So r14972 commits the new scroll code, and turns off jpeg for non-scroll region updates.

Still TODO:

• avoid using python arrays for checksums altogether
• enable scrolling without video modes

---

Useful debugging steps for future reference:

• run the client with:

XPRA_PAINT_FLUSH=0 \
XPRA_OPENGL_SAVE_BUFFERS=png \
   xpra attach -d draw --no-mmap

This will make the paint code really slow (lack of delayed flush) and save each FBO to disk so we can see the screen updates one by one, including the scroll ones.

• run the server with:

XPRA_SAVE_TO_FILE=png \
    xpra start -d scroll

So we can see the original region we sent for non-scroll areas.

[totaam]

2017-02-05 13:41:15: antoine uploaded file scroll-cythonized-v4.patch (34.6 KiB)

avoid back and forth: stay in cython

[totaam]

2017-02-06 05:57:39: antoine commented

---

Cythonization complete in r14978.
We no longer use python-xxhash and ship xxhash with a cython wrapper instead. I've kept "python-xxhash" packaging in the tree for now so we can continue to build updates from trunk for v1.0.x.

Remaining tasks:

• enable scrolling without video encodings (for html5 client)
• split checksums so we can do partial line scrolling (we're likely to still invalidate the whole line for simplicity)

[totaam]

2017-02-06 10:18:05: antoine changed status from assigned to new

[totaam]

2017-02-06 10:18:05: antoine changed owner from antoine to afarr

[totaam]

2017-02-06 10:18:05: antoine commented

---

r14984 enables scrolling without requiring video support - also some minor fixes and improvements in r14994, r15002. It can be tested with the python client using:

xpra attach --encodings=png,jpeg,rgb --no-mmap -d draw

Or with the HTML5 client, but that one seems to have paint issues with the "scroll" encoding... which will need to be fixed and backported, see #1432.

The non-scroll paints will also be less optimal than before, because we end up in the video codepath - which will often encode a larger region.. This can happen if the page has lots of animation on it.

Partial line scrolling has been moved to #1429, so this is ready for testing.

---

@afarr: mostly a FYI at this point, scroll should perform better than before but that's quite difficult to quantify without running the profiling tools.
The more important improvement is for the HTML5 client, but this still needs fixing...

[totaam]

2017-03-09 09:30:07: antoine commented

---

Fixed some visual corruption, see #1432#comment:4, the html5 client still needs fixing...

Edit: now fixed, see #1432#comment:5

[totaam]

2017-07-20 21:29:29: maxmylyn commented

---

Retesting with a Trunk 16434 Fedora 25 server and client:

• Started server with xpra start :13 --html=on --start-via-proxy=no --start-child=xterm --start-new-commands=yes --bind-tcp=0.0.0.0:

I'm still seeing the visual corruption you mentioned in comment:7 despite being well after the version it was fixed with in #1432. I also triggered a few error prints so I'll look into it a bit further. (sidetracked with other things right now) I managed to get it to stick once, but other than that one time it seems to clear up after a second.

[totaam]

2017-07-20 21:29:47: maxmylyn uploaded file Xpra HTML Paint Bug.png (210.1 KiB)

[totaam]

2017-07-20 23:55:52: maxmylyn commented

---

Oh neat I got a traceback while writing a new ticket:

2017-07-20 15:55:17,523 Error during scrolling detection
2017-07-20 15:55:17,524  with image=XShmImageWrapper(BGRX: 1193, 71, 12, 754), options={'scroll': True}
Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/xpra/server/window/window_video_source.py", line 1645, in do_video_encode
    return self.encode_scrolling(scroll_data, image, options)
  File "/usr/lib64/python2.7/site-packages/xpra/server/window/window_video_source.py", line 1470, in encode_scrolling
    for scroll, line_defs in raw_scroll.items():
AttributeError: 'list' object has no attribute 'items'

[totaam]

2017-07-21 05:52:14: antoine commented

---

The stacktrace from comment:9 was caused by r16343 (2.1 only - just a few days ago) and is fixed by r16436. Happens when the video region size changes.
It should have been harmless, we detect something has gone wrong and fallback to non-video encoding when anything like that happens.

[totaam]

2017-07-25 00:04:10: maxmylyn changed owner from afarr to antoine

[totaam]

2017-07-25 00:04:10: maxmylyn commented

---

Alright after the change in r16436 (upped server to r16498) I'm not seeing that traceback any more. I still see the corruption while scrolling(screenshot I attached between comment:8 and comment:9), but it clears after I finish scrolling.

@antoine: Is that still a bug, or is the refresh enough to close this?

[totaam]

2017-08-03 18:33:30: antoine changed owner from antoine to maxmylyn

[totaam]

2017-08-03 18:33:30: antoine commented

---

Is that still a bug, or is the refresh enough to close this?
Visual corruption is a bug: the scrolling moves things around, and the regions that are updated separately may get painted with lossy jpeg. But the screenshots show something else, it looks like scroll and paints are happening out of order: [/attachment/ticket/1426/Xpra%20HTML%20Paint%20Bug.png]

The question is: is this related to the improvements to scrolling from this ticket (due to be closed for the 2.0 release...), or a bug in the HTML5 client scroll paint code (which would belong in a different ticket).
If you cannot reproduce similar visual corruption with the standard python client (with opengl), then it is the latter.

[totaam]

2017-09-08 11:38:57: antoine changed status from new to closed

[totaam]

2017-09-08 11:38:57: antoine set resolution to worksforme

[totaam]

2017-09-08 11:38:57: antoine commented

---

Not heard back, closing.

[totaam]

2017-09-11 18:35:43: maxmylyn changed status from closed to reopened

[totaam]

2017-09-11 18:35:43: maxmylyn removed resolution (was worksforme)

[totaam]

2017-09-11 18:35:43: maxmylyn commented

---

Woops look like this one slipped through the cracks.

I can't reproduce it via the Python client, but it's still reproducible as of trunk r16825.

[totaam]

2017-09-11 18:39:59: antoine changed status from reopened to closed

[totaam]

2017-09-11 18:39:59: antoine set resolution to fixed

[totaam]

2017-09-11 18:39:59: antoine commented

---

I can't reproduce it via the Python client, but it's still reproducible as of trunk r16825.

See comment:12 :

... or a bug in the HTML5 client scroll paint code (which would belong in a different ticket)