Closed Bug 1001438 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(5 files, 8 obsolete files)

4.14 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
9.57 KB, patch
kats
: review+
Details | Diff | Splinter Review
17.32 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
13.15 KB, patch
kats
: review+
Details | Diff | Splinter Review
12.94 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8412621 - Flags: review?(tnikkel)
Same patch as part 1 but ignoring whitespace changes so that it's easier to see.
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.
Attachment #8412624 - Flags: review?(chrislord.net)
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 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.
Attachment #8412626 - Flags: feedback?(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.
(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.
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.
Updated part 1 to return true and populate aResult when !frame. Also log a warning for good measure.
Attachment #8412621 - Attachment is obsolete: true
Attachment #8412621 - Flags: review?(tnikkel)
Attachment #8412879 - Flags: review?(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.
Attachment #8412879 - Flags: review?(tnikkel) → review+
Updated to return the base rect if !frame, carrying r+
Attachment #8412879 - Attachment is obsolete: true
Attachment #8412881 - Attachment is obsolete: true
Attachment #8413724 - Flags: review+
Rebased part 3, added UUID bump to domwindowutils. Seems to work fine on Fennec and B2G.
Attachment #8412626 - Attachment is obsolete: true
Attachment #8412626 - Flags: feedback?(tnikkel)
Attachment #8413725 - Flags: review?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
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
Attachment #8413725 - Attachment is obsolete: true
Attachment #8413725 - Flags: review?(tnikkel)
Attachment #8413924 - Flags: review?(tnikkel)
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
Attachment #8412624 - Flags: review?(chrislord.net) → review+
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?
(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.
Attachment #8413924 - Attachment is obsolete: true
Attachment #8413924 - Flags: review?(tnikkel)
Attachment #8418153 - Flags: review?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
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
Attachment #8419474 - Flags: review?(chrislord.net)
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 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 :)
Attachment #8419474 - Flags: review?(chrislord.net) → review+
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+
Attachment #8419474 - Attachment is obsolete: true
Attachment #8419515 - Flags: review+
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
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
Flags: needinfo?(chrislord.net)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(chrislord.net)
Depends on: 1009184
Ok, I split that into bug 1009184 because technically it's a bug even without my patches here.
tn, any ETA on the pending review here?
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
Today hopefully. Sorry about that.
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 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?
Attachment #8418153 - Flags: review?(tnikkel) → review-
A new patch is okay.
It's probably easier to not read this as a diff and just read through the new code to see if it makes sense.
Attachment #8423122 - Flags: review?(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.
Attachment #8423122 - Flags: review?(tnikkel) → review+
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.
Attachment #8418153 - Flags: review- → review+
Depends on: 1014054
Depends on: 1086683
You need to log in before you can comment on or make changes to this bug.