1223479 - Displayport is too large on fennec

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jamie Nicol [:jnicol]]

There are a few issues with the displayport size calculation on fennec:

1) Java code calculates a displayport aligned to tile boundaries. Then, because of a mismatch between the viewport in DisplayPortCalculator.java and the one in browser.js and GetDisplayPortFromMarginsData(), the margins set in C++ will no longer align to the tile boundaries. GetDisplayPortFromMarginsData will then align to tile boundaries again, possibly adding an extra tile.

2) GetDisplayPortFromMarginsData() inflates the rect by 1 in each direction before expanding outwards to tile boundaries. So even if problem 1) did not exist, this would be guaranteed to add 2 extra rows and 2 extra columns of tiles, since java has already aligned the displayport.

3) This inflate by 1 causes us to use an extra row or column of tiles in circumstances where the displayport offset and size are aligned with the tile sizes. I believe this is worse than without inflating, where we use 1 less row or column when aligned. The displayport size might not be a multiple of the tile size, making the solution to this more complex, anyway.

4) The java code depends on a hard-coded tile size when aligning the displayport. But the tile size is set either in a pref or automatically.

The solution to the above problems is to a) stop aligning the display port to the tile size in java code, and b) remove the inflate by 1 from GetDisplayPortFromMarginsData()

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Patch v1

This should do it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28c32e0fcc9e

Comment 2

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

Comment on attachment 8685544 [details] [diff] [review]
Patch v1

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

LGTM, thanks!

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +174,5 @@
>       * does not have any partial tiles, except when it is clipped by the page bounds. This assumes
>       * the tiles are TILE_SIZE by TILE_SIZE and start at the origin, such that there will always be
>       * a tile at (0,0)-(TILE_SIZE,TILE_SIZE)).
>       */
> +    private static DisplayPortMetrics getPageClampedDisplayPortMetrics(RectF margins, float zoom, ImmutableViewportMetrics metrics) {

Remove the comment above this function

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Patch v2

Fixed the comment. No changes to code.

Comment 4

[Jamie Nicol [:jnicol]]

Requesting checkin on patch v2 please. Try run in comment 1.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b812b3dc8aa

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b812b3dc8aa

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

We should probably uplift this to 43 and 44. Jamie, if you agree, please do the appropriate requests.

Comment 8

[Jamie Nicol [:jnicol]]

Attachment: Patch for 43

Patch backported to 43

Comment 9

[Jamie Nicol [:jnicol]]

Comment on attachment 8685917 [details] [diff] [review]
Patch v2

I agree we should definitely uplift this to aurora. The patch for nightly applies fine and works.

Approval Request Comment
[Feature/regressing bug #]: Problem has been there for a long time but bug 1182665 made it very noticeable.
[User impact if declined]: Higher memory usage leading to more frequent crashes
[Describe test coverage new/current, TreeHerder]: Patch has been on nightly for weeks without problem. Manually compiled and tested on 44.
[Risks and why]: Low: small change which has been well tested.
[String/UUID change made/needed]: None

Comment 10

[Jamie Nicol [:jnicol]]

Comment on attachment 8693637 [details] [diff] [review]
Patch for 43

We probably want to uplift to beta too. The patch increasing the tile size (bug 1182665) isn't on beta. But this is still a bug regardless, even if it isn't quite as noticeable. And since this is low risk I'd recommend uplift.

Approval Request Comment
[Feature/regressing bug #]: Long-time problem
[User impact if declined]: Slightly higher memory usage, possibly leading to more crashes.
[Describe test coverage new/current, TreeHerder]: Patch has been on nightly for weeks without problem.
[Risks and why]: Low: small change which has been well tested.
[String/UUID change made/needed]: None

Comment 11

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

FWIW I would advise against uplifting to beta because historically code that the touches the displayport stuff often has unforeseen consequences, even if it seems pretty benign initially. On top of that the tile size patch isn't in beta (and therefore beta should be no worse than many releases we've already shipped). Although the patch has been baking on m-c for a while I feel we're pretty late in the cycle to uplift this and catch problems, specially since there is an unclear benefit and definitely some risk.

Comment 12

[Jamie Nicol [:jnicol]]

Comment on attachment 8693637 [details] [diff] [review]
Patch for 43

If kats feels there's too big a risk on display port related patches then I'm happy for this to not go on beta.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685917 [details] [diff] [review]
Patch v2

This has been on Nightly for a month, and seems to improve memory usage and stability, let's uplift to Aurora44.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e62acda19b7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/4e62acda19b7