Chris Lord [:cwiiis] Assignee
	Description • 11 years ago

For efficient tiled updates, display-port boundaries should align to tile boundaries. Android already does this.

	Chris Lord [:cwiiis] Assignee
	Comment 1 • 11 years ago

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...

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 11 years ago

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.

	Chris Lord [:cwiiis] Assignee
	Comment 3 • 11 years ago

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).

	Chris Lord [:cwiiis] Assignee
	Comment 4 • 11 years ago

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.

	Chris Lord [:cwiiis] Assignee
	Comment 5 • 11 years ago

I'm off for 2 months now, I'm hoping that kats or botond can pick this up.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 11 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 8 • 11 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 11 years ago

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.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 13 • 11 years ago

It is unclear to me if this improves performance or not. Any good way to measure that?

	Brad Lassey [:blassey] (use needinfo?)
	Comment 14 • 11 years ago

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?

	Chris Lord [:cwiiis] Assignee
	Comment 15 • 11 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).

	Chris Lord [:cwiiis] Assignee
	Comment 16 • 11 years ago

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).

	Chris Lord [:cwiiis] Assignee
	Comment 17 • 11 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.

	Botond Ballo [:botond]
	Comment 18 • 11 years ago

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?

	Chris Lord [:cwiiis] Assignee
	Comment 19 • 11 years ago

Nice catches on all fronts, all sorted now (hopefully).

	Botond Ballo [:botond]
	Comment 20 • 11 years ago

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!

	Chris Lord [:cwiiis] Assignee
	Comment 21 • 11 years ago

Pushed to b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/e3b5dca30ade

	Chris Lord [:cwiiis] Assignee
	Comment 22 • 11 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.

	Chris Lord [:cwiiis] Assignee
	Comment 23 • 11 years ago

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=d2585e2a6852

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/de63b66f3d12

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 11 years ago

Backed out for Windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdd349ef5b0

https://tbpl.mozilla.org/php/getParsedLog.php?id=31105815&tree=Mozilla-Inbound

	Chris Lord [:cwiiis] Assignee
	Comment 25 • 11 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

	Chris Lord [:cwiiis] Assignee
	Comment 26 • 11 years ago

Might as well assign this again before it gets marked as fixed...

	Carsten Book [:Tomcat]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/dc85ab7ac74a

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 28 • 11 years ago

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).