Last Comment Bug 777264 - Throttle repaint requests in Gecko panning/zooming
: Throttle repaint requests in Gecko panning/zooming
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on:
Blocks: 745136
  Show dependency treegraph
 
Reported: 2012-07-25 01:38 PDT by Doug Sherk (:drs) (inactive)
Modified: 2012-08-27 06:35 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Throttle repaints by only allowing one at a time (7.73 KB, patch)
2012-07-25 17:58 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Review
Fix Axis and AsyncPanZoomController to use CSS pixels for all comparisons (6.93 KB, patch)
2012-07-25 18:00 PDT, Doug Sherk (:drs) (inactive)
cjones.bugs: review+
Details | Diff | Review
Remove timer throttling (old implementation of this) (2.38 KB, patch)
2012-07-25 18:02 PDT, Doug Sherk (:drs) (inactive)
cjones.bugs: review+
Details | Diff | Review
Throttle repaints by only allowing one at a time (7.90 KB, patch)
2012-07-26 20:23 PDT, Doug Sherk (:drs) (inactive)
cjones.bugs: review+
Details | Diff | Review

Description Doug Sherk (:drs) (inactive) 2012-07-25 01:38:33 PDT
Currently, we throttle by a timer, but instead we should only allow one pending paint at a time and when a layers update arrives with the metrics we requested (meaning the paint completed) we send another one if one has been queued up.
Comment 1 Doug Sherk (:drs) (inactive) 2012-07-25 17:58:52 PDT
Created attachment 645974 [details] [diff] [review]
Throttle repaints by only allowing one at a time

This throttles repaints by watching for a ShadowLayersUpdate after painting. Until the ShadowLayersUpdate comes in, any repaint requests get queued up. Every time a repaint is queued up, the previous queued request is destroyed. Once the ShadowLayersUpdate arrives, any queued repaint request is immediately sent out.
Comment 2 Doug Sherk (:drs) (inactive) 2012-07-25 18:00:54 PDT
Created attachment 645976 [details] [diff] [review]
Fix Axis and AsyncPanZoomController to use CSS pixels for all comparisons

It turns out what was preventing this from working completely was that bounds checking was wrong when zoomed in. If we went off the page rect when requesting a repaint, no ShadowLayersUpdate would come in and we would think that we were locked in a repaint request. This wasn't a problem before because we didn't care whether or not a ShadowLayersUpdate arrived.

This patch fixes our bounds checking, to not only work correctly, but also use CSS pixels now.
Comment 3 Doug Sherk (:drs) (inactive) 2012-07-25 18:02:03 PDT
Created attachment 645977 [details] [diff] [review]
Remove timer throttling (old implementation of this)

Now that we have this new implementation, the old one based on throttling with minimum time deltas between repaint requests can be removed.
Comment 4 Doug Sherk (:drs) (inactive) 2012-07-25 18:04:00 PDT
Comment on attachment 645974 [details] [diff] [review]
Throttle repaints by only allowing one at a time

Noticed "This is distinct from mRepaintIsHappening [...]". I'll fix this before landing to refer to PAINT_HAPPENING instead.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-25 18:46:12 PDT
Andreas can you give this a spin and see if it fixes some of the lagginess you were seeing?
Comment 6 Andreas Gal :gal 2012-07-25 19:33:32 PDT
Will do in an hour.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-26 02:23:05 PDT
Comment on attachment 645974 [details] [diff] [review]
Throttle repaints by only allowing one at a time

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+  if (oldDisplayPort.IsEqualEdges(newDisplayPort) &&
>+      mFrameMetrics.mResolution.width == mLastPaintRequestMetrics.mResolution.width) {

This needs to take scaling into account as well.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h
>--- a/gfx/layers/ipc/AsyncPanZoomController.h
>+++ b/gfx/layers/ipc/AsyncPanZoomController.h
>@@ -51,16 +51,32 @@ public:
>     // The platform code is responsible for forwarding gesture events here. We
>     // will not attempt to generate gesture events from MultiTouchInputs.
>     DEFAULT_GESTURES,
>     // An instance of GestureEventListener is used to detect gestures. This is
>     // handled completely internally within this class.
>     USE_GESTURE_DETECTOR
>   };
> 
>+  enum PaintStatus {

ContentPainterStatus

>+    // No paint is currently happening; at least one invoked by this class.

I don't understand the "invoked by this class" part.  Can you clarify?

Note there that the content painter may not actually be idle if
content is invalidating itself, but rather it's idle wrt requests made
by apzc.

>+    PAINT_IDLE,

CONTENT_IDLE

>+    // Set every time we dispatch a request for a repaint. When a
>+    // ShadowLayersUpdate arrives and the metrics of this frame have changed, we
>+    // toggle this off and assume that the paint has completed.
>+    PAINT_HAPPENING,

CONTENT_PAINTING

>+    // This is distinct from mRepaintIsHappening in that it signals that a repaint

Don't think |mRepaintIsHappening| exists anymore.

>+    // is happening, whereas this signals that we want to repaint as soon as the
>+    // previous paint finishes.

Also note that the request will be made for the most up-to-date
metrics.

>+    PAINT_PENDING

CONTENT_PAINTING_AND_PAINT_PENDING

>+  // Stores the current paint status of the frame that we're managing. Repaints
>+  // may be triggered by other things (like content doing things), in which case
>+  // this status will not be updated. It is only changed when this class
>+  // requests a repaint.

Ah, you already wrote some of the documentation I requested :).  I
would keep this all together in the state-machine comment above.

Looks quite nice.  Would like to take a look at a second version.
Comment 8 Doug Sherk (:drs) (inactive) 2012-07-26 20:23:23 PDT
Created attachment 646458 [details] [diff] [review]
Throttle repaints by only allowing one at a time
Comment 10 Danial Horton 2012-08-08 22:20:48 PDT
I believe this may have caused bug 781414 and bug 778701
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-08 22:24:21 PDT
This code doesn't run on "desktop".  (Yet.)
Comment 12 Danial Horton 2012-08-08 22:37:30 PDT
I think we better be sure about that, this is the only change between 20120727030508 and 20120728030524 that touches anything related.

As far as i can determine reading the changeset, the merge direction was to inbound, bringing it up in line with central. so there shouldn't be anything crazy landing there.
Comment 13 Doug Sherk (:drs) (inactive) 2012-08-09 01:59:30 PDT
(In reply to Danial Horton from comment #12)
> I think we better be sure about that, this is the only change between
> 20120727030508 and 20120728030524 that touches anything related.
> 
> As far as i can determine reading the changeset, the merge direction was to
> inbound, bringing it up in line with central. so there shouldn't be anything
> crazy landing there.

I'm pretty sure this has nothing to do with that. As cjones said, this doesn't run on desktop and even if it did there would be no code path for it to actually set anything.
Comment 14 Danial Horton 2012-08-09 02:09:24 PDT
yeah, i read through the code and figured that out,

then i traced tinderbox back to the async scrolling patch....
Comment 15 Doug Sherk (:drs) (inactive) 2012-08-09 02:34:18 PDT
I assume you're referring to bug 776226?

We don't do async scrolling on desktop, and the patches in that bug hit the exact same files as the ones in this bug.

Are you able to build? If so, you could always back out these changes and try it.
Comment 16 Danial Horton 2012-08-09 03:28:32 PDT
no, i tried the tinderbox build for that patch, mercurial is a pain to use for finding what patches landed between 2 tinderbox builds,

unless thats what the graph page is for?

Note You need to log in before you can comment on or make changes to this bug.