The default bug view has changed. See this FAQ.

Throttle repaint requests in Gecko panning/zooming

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: drs, Assigned: drs)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → bugzilla
Blocks: 745136
(Assignee)

Comment 1

5 years ago
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.
Attachment #645974 - Flags: review?(jones.chris.g)
(Assignee)

Comment 2

5 years ago
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.
Attachment #645976 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

5 years ago
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.
Attachment #645977 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

5 years ago
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?

Comment 6

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 646458 [details] [diff] [review]
Throttle repaints by only allowing one at a time
Attachment #645974 - Attachment is obsolete: true
Attachment #646458 - Flags: review?(jones.chris.g)
Attachment #646458 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/mozilla-central/rev/1aa94c8daba8
https://hg.mozilla.org/mozilla-central/rev/35f29e0c5e18
https://hg.mozilla.org/mozilla-central/rev/1bf8b4433d17
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 10

5 years ago
I believe this may have caused bug 781414 and bug 778701
This code doesn't run on "desktop".  (Yet.)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
(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

5 years ago
yeah, i read through the code and figured that out,

then i traced tinderbox back to the async scrolling patch....
(Assignee)

Comment 15

5 years ago
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

5 years ago
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.