Closed Bug 777264 Opened 12 years ago Closed 12 years ago

Throttle repaint requests in Gecko panning/zooming

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → bugzilla
Blocks: 745136
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.
Attachment #645974 - Flags: review?(jones.chris.g)
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.
Attachment #645976 - Flags: review?(jones.chris.g)
Now that we have this new implementation, the old one based on throttling with minimum time deltas between repaint requests can be removed.
Attachment #645977 - Flags: review?(jones.chris.g)
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.
Andreas can you give this a spin and see if it fixes some of the lagginess you were seeing?
Will do in an hour.
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.
Attachment #645974 - Flags: review?(jones.chris.g)
Attachment #645976 - Flags: review?(jones.chris.g) → review+
Attachment #645977 - Flags: review?(jones.chris.g) → review+
Attachment #645974 - Attachment is obsolete: true
Attachment #646458 - Flags: review?(jones.chris.g)
Attachment #646458 - Flags: review?(jones.chris.g) → review+
I believe this may have caused bug 781414 and bug 778701
This code doesn't run on "desktop".  (Yet.)
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.
(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.
yeah, i read through the code and figured that out,

then i traced tinderbox back to the async scrolling patch....
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.
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?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: