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

sync paint updates to the video #981

Open
totaam opened this issue Sep 12, 2015 · 11 comments
Open

sync paint updates to the video #981

totaam opened this issue Sep 12, 2015 · 11 comments
Labels
Milestone

Comments

@totaam
Copy link
Collaborator

totaam commented Sep 12, 2015

Follow up from #792.

Painting all the updates together helps, allows us to reach a higher framerate, but we should not have to slow down displaying the video region just because there are other paint events coming through.

We should send those with a higher flush value and rely on the video paint to flush everything.

@totaam
Copy link
Collaborator Author

totaam commented Sep 14, 2015

2015-09-14 12:08:00: antoine uploaded file sync-paint-with-video.patch (7.8 KiB)

attempt at implementing this

@totaam
Copy link
Collaborator Author

totaam commented Sep 14, 2015

The patch above attempts to implement this feature, but doesn't seem to be making things better... maybe worse if anything!?

@totaam
Copy link
Collaborator Author

totaam commented Oct 8, 2015

Maybe we should set the minimum video batch delay to match the vrefresh rate in this case?
Most 4k monitors are 60Hz (some are just 30Hz), which means we should be batching for at least 16ms (rounded down to take processing time into account?) whereas the default "min-delay" is currently set at 5ms.

@totaam
Copy link
Collaborator Author

totaam commented Oct 16, 2015

2015-10-16 18:02:50: antoine uploaded file opengl-show-wait-time.patch (1.6 KiB)

shows how long it takes to call swap_buffers

@totaam
Copy link
Collaborator Author

totaam commented Oct 16, 2015

r10879 added an app which can easily generate 100fps: [/browser/xpra/trunk/src/tests/xpra/test_apps/fps.py].
r10880 also modified the existing [/browser/xpra/trunk/src/tests/xpra/test_apps/test_videoregions.py] test app to hit 100fps.

The good news is that the server code quickly finds the best refresh rate (60Hz on my system).
The difficulty is to find a good test case, and verify that we are making things better. The patch above allows us to see how long we have to wait for the vblank.

With the "fps" example, the time is usually in the 12 to 15ms range, with only the occasional value below 10ms, very rarely below 5ms. We almost never miss a vblank.
That's because it is quick to draw and so we spend most of the time waiting for the vblank.
With the "videoregions" example however, it's a lot more varied (it does behave a bit more like a real application...) - but I am not convinced that this is necessarily a much better baseline.

@totaam
Copy link
Collaborator Author

totaam commented Oct 23, 2015

According to this: NVidia OpenGL env variables, __GL_SYNC_TO_VBLANK can be used to enable/disable swapbuffers syncing to the vblank.

See also #386

@totaam
Copy link
Collaborator Author

totaam commented Jun 10, 2016

2016-06-10 14:08:51: antoine uploaded file sync-paint-with-video-v2.patch (5.5 KiB)

updated patch

@totaam
Copy link
Collaborator Author

totaam commented Jun 10, 2016

Following the work on #1218, here's an updated patch and a better understanding of the problems with it: r12778 shows how many rectangles we paint when we swap the opengl buffers, and whenever we paint video with other screen updates there is a long delay preceding it because it all takes too long to process.
Fixing this is not easy. Here are some ideas:

  • we could process the video region without delay, then process the rest as a delayed flush - same problem as b-frames (allow B frames with video encoders #800): we have no way of knowing if another video frame will actually come within a reasonable amount of time, so we need a timer to send the flush message.
  • delay the non-video regions less: this way, fewer of them will accumulate and we are less likely to miss the vblank. Problem here is that we risk a recurrence of Video Region Tearing With Video Controls #1218.
  • somehow prioritize video frames. Tricky since the queue has no concept of priority, and the result is likely to lead to a corrupted window since the updates are meant to be painted in order..
  • use faster encodings (ie: lz4) when we have a video region.
  • take the fps into account: both the video region fps, and the client's monitor refresh rate.
  • group all region paints into one packet to save CPU time

@totaam
Copy link
Collaborator Author

totaam commented Jun 10, 2016

2016-06-10 16:12:49: antoine uploaded file sync-paint-with-video-v5.patch (6.4 KiB)

patch update to r12780

@totaam
Copy link
Collaborator Author

totaam commented Jun 10, 2016

2016-06-10 16:27:16: antoine uploaded file sync-paint-with-video-v6.patch (6.8 KiB)

adds the ability to toggle flush using an env var

@totaam
Copy link
Collaborator Author

totaam commented Jun 10, 2016

r12778 + r12779 make it easier to see the timing of screen updates.

With the latest patch above, I find that screen updates are A LOT smoother without the sync flush code.. which is the opposite of what would be expected.

Note: r12780 sends the non-video updates in the same call to send_delayed_regions if they're close to being due, which helps a bit.

Maybe this needs to be tested with a higher fps video, or a low fps monitor? (or both)

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