Align calculated display-ports to tile boundaries

RESOLVED FIXED in mozilla28

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
mozilla28
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [release28])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
For efficient tiled updates, display-port boundaries should align to tile boundaries. Android already does this.
(Assignee)

Updated

5 years ago
Blocks: 897994
(Assignee)

Comment 1

5 years ago
Created attachment 796855 [details] [diff] [review]
APZC align display port to tile when tiled layers are enabled

This is pretty much a straight port of the same code in fennec's browser.js. I've tested and verified it works (which is good for fennec too, assuming my port is accurate).

Sorry to burden you with the review, but I get the feeling that no one else is as suitably qualified for this particular piece of code...
Attachment #796855 - Flags: review?(bugmail.mozilla)
Comment on attachment 796855 [details] [diff] [review]
APZC align display port to tile when tiled layers are enabled

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

Minusing as per IRC discussions, I think this patch should use LayerPixel rather than LayoutDevicePixel, and use LayersPixelsPerCSSPixel() as the conversion factor instead of mDevPixelsPerCSSPixel. Some other comments below.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +861,5 @@
> +  CSSRect& aDisplayPort,
> +  const FrameMetrics& aFrameMetrics)
> +{
> +  // Convert the given rect to screen coordinates so we can work in screen
> +  // pixels.

Screen != LayoutDevice; please make sure the comments match the code. Also this particular comment doesn't add much value unless you say why it's preferable to work in "screen" pixels

@@ +898,5 @@
> +  // Any changes to this code must be considered and possibly reflected in
> +  // the code that it mirrors, with documentation if for whatever reason it
> +  // doesn't apply there.
> +  {
> +#   define EXTRA_FUDGE 0.04

Please #undef these defines at the end of the block so they don't pollute the rest of the file's namespace

@@ +1087,5 @@
>      scrollOffset.y = scrollableRect.y;
>    }
>  
>    CSSRect shiftedDisplayPort = displayPort + scrollOffset;
> +#ifdef FORCE_BASICTILEDTHEBESLAYER

If you're ifdef'ing this line you should ifdef the entire function too.
Attachment #796855 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 798038 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

This on its own does a pretty good job of making sure updates are tile-aligned, but to do a really good job it needs to be applied in conjunction with part two (I'm pretty pleased with this on its own though).
Attachment #796855 - Attachment is obsolete: true
Attachment #798038 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 4

5 years ago
Created attachment 798040 [details] [diff] [review]
Part 2 - Clip visible region of tiled thebes layers to display port

This bypasses some of the error introduced in Gecko by clipping the visible region to the display port. I'm not sure how good an idea this is, I've been looking at this too long to think rationally about it, but it does the job. Does this interfere with DLBI at all?

With this applied and taking into account the scroll fudging as done in part 1, we could probably get rid of dirtiestHackEverToWorkAroundGeckoRounding.
Attachment #798040 - Flags: review?(matt.woodrow)
Attachment #798040 - Flags: feedback?(bugmail.mozilla)
(Assignee)

Comment 5

5 years ago
I'm off for 2 months now, I'm hoping that kats or botond can pick this up.
Assignee: chrislord.net → nobody
Status: ASSIGNED → NEW
Comment on attachment 798040 [details] [diff] [review]
Part 2 - Clip visible region of tiled thebes layers to display port

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

I don't think I know this code enough to comment here.
Attachment #798040 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 798038 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +648,5 @@
>     *
>     * @param aFlushLayout flushes layout if true. Otherwise, no flush occurs.
>     * @see nsIDOMWindow::scrollX/Y
>     */
> +  void getScrollXY(in boolean aFlushLayout, out float aScrollX, out float aScrollY);

I don't think we can change these to floats without breaking existing code.

::: dom/ipc/TabChild.cpp
@@ +1604,5 @@
> +
> +#ifdef FORCE_BASICTILEDTHEBESLAYER
> +  CSSRect scrollableRect = aFrameMetrics.mScrollableRect;
> +  displayPort = ExpandDisplayPortToTileBoundaries(
> +    displayPort + aScrollOffset, aFrameMetrics.mZoom * ScreenToLayerScale(1));

This ScreenToLayerScale multiplier needs a comment.
Attachment #798038 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 798040 [details] [diff] [review]
Part 2 - Clip visible region of tiled thebes layers to display port

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

::: gfx/layers/client/ClientTiledThebesLayer.cpp
@@ +52,5 @@
> +  //     It rarely gets hit though, and shouldn't have terrible consequences
> +  //     even if it is wrong.
> +  for (ContainerLayer* parent = GetParent(); parent; parent = parent->GetParent()) {
> +    if (parent->UseIntermediateSurface()) {
> +      transform.PreMultiply(parent->GetEffectiveTransform());

I think this should be post-multiply, see http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp#882
Attachment #798040 - Flags: review?(matt.woodrow) → review+
Created attachment 802609 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging
Assignee: nobody → blassey.bugs
Attachment #798038 - Attachment is obsolete: true
Attachment #802609 - Flags: review?(bugmail.mozilla)
Comment on attachment 802609 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

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

I don't think this will have the same effect as Chris' original patch - the float out-params from getScrollXY was intentional, and needs to be done so that the code in MaybeAlignAndClampDisplayPort properly compensates for the rounding error. As discussed on IRC a better option would be to add another getScrollXY function that returns the unrounded floats and use that.
Attachment #802609 - Flags: review?(bugmail.mozilla) → review-
Created attachment 802631 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging
Attachment #802609 - Attachment is obsolete: true
Attachment #802631 - Flags: review?(bugmail.mozilla)
Comment on attachment 802631 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

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

r=me with comments addressed.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +656,5 @@
> +   *
> +   * @param aFlushLayout flushes layout if true. Otherwise, no flush occurs.
> +   * @see nsIDOMWindow::scrollX/Y
> +   */
> +  void getScrollXYFloat(in boolean aFlushLayout, out float aScrollX, out float aScrollY);

UUID in this file needs updating

::: dom/ipc/TabChild.cpp
@@ +1606,5 @@
> +    CSSRect scrollableRect = aFrameMetrics.mScrollableRect;
> +    displayPort = ExpandDisplayPortToTileBoundaries(
> +    // aFrameMetrics.mZoom is the zoom amount reported by the APZC,
> +    // scale by ScreenToLayerScale to get the gecko zoom amount
> +    displayPort + aScrollOffset, aFrameMetrics.mZoom * ScreenToLayerScale(1));

Indentation here is hard to read. These three lines should be indented a bit more.
Attachment #802631 - Flags: review?(bugmail.mozilla) → review+
It is unclear to me if this improves performance or not. Any good way to measure that?
Assignee: blassey.bugs → nobody
I think this patch is either creating or unveiling a clipping issue. Here is the behavior I see:
https://www.dropbox.com/s/dj6wlbc6jrffu5v/VIDEO0004.mp4

Kats, any ideas here?
(Assignee)

Comment 15

5 years ago
Thanks for taking this - in terms of perf measurement, we need something like tpan and tchk that runs with APZC enabled to see the perf difference.

Wrt that bug in the video, it kind of looks like the clipping of the display port to the page size isn't working quite correctly? I'd be interested to see if that bug occurs on a Keon, or any non-HD phone (if it doesn't, it'd indicate some units are wrong somewhere in the display port calculation code. Probably).
Whiteboard: [release28]
(Assignee)

Comment 16

5 years ago
Created attachment 8336852 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

I'm going to carry r=kats as this is really just a rebase, but I'd like Botond's review for the way I've rebased it (which affects Metro).
Attachment #802631 - Attachment is obsolete: true
Attachment #8336852 - Flags: review?(botond)
(Assignee)

Comment 17

5 years ago
Comment on attachment 798040 [details] [diff] [review]
Part 2 - Clip visible region of tiled thebes layers to display port

This is kind of expensive, and shouldn't be necessary. Rather than rebase this, I'd rather just commit the first patch and try and work towards minimising the rounding errors that cause this patch to be necessary.
Attachment #798040 - Attachment is obsolete: true
Comment on attachment 8336852 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +664,5 @@
> +   *
> +   * @param aFlushLayout flushes layout if true. Otherwise, no flush occurs.
> +   * @see nsIDOMWindow::scrollX/Y
> +   */
> +  void getScrollXYFloat(in boolean aFlushLayout, out float aScrollX, out float aScrollY);

Can we place this declaration next to getScrollXY, since they are closely related?

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +61,5 @@
> +}
> +
> +static void
> +MaybeAlignAndClampDisplayPort(mozilla::layers::FrameMetrics& aFrameMetrics,
> +                              const CSSPoint& aScrollOffset)

Can we call aScrollOffset aActualScrollOffset or aResultingScrollOffset?

@@ +79,5 @@
> +  }
> +
> +  // Finally, clamp the display port to the scrollable rect.
> +  CSSRect scrollableRect = aFrameMetrics.mScrollableRect;
> +  displayPort = scrollableRect.ClampRect(displayPort) - aScrollOffset;

We only add aScrollOffset to displayPort if the 'if' condition is true. Should we be subtracting it here unconditionally?
(Assignee)

Comment 19

5 years ago
Created attachment 8337733 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

Nice catches on all fronts, all sorted now (hopefully).
Attachment #8336852 - Attachment is obsolete: true
Attachment #8336852 - Flags: review?(botond)
Attachment #8337733 - Flags: review?(botond)
Comment on attachment 8337733 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

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

Looks good!
Attachment #8337733 - Flags: review?(botond) → review+
(Assignee)

Comment 22

5 years ago
Backed out due to Windows build failure: https://tbpl.mozilla.org/?tree=Try&rev=d2585e2a6852

Cause is obvious, but I'm confirming with try first anyway before repushing.
(Assignee)

Comment 25

5 years ago
Ugh, I pushed the wrong patch (the same, unfixed patch). Pushed the correct one again to inbound, sorry about that :/

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc85ab7ac74a
(Assignee)

Comment 26

5 years ago
Might as well assign this again before it gets marked as fixed...
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/dc85ab7ac74a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 8337733 [details] [diff] [review]
Part 1 - Align to tile boundaries, taking into account scroll fudging

+  // Expand the display port to the next tile boundaries, if tiled thebes layers
+  // are enabled.
+  if (Preferences::GetBool("layers.force-tiles")) {

Is this in performance-critical code (e.g. while actually panning/zooming)?  Grabbing this pref each time is a waste of work either way; we should be using a cached perf here (or just getting it once and saving the result statically; supporting major pref changes like this at runtime is pointless pain).
(Assignee)

Updated

5 years ago
Depends on: 950696
You need to log in before you can comment on or make changes to this bug.