Closed
Bug 1223479
Opened 9 years ago
Closed 9 years ago
Displayport is too large on fennec
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: jnicol, Assigned: jnicol)
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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()
Assignee | ||
Comment 1•9 years ago
|
||
This should do it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28c32e0fcc9e
Attachment #8685544 -
Flags: review?(bugmail.mozilla)
Comment 2•9 years ago
|
||
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
Attachment #8685544 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Fixed the comment. No changes to code.
Assignee | ||
Updated•9 years ago
|
Attachment #8685544 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Requesting checkin on patch v2 please. Try run in comment 1.
Keywords: checkin-needed
Keywords: checkin-needed
Comment 6•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
We should probably uplift this to 43 and 44. Jamie, if you agree, please do the appropriate requests.
Flags: needinfo?(jnicol)
Assignee | ||
Comment 8•9 years ago
|
||
Patch backported to 43
Assignee | ||
Comment 9•9 years ago
|
||
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
Flags: needinfo?(jnicol)
Attachment #8685917 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•9 years ago
|
||
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
Attachment #8693637 -
Flags: approval-mozilla-beta?
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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.
Attachment #8693637 -
Flags: approval-mozilla-beta?
status-firefox44:
--- → affected
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.
Attachment #8685917 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•9 years ago
|
||
bugherder uplift |
Comment 15•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•