967449 - Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Doug Sherk (:drs) (inactive)]

If a frame's scrollable rect is constrained on either axis to the frame's composition bounds, then that frame must not be scrollable along that axis. In this case, the displayport also can't be enlarged along this axis past the composition bounds. This gives us some extra freedom to enlarge the opposite axis, since we have more memory available. We should enlarge the displayport along the opposite axis in this case.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

Comment 2

[Doug Sherk (:drs) (inactive)]

This slightly improves checkerboarding, though only when we're stationary.

Comment 3

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #2)
> This slightly improves checkerboarding, though only when we're stationary.

I meant going from stationary to moving as opposed to already panning or skating :)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8369986 [details] [diff] [review]
Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

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

Would like to see the updated patch before +'ing.

Also put the entire commit message on one line.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1311,5 @@
> +
> +  float xStationarySizeMultiplier = gXStationarySizeMultiplier,
> +        xSkateSizeMultiplier = gXSkateSizeMultiplier,
> +        yStationarySizeMultiplier = gYStationarySizeMultiplier,
> +        ySkateSizeMultiplier = gYSkateSizeMultiplier;

You don't really need the *SkateSizeMultiplier local variables; you can just pass in the global ones to the EnlargeDisplayPortAlongAxis functions since they're never modified.

@@ +1316,5 @@
> +
> +  // Check if the displayport, without being enlarged, fits tightly inside the
> +  // scrollable rect. If so, we're not going to be able to enlarge it, so we
> +  // should consider enlarging the opposite axis.
> +  if (scrollableRect.width - compositionBounds.width <= EPSILON) {

I'd prefer to duplicate the conditionals rather than duplicate the body, to encapsulate this code better. That is, do:

if (aFrameMetrics.GetDisableScrollingX() || (scrollableRect.width - compositionBounds.width <= EPSILON)) {
   ...
}

...

if (aFrameMetrics.GetDisableScrollingX()) {
  velocity.x = 0;
}


That way you can wrap the entire block of "new" code in a pref. In this case I don't mind making the pref enabled by default since I don't think it has much potential for regression.

Comment 5

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8369986 [details] [diff] [review]
> Enlarge the opposite axis's displayport when an axis's scrollable rect <=
> composition bounds rect
> 
> Review of attachment 8369986 [details] [diff] [review]:

Ok, makes sense, except...

> You don't really need the *SkateSizeMultiplier local variables; you can just
> pass in the global ones to the EnlargeDisplayPortAlongAxis functions since
> they're never modified.

Could you clarify please? I'm using them and they do get modified at runtime.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

xSkateSizeMultiplier and ySkateSizeMultiplier are never assigned to beyond the initial assignment. You modify xStationarySizeMultipler and yStationarySizeMultiplier only.

Comment 7

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> xSkateSizeMultiplier and ySkateSizeMultiplier are never assigned to beyond
> the initial assignment. You modify xStationarySizeMultipler and
> yStationarySizeMultiplier only.

Oh yeah, you're right, OK.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

Addressed review comments.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8370114 [details] [diff] [review]
Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

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

Much better, thanks!

Comment 10

[Mike Lee [:mlee]]

1.3+: blocking two other blockers.

Comment 11

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

I found that this wasn't actually improving checkerboarding, but I tweaked it to tune the skating multiplier instead of the stationary multiplier, and now it definitely is.

Not much changed, but you might want to review it again anyways. For summary, s/xStationaryMultiplier/xSkateMultiplier/ and s/yStationaryMultiplier/ySkateMultiplier/.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8370743 [details] [diff] [review]
Enlarge the opposite axis's displayport when an axis's scrollable rect <= composition bounds rect

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

r=me if you default the pref to false. I'm still a bit leery of increasing the displayport size while scrolling. Chris confirmed yesterday that doing that seems to make things worse at least in some cases. Also doing this by default largely negates having separate multipliers for stationary and skating to begin with.

Comment 14

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> r=me if you default the pref to false.

Sure, done.

https://hg.mozilla.org/integration/b2g-inbound/rev/2f2299a55c68

Comment 15

[Doug Sherk (:drs) (inactive)]

For reference, I noticed that this works better on pages that are more predictable. For example, this improves the SMS app more than the call log or contacts (probably because of the images). There's work going into using thumbnails instead of downscaled images for these apps, so they should run smoother after that too.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f2299a55c68

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/93c27e195d94

Do we need this on any other branches?

Comment 18

[Doug Sherk (:drs) (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #17)
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/93c27e195d94
> 
> Do we need this on any other branches?

Not that I'm aware of. The only important one is b2g 1.3.