933389 - For swipe snapshots, consider getting the snapshots from the window server using the private API CGSCaptureWindowsContentsToRectWithOptions

Status: NEW

(Core :: Widget: Cocoa, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: proof of concept

Starting a swipe is janky at the moment because we repaint the whole window synchronously in order to get the snapshot.

I wondered why Safari is so much smoother and profiled it, and it turns out that Safari is using the private function CGSCaptureWindowsContentsToRect in order to get the snapshot directly from the window server.

Here's a patch that lets us do the same, and it reduces swipe start jank by about 50% for me. The rest of the jank can now be attributed to either making unnecessary copies of the pixel data or to scaling down and up for HiDPI displays.

Stephen, can you test how much of a difference this patch makes for non-HiDPI displays?

The right way to fix the capturing jank would be to create a new snapshotting graphics API (which I know is planned) and use that, but if we don't want to wait for it, we could use this approach instead, for now.

Comment 1

[Stephen A Pohl [:spohl]]

Nice! Thanks, Markus!

(In reply to Markus Stange [:mstange] from comment #0)
> Stephen, can you test how much of a difference this patch makes for
> non-HiDPI displays?

Did you have a specific method of testing in mind? Is your 50% improvement number based on profiling?

> The right way to fix the capturing jank would be to create a new
> snapshotting graphics API (which I know is planned) and use that, but if we
> don't want to wait for it, we could use this approach instead, for now.

I think this would be a great alternative until we have a snapshotting API.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Markus, could you do a tryserver build of your patch, so that people can test with it?

Comment 3

[Stephen A Pohl [:spohl]]

n-i for questions in comment 1. I can kick off a try build for you if you'd like, Markus.

Comment 4

[Reuben Morais [:reuben]]

FWIW with that patch applied I can't use history swipes at all.

Comment 5

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #1)
> Nice! Thanks, Markus!
> 
> (In reply to Markus Stange [:mstange] from comment #0)
> > Stephen, can you test how much of a difference this patch makes for
> > non-HiDPI displays?
> 
> Did you have a specific method of testing in mind?

Not really, but attaching a profiler, swiping back and forth a few times, and looking at the time spent in canvas.drawWindow should give us a good idea.

> Is your 50% improvement number based on profiling?

It's based on profiling and subjective experience. The 50% number is not accurate at all, it's just what it felt like to me during my brief testing. But we should try to get some more accurate numbers before seriously considering this ;)

A try build would be much appreciated!

Comment 6

[Markus Stange [:mstange]]

(In reply to Reuben Morais [:reuben] from comment #4)
> FWIW with that patch applied I can't use history swipes at all.

Which version of OS X are you on? I've only tested this on 10.8.5.

Comment 7

[Stephen A Pohl [:spohl]]

Try build will become available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e723c5868874

Comment 8

[Reuben Morais [:reuben]]

(In reply to Markus Stange [:mstange] from comment #6)
> (In reply to Reuben Morais [:reuben] from comment #4)
> > FWIW with that patch applied I can't use history swipes at all.
> 
> Which version of OS X are you on? I've only tested this on 10.8.5.

10.9

Comment 9

[Stephen A Pohl [:spohl]]

(In reply to Reuben Morais [:reuben] from comment #4)
> FWIW with that patch applied I can't use history swipes at all.

Is it possible that you didn't resolve the rejects when applying the patch? I'll upload a patch shortly that's updated for current trunk (plus some known, broken behavior at the moment).

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> e723c5868874

This build displays swipe animations for me on 10.9.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: proof of concept

Updated for current trunk. I will need to revisit this patch to make sure that the changes from earlier bugs around page zoom get applied again. Zooming in on a page currently creates black borders when swiping. Indexing of snapshots also seems to be broken because the current page gets displayed twice (instead of the next page) when swiping forward. But this patch should unblock anyone wanting to apply the patch locally for performance testing.

Comment 11

[(no longer active)]

Comment on attachment 830541 [details] [diff] [review]
proof of concept

Review of attachment 830541 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
@@ +31,5 @@
>    // Don't synchronously decode images - draw what we have
>    const unsigned long DRAWWINDOW_ASYNC_DECODE_IMAGES = 0x10;
> +  // Avoid any rendering if possible and re-use recent existing snapshot of
> +  // the window, for example directly from the window server.
> +  const unsigned long DRAWWINDOW_USE_RECENT_WINDOW_SNAPSHOT = 0x20;

Nit: please remember to update the uuid of this interface, otherwise doing this may cause non-clobber builds to not properly update their xpt files.