Closed Bug 979084 Opened 10 years ago Closed 10 years ago

Display port easily grows too big on b2g with APZC and double-buffered tiles

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(2 files, 2 obsolete files)

Replace apz.enlarge_displayport_when_only_scrollable with apz.enlarge_displayport_when_clipped
10.82 KB, patch
ajones
: review+
Details | Diff | Splinter Review
Enable apz.enlarge_displayport_when_clipped by default on b2g
1.50 KB, patch
ajones
: review+
Details | Diff | Splinter Review 
Our current display port numbers apply a multiplier of 3.5 to the vertical axis and 3.0 to the horizontal axis. I believe that when these numbers are applied simultaneously, especially when using double-buffered tiles, we end up using too much memory.

On the most part, on a 320x480 display it's not too bad. Taking the worst case:
320x480 -> 960x1680 -> 1024x1792 (tile alignment)
 = 28 tiles, worst-case 56 (double-buffering)
 = 7mb, 14mb per scrollable layer

We can probably spare that amount on a 256mb device, but it isn't ideal.

On a 960x540 display (like the Peak, but also quite similar to some of the upcoming devices):
540x960 -> 1620x3360 -> 1792x3584
 = 98 tiles, 196 double-buffered
 = 24.5mb, 49mb

Which is way, way too much, even on a 512mb device.


I think we should do what we do on Android, where when horizontal scrolling won't/can't happen (either axis lock or a narrow page), the space we would've assigned to that axis gets reassigned to the vertical axis (and vice-versa). We should then balance the numbers so that the scrollable axis gets 3.5x displayport in the best case (when there's 1.0x on the other axis), but that provides a more sensible balance when there's possible scrolling in all directions.

Of course, these numbers should likely take into account device metrics (speed, available ram, screen resolution/size), but this would be a reasonable first step and has served us well on Android so far.
Just to note, about the doubling figures, this is a cost we can't really avoid. We either single-buffer with a copy in system memory and another in texture memory (so doubling), or we double-buffer with both buffers in video memory and none in system memory.

As video and system memory are shared, the same amount of memory gets used, but on some platforms this memory is split (such as the Geeksphone Peak), with less memory assigned to graphics, so on some platforms, double-buffered, gralloc-backed tiles can exacerbate this issue.
If we have two buffers in system memory then we could conceivably use COW. Are there shared memory systems where we can use COW in cases where we're painting with the CPU?
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)
> If we have two buffers in system memory then we could conceivably use COW.
> Are there shared memory systems where we can use COW in cases where we're
> painting with the CPU?

I'm not sure that this could help us - the only times we're double-buffered are when both buffers are memory-mapped graphics memory, in which case I guess we're already using COW in a way? I'm not sure I've understood you correctly.

