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

lossless scrolling #1320

Closed
totaam opened this issue Sep 21, 2016 · 12 comments
Closed

lossless scrolling #1320

totaam opened this issue Sep 21, 2016 · 12 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Sep 21, 2016

Issue migrated from trac ticket # 1320

component: encodings | priority: minor | resolution: fixed | keywords: scrolling

2016-09-21 11:52:20: antoine created the issue


Follow up from #1232: we could remove the need for auto-refresh packets when we do scrolling and the original data is lossless.
The same rectangle transformations could be applied to the list of regions needed a refresh.

@totaam
Copy link
Collaborator Author

totaam commented Feb 8, 2019

2019-02-08 06:00:50: antoine changed status from new to assigned

@totaam
Copy link
Collaborator Author

totaam commented Mar 21, 2019

2019-03-21 09:04:38: antoine uploaded file lossless-scrolling.patch (4.8 KiB)

work in progress patch

@totaam
Copy link
Collaborator Author

totaam commented Mar 21, 2019

2019-03-21 09:09:46: antoine commented


Updates:

  • r22188 better rectangle hash: prevent collisions, already backported
  • r22189 bug fix (almost impossible to trigger), already backported
  • r22190 boost non-scroll quality, more so if we have a high match percentage (less to send) - could be backported but not strictly a bug fix

Those fixes combined with the patch above and scrolling already works a lot better than I expected. Giving smooth lossless scrolling in most cases.

Still TODO:

  • fix race conditions: we modify the refresh list from a network thread..
  • the list of rectangles could grow too big, and we traverse it twice (O(N^2)): give up past a certain size?
  • maybe try harder to re-merge contiguous rectangles?

One unintended benefit of this ticket is that we should be able to see bugs in the scroll paint code more easily because those would not get refreshed. So far so good.

@totaam
Copy link
Collaborator Author

totaam commented Mar 22, 2019

2019-03-22 08:10:36: antoine changed status from assigned to new

@totaam
Copy link
Collaborator Author

totaam commented Mar 22, 2019

2019-03-22 08:10:36: antoine changed owner from antoine to encodedEntropy

@totaam
Copy link
Collaborator Author

totaam commented Mar 22, 2019

2019-03-22 08:10:36: antoine commented


Changes:

  • r22205 + r22206: reduce race condition window, try to only add to the refresh list until we consume it
  • r22207 more correct pixel accounting when adding regions to the refresh list (side note: the cython rectangle code is much faster with python2.. not sure why)
  • r22208 "lossless scrolling", the core changes

It works very well.

Caveats:

  • it may still be possible for the server to lose track of some regions that need a refresh, but the window of opportunity for this race condition is very small and unlikely - adding locking to prevent this race condition would be too costly.
  • a video may still look weird when scrolling since it runs in parallel, with its own scheduling.
  • I trust the html5 client scroll paint code less than the python client (ie: Scroll paint bug in HTML5 Client #1637), though it seems to work fine when I tried.

For testing, I had to force the quality lower to ensure that the non-scroll areas would get sent using a lossy encoding and later refreshed correctly (ie: jpeg or webp in lossy mode is what we want).
To make sure that the server doesn't choose rgb for those, I configured my client to not use rgb at all: --encodings=webp,png,jpeg,h264

To be able to see what is being sent to the client:

I can scroll up and down frantically using a browser as a client application and I never see any visual artifacts. And it's smooth too.

@encodedEntropy: this is a major improvement over the previous version of the scrolling code.

@totaam
Copy link
Collaborator Author

totaam commented Aug 1, 2019

2019-08-01 13:02:40: smo changed owner from encodedEntropy to smo

@totaam
Copy link
Collaborator Author

totaam commented Aug 7, 2019

2019-08-07 15:33:38: smo changed owner from smo to Antoine Martin

@totaam
Copy link
Collaborator Author

totaam commented Aug 7, 2019

2019-08-07 15:33:38: smo commented


Tested with server and client 3.0-r23413

server

xpra --bind-tcp=0.0.0.0:14000 --start=xterm --no-daemon -d compress,refresh,scroll start :15

client

XPRA_OPENGL_PAINT_BOX=1 xpra --encodings=webp,png,jpeg,h264 attach tcp:localhost:14000

Also tested with google-chrome and the html5 client.

In both cases the scrolling was working as expected and was extremely smooth.
Very good!

@totaam
Copy link
Collaborator Author

totaam commented Aug 8, 2019

2019-08-08 06:39:29: antoine changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Aug 8, 2019

2019-08-08 06:39:29: antoine set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Aug 8, 2019

2019-08-08 06:39:29: antoine commented


@smo: why override the encodings?
Not having rgb in that list is going to hurt performance, why bother?

@totaam totaam closed this as completed Aug 8, 2019
This was referenced Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant