Opened 4 years ago
Closed 3 years ago
#1426 closed task (fixed)
scrolling improvements
Reported by: | Antoine Martin | Owned by: | J. Max Mena |
---|---|---|---|
Priority: | major | Milestone: | 2.0 |
Component: | encodings | Version: | trunk |
Keywords: | scroll | Cc: |
Description (last modified by )
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
Attachments (5)
Change History (20)
Changed 4 years ago by
Attachment: | scroll-timing.patch added |
---|
comment:1 Changed 4 years ago by
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 senddistance 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?)
comment:2 Changed 4 years ago by
Description: | modified (diff) |
---|---|
Status: | new → assigned |
Changed 4 years ago by
Attachment: | scroll-cythonized.patch added |
---|
this cythonized version of the code is super fast, but also not visually correct (yet)
Changed 4 years ago by
Attachment: | scroll-cythonized-v3.patch added |
---|
almost done - almost renders correctly
comment:3 Changed 4 years ago by
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)
comment:4 Changed 4 years ago by
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.
Changed 4 years ago by
Attachment: | scroll-cythonized-v4.patch added |
---|
avoid back and forth: stay in cython
comment:5 Changed 4 years ago by
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)
comment:6 Changed 4 years ago by
Owner: | changed from Antoine Martin to alas |
---|---|
Status: | assigned → new |
r14984 enables scrolling without requiring video support - also some minor fixes and improvements in 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...
comment:7 Changed 4 years ago by
Fixed some visual corruption, see ticket:1432#comment:4, the html5 client still needs fixing...
Edit: now fixed, see ticket:1432#comment:5
comment:8 Changed 4 years ago by
Retesting with a Trunk r16434 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.
Changed 4 years ago by
Attachment: | Xpra HTML Paint Bug.png added |
---|
comment:9 Changed 4 years ago by
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'
comment:10 Changed 4 years ago by
comment:11 Changed 3 years ago by
Owner: | changed from alas to Antoine Martin |
---|
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?
comment:12 Changed 3 years ago by
Owner: | changed from Antoine Martin to J. Max Mena |
---|
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.
comment:13 Changed 3 years ago by
Resolution: | → worksforme |
---|---|
Status: | new → closed |
Not heard back, closing.
comment:14 Changed 3 years ago by
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
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.
comment:15 Changed 3 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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)
add logging for scroll timing within encode_scrolling function