Either way, if you feel there's a win to be had there, let's open a new bug and keep this one on track :)
I'm currently blocked and this is a quick fix, taking.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
This replaces the unused apz.enlarge_displayport_when_only_scrollable (unused in that it doesn't appear in the Gecko or Gaia tree anywhere except here) with apz.enlarge_displayport_when_clipped, which does a similar but more comprehensive thing (as well as checking whether scrolling is disabled, it also checks to see if the displayport will be clipped by the scrollable rect).

I've left it off in this patch.
Attachment #8386112 - Flags: review?(ajones)
This enables the setting by default and also adjusts the default displayport values so that they provide roughly similar results as before, except in the situation where both axes are scrollable and the displayport is smaller.

This patch lets me zoom into pages now without causing OOMs on buri (I've yet to test on Peak, but I assume it will have the same result there too).
Attachment #8386116 - Flags: review?(ajones)
Attachment #8386116 - Flags: feedback?(botond)
Heh, the default settings are still too much for the Peak, but then I guess what do you expect with a qHD screen and 128mb of video memory... I'm just double-checking to make sure that everything is working as expected.
I can confirm that this code at least is working correctly. May well be issues elsewhere, however...
Blocks: b2g-tiling
As it happens, the rest of the issues on Peak were a combination of [1] and [2].

Although things are now mostly ok without fixing [2], we could easily still OOM if you made a more concerted effort, or had a very layer-heavy page (we're right on its limits when the displayport gets enlarged on big zooms).

[1] http://hg.mozilla.org/users/bschouten_mozilla.com/tiling/rev/d712f0fbe690
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=979973
Comment on attachment 8386116 [details] [diff] [review]
Enable apz.enlarge_displayport_when_clipped by default

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

This is fine in principle. We should assess its effect on checkerboarding before landing.
Attachment #8386116 - Flags: feedback?(botond) → feedback+
Comment on attachment 8386112 [details] [diff] [review]
Replace apz.enlarge_displayport_when_only_scrollable with apz.enlarge_displayport_when_clipped

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

I'm happy to accept these changes and suggestions as a follow up patch on the basis that the logic all looks correct.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1368,4 @@
>    EnlargeDisplayPortAlongAxis(&(displayPort.y), &(displayPort.height),
> +    velocity.y, gYStationarySizeMultiplier, ySkateSizeMultiplier);
> +
> +  if (gEnlargeDisplayPortWhenClipped) {

Suggestion: This whole chunk should be a separate function.

@@ +1376,5 @@
> +    if (aFrameMetrics.GetDisableScrollingX()) {
> +      xSlack = displayPort.width - compositionBounds.width;
> +    } else {
> +      xSlack = std::max(0.0f, displayPort.width - scrollableRect.width);
> +    }

Suggestion: You get less duplication from:

  xSlack = std::max(0.0f, displayPort.width - 
                          (aFrameMetrics.GetDisableScrollingX() ?
                           compositionBounds.width :
                           scrollableRect.width));

It is up to you whether you consider it to be simpler or not.

@@ +1392,5 @@
> +      displayPort.x -= xExtra / 2;
> +      displayPort.width += xExtra;
> +    } else if (xSlack > 0) {
> +      // Reassign wasted x-axis displayport to the y-axis
> +      displayPort.x += xSlack / 2;

We could eliminate this adjustment and the one in EnlargeDisplayPortAlongAxis by calculating is at the end. Consider putting this at the end:

  auto adjustment = (displayPort.Size() - compositionBounds.Size()) / 2;
  displayPort.MoveTo(scrollOffset + Point(adjustment.width, adjustment.height));

You could consider only calculating displayPortSize through this code and finishing initialising the DisplayPort as a rectangle at the end.

@@ +1405,5 @@
> +  // estimated time it takes to paint, to try to minimise checkerboarding.
> +  OffsetDisplayPortAlongAxis(&(displayPort.x),
> +    estimatedPaintDurationMillis, velocity.x);
> +  OffsetDisplayPortAlongAxis(&(displayPort.y),
> +    estimatedPaintDurationMillis, velocity.y);

I don't really like unnecessarily splitting the x and y into components. It would be better to get rid of OffsetDisplayPortAlongAxis and use something like:

 displayPort.MoveBy(velocity * gVelocityBias * (gUsePaintDuration ?
                                                aEstimatedPaintDurationMillis :
                                                50.0));

It could be a function that replaces OffsetDislpayPortAlongAxis.
Attachment #8386112 - Flags: review?(ajones) → review+
Attachment #8386116 - Flags: review?(ajones) → review+
A thorough review, thank you :)

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #11)
> Comment on attachment 8386112 [details] [diff] [review]
> Replace apz.enlarge_displayport_when_only_scrollable with
> apz.enlarge_displayport_when_clipped
> 
> Review of attachment 8386112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm happy to accept these changes and suggestions as a follow up patch on
> the basis that the logic all looks correct.
> 
> ::: gfx/layers/ipc/AsyncPanZoomController.cpp
> @@ +1368,4 @@
> >    EnlargeDisplayPortAlongAxis(&(displayPort.y), &(displayPort.height),
> > +    velocity.y, gYStationarySizeMultiplier, ySkateSizeMultiplier);
> > +
> > +  if (gEnlargeDisplayPortWhenClipped) {
> 
> Suggestion: This whole chunk should be a separate function.

Agreed, will change.

> @@ +1376,5 @@
> > +    if (aFrameMetrics.GetDisableScrollingX()) {
> > +      xSlack = displayPort.width - compositionBounds.width;
> > +    } else {
> > +      xSlack = std::max(0.0f, displayPort.width - scrollableRect.width);
> > +    }
> 
> Suggestion: You get less duplication from:
> 
>   xSlack = std::max(0.0f, displayPort.width - 
>                           (aFrameMetrics.GetDisableScrollingX() ?
>                            compositionBounds.width :
>                            scrollableRect.width));
> 
> It is up to you whether you consider it to be simpler or not.

Hmm... I like fewer characters, but I think I'd err on the side of readability here.

> @@ +1392,5 @@
> > +      displayPort.x -= xExtra / 2;
> > +      displayPort.width += xExtra;
> > +    } else if (xSlack > 0) {
> > +      // Reassign wasted x-axis displayport to the y-axis
> > +      displayPort.x += xSlack / 2;
> 
> We could eliminate this adjustment and the one in
> EnlargeDisplayPortAlongAxis by calculating is at the end. Consider putting
> this at the end:
> 
>   auto adjustment = (displayPort.Size() - compositionBounds.Size()) / 2;
>   displayPort.MoveTo(scrollOffset + Point(adjustment.width,
> adjustment.height));
> 
> You could consider only calculating displayPortSize through this code and
> finishing initialising the DisplayPort as a rectangle at the end.

Yes, this seems like a decent idea, will do that.

> @@ +1405,5 @@
> > +  // estimated time it takes to paint, to try to minimise checkerboarding.
> > +  OffsetDisplayPortAlongAxis(&(displayPort.x),
> > +    estimatedPaintDurationMillis, velocity.x);
> > +  OffsetDisplayPortAlongAxis(&(displayPort.y),
> > +    estimatedPaintDurationMillis, velocity.y);
> 
> I don't really like unnecessarily splitting the x and y into components. It
> would be better to get rid of OffsetDisplayPortAlongAxis and use something
> like:
> 
>  displayPort.MoveBy(velocity * gVelocityBias * (gUsePaintDuration ?
>                                                
> aEstimatedPaintDurationMillis :
>                                                 50.0));
> 
> It could be a function that replaces OffsetDisplayPortAlongAxis.

This goes well with the previous suggestion, will do.
(In reply to Botond Ballo [:botond] from comment #10)
> Comment on attachment 8386116 [details] [diff] [review]
> Enable apz.enlarge_displayport_when_clipped by default
> 
> Review of attachment 8386116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine in principle. We should assess its effect on checkerboarding
> before landing.

Any suggestions on how to do this before landing? I see roughly equivalent checkerboarding on pages with constrained horizontal scrolling, and more checkerboarding when scrolling is possible in all directions (though it also doesn't crash, so that's nice :)).

Fixing bug 979973 may allow us to make the displayport a bit bigger. We may also want to consider removing, or lessening the vertical bias of the displayport, given that there'll only be horizontal space if there is horizontal scrolling possible.
Comment on attachment 8386116 [details] [diff] [review]
Enable apz.enlarge_displayport_when_clipped by default

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +291,5 @@
>   * Pref that enables enlarging of the displayport along one axis when the
>   * generated displayport's size is beyond that of the scrollable rect on the
>   * opposite axis.
>   */
> +static bool gEnlargeDisplayPortWhenClipped = true;

Please make this change in the b2g prefs file. I don't want it affect metro without proper testing there.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Please make this change in the b2g prefs file. I don't want it affect metro
> without proper testing there.

Err actually since all of the changes in this patch go together, please make them all in the b2g prefs file.
Comment on attachment 8386112 [details] [diff] [review]
Replace apz.enlarge_displayport_when_only_scrollable with apz.enlarge_displayport_when_clipped

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

Some additional comments on this patch.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1349,5 @@
>  
>    CSSRect scrollableRect = aFrameMetrics.GetExpandedScrollableRect();
>  
>    float xSkateSizeMultiplier = gXSkateSizeMultiplier,
>          ySkateSizeMultiplier = gYSkateSizeMultiplier;

Get rid of these and replace the uses with g[XY]SkateSizeMultiplier. The code that modified the local variables is removed in this patch so these are no longer needed.

@@ +1372,5 @@
> +    // If the displayport has been enlarged beyond the scrollable rect on one
> +    // axis, or that axis is not scrollable, try to reassign that extra space
> +    // to the other axis, whilst maintaining the total displayport area.
> +    float xSlack, ySlack;
> +    if (aFrameMetrics.GetDisableScrollingX()) {

Note that my patch in bug 975962 removes GetDisableScrollingX and GetDisableScrollingY. Whoever lands second will need to rebase.
(In reply to Chris Lord [:cwiiis] from comment #13)
> (In reply to Botond Ballo [:botond] from comment #10)
> > This is fine in principle. We should assess its effect on checkerboarding
> > before landing.
> 
> Any suggestions on how to do this before landing?

I was thinking of bug 969466. On second thought, though, since that is yet to be implemented, and crashing is worse than checkerboarding, it probably makes more sense to do that as a follow-up.
Implemented most of your suggestions, I think.
Attachment #8386112 - Attachment is obsolete: true
Attachment #8387225 - Flags: review?(ajones)
Do the same as before, but for b2g only, via prefs.
Attachment #8386116 - Attachment is obsolete: true
Attachment #8387228 - Flags: review?(ajones)
Comment on attachment 8387225 [details] [diff] [review]
Replace apz.enlarge_displayport_when_only_scrollable with apz.enlarge_displayport_when_clipped

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

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1391,5 @@
> +                                displayPortSize);
> +
> +  // Re-center the displayport based on its expansion over the composition bounds.
> +  displayPort.MoveBy((displayPort.width - compositionBounds.width)/2.0f,
> +                     (displayPort.height - compositionBounds.height)/2.0f);

These are the wrong way round, pretend they're correct :)
Comment on attachment 8387225 [details] [diff] [review]
Replace apz.enlarge_displayport_when_only_scrollable with apz.enlarge_displayport_when_clipped

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

Great work. Thanks for the updated patch. Feel free to just land this patch as is with your correctness change.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1308,5 @@
> +                         const CSSPoint& aVelocity)
> +{
> +  float xMultiplier = (fabsf(aVelocity.x) < gMinSkateSpeed
> +                        ? gXStationarySizeMultiplier
> +                        : gXSkateSizeMultiplier);

Suggestion: Remove unnecessary brackets.

@@ +1343,5 @@
>  
> +  if (ySlack > 0) {
> +    // Reassign wasted y-axis displayport to the x-axis
> +    aDisplayPortSize.height -= ySlack;
> +    float xExtra = ySlack * (aDisplayPortSize.width / aDisplayPortSize.height);

Suggestion: Remove unnecessary brackets.

@@ +1391,5 @@
> +                                displayPortSize);
> +
> +  // Re-center the displayport based on its expansion over the composition bounds.
> +  displayPort.MoveBy((displayPort.width - compositionBounds.width)/2.0f,
> +                     (displayPort.height - compositionBounds.height)/2.0f);

Suggestion: I'm not sure if it is worth it but this gives you slightly stronger typing.

CSSSize sizeDiff = compositionBounds.Size() - displayPort.Size();
displayPort.MoveBy(sizeDiff.width / 2.0f, sizeDiff.height / 2.0f);
Attachment #8387225 - Flags: review?(ajones) → review+
Comment on attachment 8387228 [details] [diff] [review]
Enable apz.enlarge_displayport_when_clipped by default on b2g

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

I'm happy with this on the basis that you're following kats' suggestion.
Attachment #8387228 - Flags: review?(ajones) → review+
Removed unnecessary parentheses and pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1371ee7744
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03a1df22a53b

I didn't think the sizeDiff change was worth it, but we can always fix it in a follow-up if you disagree.
https://hg.mozilla.org/mozilla-central/rev/3b1371ee7744
https://hg.mozilla.org/mozilla-central/rev/03a1df22a53b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: