1001438 - Get rid of setCriticalDisplayPortForElement and automatically compute the low-res buffer area

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Split out from https://bugzilla.mozilla.org/show_bug.cgi?id=897996#c4. Quoting:

What I would like to do is remove the call from Fennec's browser.js and move it into gecko itself. So instead of the front-end setting both the critical displayport and the displayport, the front-end would just set a displayport. If the low-precision-buffer pref is set to true, then Gecko can use the set displayport as the critical displayport, and use an expanded rect as the displayport. The expansion could be somewhat pref-controlled but would live inside Gecko. If the pref is set to false then it just uses the displayport as the displayport.

Comment 1

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

Attachment: Part 1 - Rearrange GetDisplayPort

This is mostly code rearranging. The only functional change is that if frame is null I return false instead of returning true. I think this is more correct because in the pre-existing code aResult was never populated when this happened and so it seems wrong to return true anyway.

Comment 2

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

Attachment: Part 1 - Rearrange GetDisplayPort (ignore-whitespace diff)

Same patch as part 1 but ignoring whitespace changes so that it's easier to see.

Comment 3

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

Attachment: Part 2 - Change the low-res pref to a float

Seems simpler this way. If we're concerned about people having modified this pref locally and the new code barfing on old integer values, I can rename it too.

Comment 4

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

Attachment: Part 3 - Move the low-res displayport computation into gecko

Limited testing done so far, but has the desired effect on fennec in that the low-res buffer area is the same as it was before, and all the code is shoved into Gecko. Needs a try push and more testing before requesting review.

Comment 5

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

Comment on attachment 8412626 [details] [diff] [review]
Part 3 - Move the low-res displayport computation into gecko

Wouldn't mind some feedback though if you have any thoughts on arranging this code differently.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=dd60939a5eaf

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8412621 [details] [diff] [review]
Part 1 - Rearrange GetDisplayPort

So we'll get different answers if we ask for the rect vs not asking for the rect back? Can we cobble together some rect if we don't have a frame and return true? I don't know if this situation ever comes up though, so importance is probably low.

Comment 8

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

