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

html5 client should honour paint flush order #1432

Closed
totaam opened this issue Feb 7, 2017 · 11 comments
Closed

html5 client should honour paint flush order #1432

totaam opened this issue Feb 7, 2017 · 11 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Feb 7, 2017

Issue migrated from trac ticket # 1432

component: html5 | priority: major | resolution: fixed

2017-02-07 10:13:48: antoine created the issue


Split from #1426. Maybe scrolling should be disabled in v1.x html5 clients, otherwise they will show display corruption when connecting to v2.x servers that provide the "scroll" encoding. (v1.x servers require video - which is not enabled by default)

The "flush" screen update attribute can be used as some kind of barrier: we can decode and paint the updates in parallel as they come in, but we should not start decoding a new group of screen updates until the current group has made it to the canvas. (this is only per-window, and there are no constraints on the paint ordering of screen updates within the same flush group)

Ideally, we could start decoding the new screen updates, but just avoid updating the canvas. But that's likely to be much more difficult to implement.

@totaam
Copy link
Collaborator Author

totaam commented Feb 7, 2017

2017-02-07 10:26:16: antoine changed status from new to assigned

@totaam
Copy link
Collaborator Author

totaam commented Feb 7, 2017

2017-02-07 10:26:16: antoine edited the issue description

@totaam
Copy link
Collaborator Author

totaam commented Feb 7, 2017

2017-02-07 10:26:16: antoine commented


Keeping a simple queue of pending paint events would probably be enough:

  • add to the queue instead of actually painting whenever we have pending paints
  • process all elements in that queue (play catch up) when we have processed all the paint events for a given flush group

We just keep track of the pending flush numbers in an associative array.
A flush value of 0 (or undefined) indicates the end of the current group. Or another way: if the flush value increases, we are receiving the start of a new paint group. At that point, we cannot add new entries to the associative array until it is empty.

@totaam
Copy link
Collaborator Author

totaam commented Feb 22, 2017

2017-02-22 14:15:35: antoine commented


Done in r15139 using a simple queue. Sadly, the scroll paints still look a bit off.

Note: we could do something more fancy where we decode more than all the draw commands as they come in and only synchronize the actual updating of the offscreen canvas. But seeing how quickly things get decoded, this doesn't look like it is worth the effort.

@totaam
Copy link
Collaborator Author

totaam commented Feb 22, 2017

2017-02-22 15:11:11: antoine uploaded file html5-scroll-tmp-canvas.patch (1.3 KiB)

use a temporary canvas to avoid overlapping blitting - does not help

@totaam
Copy link
Collaborator Author

totaam commented Mar 8, 2017

2017-03-08 12:44:02: antoine commented


I've seen similar visual corruption with the python client... so maybe the scrolling paint code is fine and we're just sending the wrong instructions.
The python client just auto-refreshes it quickly so we don't see it as much.

@totaam
Copy link
Collaborator Author

totaam commented Mar 9, 2017

2017-03-09 09:29:20: antoine commented


The visual corruption with scrolling has been fixed in r15235 (15236 for v1.0.x branch), but the html5 client still shows artifacts.

@totaam
Copy link
Collaborator Author

totaam commented Mar 12, 2017

2017-03-12 08:29:14: antoine changed status from assigned to closed

@totaam
Copy link
Collaborator Author

totaam commented Mar 12, 2017

2017-03-12 08:29:14: antoine set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Mar 12, 2017

2017-03-12 08:29:14: antoine commented


The bug was in the scroll paint code, fixed in r15275 (backport in 15277)

The flush paint order is being honoured and the picture is now fine.

@totaam totaam closed this as completed Mar 12, 2017
@totaam
Copy link
Collaborator Author

totaam commented Jul 21, 2017

2017-07-21 07:42:39: antoine commented


Some changes related to the paint lock: r16441.

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