Doug Sherk (:drs) (inactive) Assignee
	Description • 12 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.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 1 • 12 years ago

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.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 2 • 12 years ago

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.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 3 • 12 years ago

Now that we have this new implementation, the old one based on throttling with minimum time deltas between repaint requests can be removed.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 4 • 12 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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 5 • 12 years ago

Andreas can you give this a spin and see if it fixes some of the lagginess you were seeing?

	Andreas Gal :gal
	Comment 6 • 12 years ago

Will do in an hour.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 7 • 12 years ago

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.

	Ed Morley [:emorley]
	Comment 9 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/1aa94c8daba8
https://hg.mozilla.org/mozilla-central/rev/35f29e0c5e18
https://hg.mozilla.org/mozilla-central/rev/1bf8b4433d17

	Danial Horton
	Comment 10 • 12 years ago

I believe this may have caused bug 781414 and bug 778701

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 11 • 12 years ago

This code doesn't run on "desktop".  (Yet.)

	Danial Horton
	Comment 12 • 12 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.

	Doug Sherk (:drs) (inactive) Assignee
	Comment 13 • 12 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.

	Danial Horton
	Comment 14 • 12 years ago

yeah, i read through the code and figured that out,

then i traced tinderbox back to the async scrolling patch....

	Doug Sherk (:drs) (inactive) Assignee
	Comment 15 • 12 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.

	Danial Horton
	Comment 16 • 12 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?