(In reply to Timothy Nikkel (:tn) from comment #7)
> So we'll get different answers if we ask for the rect vs not asking for the
> rect back? Can we cobble together some rect if we don't have a frame and
> return true? I don't know if this situation ever comes up though, so
> importance is probably low.

One option is to do the !frame check earlier in the function and return false up front. I would prefer that. The part 3 patch moves it up a bit, but it could be moved all the way to the top. If we want to leave it where it is then yeah we can probably just return a 0 rect or something.

Comment 9

[Timothy Nikkel (:tnikkel)]

We use existence of a displayport in some places to determine things, so I think I'd prefer returning true if a displayport is set and returning a "not quite right" rect.

Comment 10

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

Attachment: Part 1 - Rearrange GetDisplayPort

Updated part 1 to return true and populate aResult when !frame. Also log a warning for good measure.

Comment 11

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

Attachment: Part 1 - Rearrange GetDisplayPort (ignore-whitespace diff)

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8412879 [details] [diff] [review]
Part 1 - Rearrange GetDisplayPort

I think we should return the base rect in the no frame case instead of the empty rect, it'll at least encompass the visible part.

Comment 13

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

Attachment: Part 1 - Rearrange GetDisplayPort

Updated to return the base rect if !frame, carrying r+

Comment 14

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

Attachment: Part 3 - Move the low-res displayport computation into gecko

Rebased part 3, added UUID bump to domwindowutils. Seems to work fine on Fennec and B2G.

Comment 15

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=dd60939a5eaf

There is an Ripc failure on this try push that looks legit. I'll have to investigate that before this can land.

Comment 16

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> There is an Ripc failure on this try push that looks legit. I'll have to
> investigate that before this can land.

I can reproduce this locally by running:
EXTRA_TEST_ARGS="--setpref=layers.async-pan-zoom.enabled=true" mach reftest-ipc layout/reftests/reftest-sanity
on my Linux desktop build.

The problem is that the async-scroll-1a.html test sets overflow:hidden on the body, so CalculateExpandedScrollableRect for the page returns 1000 for the height of the page. The code in GetDisplayPortImpl that clips the displayport to the expanded scrollable rect used to run only on the margins version of the displayport, but my patch makes it run on the absolute-rect version of the displayport, and that crops the displayport from h=2000px to h=1000px.

I could just restore the old behaviour in my patch by guarding the clipping code with if (!rectData), or I could find some way to modify the test so that it works with the new code as well. (I tried this a little and couldn't find anything easily).

Comment 17

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

Attachment: Part 3 - Move the low-res displayport computation into gecko

After thinking about it a little more I think it makes sense to not do the clipping when we have a displayport set via setDisplayPortForElement because existing code that uses that function assumes it will behave the same as it did before. Also there's really no reason to make the change - I just did it initially because I felt it would be more consistent but it doesn't buy us anything in practice.

Updated try push at https://tbpl.mozilla.org/?tree=Try&rev=3b21f023ac96

Comment 18

[Chris Lord [:cwiiis]]

Comment on attachment 8412624 [details] [diff] [review]
Part 2 - Change the low-res pref to a float

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

LGTM. I wonder why it wasn't this way in the first place... Probably my mistake :p

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8413924 [details] [diff] [review]
Part 3 - Move the low-res displayport computation into gecko

>@@ -690,45 +692,51 @@ nsLayoutUtils::GetDisplayPort(nsIContent* aContent, nsRect *aResult)
>     // choose margins if equal priority
>     if (rectData->mPriority > marginsData->mPriority) {
>       marginsData = nullptr;
>     } else {
>       rectData = nullptr;
>     }
>   }
> 
>+  nsIFrame* frame = aContent->GetPrimaryFrame();
>+  if (!frame) {
>+    // Turns out we can't really compute it. Oops. We still should
>+    // return true and set aResult to something sane.
>+    NS_WARNING("Attempting to get a displayport from a content with no primary frame!");
>+    if (rectData) {
>+      *aResult = rectData->mRect;
>+    } else if (baseData) {
>+      *aResult = *baseData;
>+    } else {
>+      *aResult = nsRect();
>+    }
>+    return true;
>+  }

We don't need a frame for plain rects, so warning is a little too much. And we should probably take into account the float factor in this early return.

>+  bool isRoot = false;
>+  if (aContent->OwnerDoc()->GetRootElement() == aContent) {
>+    // We want the scroll frame, the root scroll frame differs from all
>+    // others in that the primary frame is not the scroll frame.
>+    frame = frame->PresContext()->PresShell()->GetRootScrollFrame();
>+    isRoot = true;
>+  }
>+  nsIScrollableFrame* scrollableFrame = frame->GetScrollTargetFrame();
>+  nsPoint scrollPos(scrollableFrame ? scrollableFrame->GetScrollPosition() : nsPoint(0,0));
>+
>+  nsRect result;
>   if (rectData) {
>     // We have it as a straight-up rect
>-    *aResult = rectData->mRect;
>+    result = rectData->mRect;
>   } else {
>     // We need to compute it from the margins + base rect
>-    nsRect* baseData =
>-      static_cast<nsRect*>(aContent->GetProperty(nsGkAtoms::DisplayPortBase));
>     nsRect base;
>     if (baseData) {
>       base = *baseData;
>     }
> 
>-    nsIFrame* frame = aContent->GetPrimaryFrame();
>-    if (!frame) {
>-      // Turns out we can't really compute it. Oops. We still should
>-      // return true and set aResult to something sane.
>-      NS_WARNING("Attempting to get a displayport from a content with no primary frame!");
>-      *aResult = base;
>-      return true;
>-    }
>-
>-    bool isRoot = false;
>-    if (aContent->OwnerDoc()->GetRootElement() == aContent) {
>-      // We want the scroll frame, the root scroll frame differs from all
>-      // others in that the primary frame is not the scroll frame.
>-      frame = frame->PresContext()->PresShell()->GetRootScrollFrame();
>-      isRoot = true;
>-    }
>-
>     // first convert the base rect to layer pixels
>     nsPresContext* presContext = frame->PresContext();
>     int32_t auPerDevPixel = presContext->AppUnitsPerDevPixel();
>     gfxSize res = presContext->PresShell()->GetCumulativeResolution();
>     gfxSize parentRes = res;
>     if (isRoot) {
>       // the base rect for root scroll frames is specified in the parent document
>       // coordinate space, so it doesn't include the local resolution.
>@@ -739,18 +747,16 @@ nsLayoutUtils::GetDisplayPort(nsIContent* aContent, nsRect *aResult)
>     LayerRect rect;
>     rect.x = parentRes.width * NSAppUnitsToFloatPixels(base.x, auPerDevPixel);
>     rect.y = parentRes.height * NSAppUnitsToFloatPixels(base.y, auPerDevPixel);
>     rect.width = parentRes.width * NSAppUnitsToFloatPixels(base.width, auPerDevPixel);
>     rect.height = parentRes.height * NSAppUnitsToFloatPixels(base.height, auPerDevPixel);
> 
>     rect.Inflate(marginsData->mMargins);
> 
>-    nsIScrollableFrame* scrollableFrame = frame->GetScrollTargetFrame();
>-    nsPoint scrollPos(scrollableFrame ? scrollableFrame->GetScrollPosition() : nsPoint(0,0));
>     if (marginsData->mAlignmentX > 0 || marginsData->mAlignmentY > 0) {
>       // Avoid division by zero.
>       if (marginsData->mAlignmentX == 0) {
>         marginsData->mAlignmentX = 1;
>       }
>       if (marginsData->mAlignmentY == 0) {
>         marginsData->mAlignmentY = 1;
>       }
>@@ -769,31 +775,51 @@ nsLayoutUtils::GetDisplayPort(nsIContent* aContent, nsRect *aResult)
>       float left = marginsData->mAlignmentX * floor(rect.x / marginsData->mAlignmentX);
>       float top = marginsData->mAlignmentY * floor(rect.y / marginsData->mAlignmentY);
>       float right = marginsData->mAlignmentX * ceil(rect.XMost() / marginsData->mAlignmentX);
>       float bottom = marginsData->mAlignmentY * ceil(rect.YMost() / marginsData->mAlignmentY);
>       rect = LayerRect(left, top, right - left, bottom - top);
>       rect -= scrollPosLayer;
>     }
> 
>-    nsRect result;
>     result.x = NSFloatPixelsToAppUnits(rect.x / res.width, auPerDevPixel);
>     result.y = NSFloatPixelsToAppUnits(rect.y / res.height, auPerDevPixel);
>     result.width = NSFloatPixelsToAppUnits(rect.width / res.width, auPerDevPixel);
>     result.height = NSFloatPixelsToAppUnits(rect.height / res.height, auPerDevPixel);
>+  }
> 
>-    // Finally, clamp the display port to the expanded scrollable rect.
>-    nsRect expandedScrollableRect = CalculateExpandedScrollableRect(frame);
>-    result = expandedScrollableRect.Intersect(result + scrollPos) - scrollPos;
>+  if (aMultiplier != 1.0f) {
>+    // This code should only get run to compute the low-precision paint area when low-precision
>+    // painting is enabled.
>+    float newWidth = result.width * aMultiplier;
>+    float newHeight = result.height * aMultiplier;
>+    float newX = result.x - ((newWidth - result.width) / 2.0f);
>+    float newY = result.y - ((newHeight - result.height) / 2.0f);
>+    result = nsRect(floor(newX), floor(newY), floor(newWidth), floor(newHeight));
>+  }

Nit: any particular reason to use floor for all of them? It's in appunits so rounding likely doesn't matter but we should probably use one of the rounding idioms we have already: inside, outside, or nearest.

>-    *aResult = result;
>+  if (marginsData) {
>+    // Finally, if the displayport was set via margins, clamp it to the expanded scrollable rect.
>+    nsRect expandedScrollableRect = nsLayoutUtils::CalculateExpandedScrollableRect(frame);
>+    result = expandedScrollableRect.Intersect(result + scrollPos) - scrollPos;
>   }

So these loses all of the clamping to the pagerect if the float factor is > 1 that was in the old js code. Intentional?

Comment 20

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

Attachment: Part 3 - Move the low-res displayport computation into gecko (v2)

(In reply to Timothy Nikkel (:tn) from comment #19)
> 
> We don't need a frame for plain rects, so warning is a little too much. And
> we should probably take into account the float factor in this early return.

Updated

> Nit: any particular reason to use floor for all of them? It's in appunits so
> rounding likely doesn't matter but we should probably use one of the
> rounding idioms we have already: inside, outside, or nearest.

Ok, changed to round-in. I didn't use any of the existing functions on BaseRect because none of them does exactly what I want (which is some combination of Inflate and ScaleRoundIn). I did extract a helper for it because of the first comment above.

> So these loses all of the clamping to the pagerect if the float factor is >
> 1 that was in the old js code. Intentional?

It doesn't lose all of the clamping, it just changes the clamping behaviour. Rather than moving the rect to be within the pagerect, it clips it to the pagerect. This is intentional, yes. It does mean we have a smaller low-res buffer area when we are the end of the page, but in practice this should save us some painting with no noticeable increase in checkerboarding.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> > So these loses all of the clamping to the pagerect if the float factor is >
> > 1 that was in the old js code. Intentional?
> 
> It doesn't lose all of the clamping, it just changes the clamping behaviour.
> Rather than moving the rect to be within the pagerect, it clips it to the
> pagerect. This is intentional, yes. It does mean we have a smaller low-res
> buffer area when we are the end of the page, but in practice this should
> save us some painting with no noticeable increase in checkerboarding.

Maybe my brain is mis-firing but I'm missing the part where it clamps if we just have rect data and not margin data.

Comment 22

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

Oh, I see what you mean. Yeah I guess that's a mistake. I can do the clamp unconditionally but I suspect it will cause the test from comment 16 to break again. I'll probably need to modify that test too.

Comment 23

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

Attachment: Part 4 - Switch Fennec to using margins-based displayport setter

This should address tn's outstanding comment from part 3, by making Fennec also use the margins-based displayport setter. This has the bonus of getting rid of the dirtist hack ever because now the tile-alignment that is done in DisplayPortCalculator is passed unmodified through gecko (because stays in layer pixels the whole way). I could probably even take out the alignment from DisplayPortCalculator entirely and let the code in GetDisplayPortImpl handle it, but I think the return value from DisplayPortCalculator may be used in other places as well and so I'd like to defer that change to a follow-up bug.

Try push with all 4 patches: https://tbpl.mozilla.org/?tree=Try&rev=36145a10c26f

Comment 24

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

Comment on attachment 8419474 [details] [diff] [review]
Part 4 - Switch Fennec to using margins-based displayport setter

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

::: mobile/android/chrome/content/browser.js
@@ +3284,5 @@
> +      cwu.setDisplayPortMarginsForElement(displayPortMargins.left,
> +                                          displayPortMargins.top,
> +                                          displayPortMargins.right,
> +                                          displayPortMargins.bottom,
> +                                          256, 256,

Oh, I forgot to replace these with the pref values. I'll implement that the same way defaultBrowserWidth getter in browser.js works, where it reads the pref the first time and caches that value forever.

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8419474 [details] [diff] [review]
Part 4 - Switch Fennec to using margins-based displayport setter

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

Much nicer :)

Comment 26

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

Attachment: Part 4 - Switch Fennec to using margins-based displayport setter

Updated to use prefs instead of hard-coding 256. I had to add the prefs to all.js because otherwise the defaults are only defined in gfxPrefs.h which doesn't get picked up from JS. Carrying r+

Comment 27

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

Note: a try push with the latest patches also shows the same Android 4.0 Debug mochitest failures that were present in the try push in comment 23. There are some assertions being tripped by the new code and those are causing the failures. I'll need to fix those before this can land.

https://tbpl.mozilla.org/?tree=Try&rev=3f6ed93165fb

Comment 28

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

I'm unable to reproduce these failures on a local build, so I've been trying to debug it by code inspection. The problem is that we hit the assertion at [1] and that spits a thing to the log that makes the test fail. That function is only called from a few places, and the only callsite that I could find that passes in an empty aPaintRegion is the one at [2]. And that really looks like a typo, since it's passing in the same thing for both the valid and paint regions.

Chris, is the code at [2] right?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp?rev=9e5a13343d60#640
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledThebesLayer.cpp?rev=bd59c5ef0677#396

Comment 29

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> I'm unable to reproduce these failures on a local build, so I've been trying
> to debug it by code inspection. The problem is that we hit the assertion at
> [1] and that spits a thing to the log that makes the test fail. That
> function is only called from a few places, and the only callsite that I
> could find that passes in an empty aPaintRegion is the one at [2]. And that
> really looks like a typo, since it's passing in the same thing for both the
> valid and paint regions.
> 
> Chris, is the code at [2] right?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TiledContentClient.cpp?rev=9e5a13343d60#640
> [2]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> ClientTiledThebesLayer.cpp?rev=bd59c5ef0677#396

Yes, it is intended - we want to set that valid region and it's also the 'painted' region, i.e. the region we want to validate in the tiled buffer. I guess I didn't consider that an empty painted region is considered to be invalid input. In the case that it's empty though, we can probably do something quicker here, like just recreating the buffer.

Comment 30

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

Ok, I split that into bug 1009184 because technically it's a bug even without my patches here.

Comment 31

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

tn, any ETA on the pending review here?

Comment 32

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

Most recent try push with latest versions of all the dependent patches and with low-res + progressive enabled on B2G: https://tbpl.mozilla.org/?tree=Try&rev=94c29f81bc15

Comment 33

[Timothy Nikkel (:tnikkel)]

Today hopefully. Sorry about that.

Comment 34

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8418153 [details] [diff] [review]
Part 3 - Move the low-res displayport computation into gecko (v2)

The display port getter is getting very complicated. It has: early returns, rect data vs margin data, frame vs no frame, low prec expansion vs not, clamping vs not.

Would it be possible to refactor to simplify some of this? Maybe we could use some helper functions to cut down on the nesting in the main getter and then we don't need to use early returns (that require something to be done before returning).

Comment 35

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

Comment on attachment 8418153 [details] [diff] [review]
Part 3 - Move the low-res displayport computation into gecko (v2)

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

Absolutely. Is it ok if I pile on a new patch at the end, or should I refactor the existing patch series?

Comment 36

[Timothy Nikkel (:tnikkel)]

A new patch is okay.

Comment 37

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

Attachment: Part 5 - Refactor GetDisplayPort code to be more readable

It's probably easier to not read this as a diff and just read through the new code to see if it makes sense.

Comment 38

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8423122 [details] [diff] [review]
Part 5 - Refactor GetDisplayPort code to be more readable

>+GetDisplayPortImpl(nsIContent* aContent, nsRect *aResult, float aMultiplier)

>+  if (!rectData && !marginsData) {
>+    // This content element has no margins data at all
>+    return false;
>+  }

Fix the comment here please.

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8418153 [details] [diff] [review]
Part 3 - Move the low-res displayport computation into gecko (v2)

This should be good now with the two later parts fixing things up.

Comment 40

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

Addressed comment 38 and landed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d1ccf787129
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7833f01b6553
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a43da969d386
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/937f8aa9faed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc1b283e10d

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d1ccf787129
https://hg.mozilla.org/mozilla-central/rev/7833f01b6553
https://hg.mozilla.org/mozilla-central/rev/a43da969d386
https://hg.mozilla.org/mozilla-central/rev/937f8aa9faed
https://hg.mozilla.org/mozilla-central/rev/bfc1b283e10d