Closed
Bug 1306371
Opened 8 years ago
Closed 8 years ago
Displayport on android becomes way too large at high zoom
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: jnicol, Unassigned)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
kats
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
I can OOM crash on pretty much any page on my nexus 6 just by zooming in... The APZ danger zone is specified in layer pixels. It's default values are x=50, y=100. If I zoom in to 35x then the danger zone is 1750x3500 in device pixels. On my nexus 6 (1440x2560 display) this is a display port of 4940x9560. (It would have been 2160x3840 if we didn't inflate by the danger zone, as the multiplier is 1.5 on android) We will OOM on a lot of devices if we try to render 4940x9560 of even a single layer. Could the danger zone be specified in a unit which takes zoom in to account?
Reporter | ||
Comment 1•8 years ago
|
||
Probably worth mentioning that before bug 1277719 landed we couldn't zoom in far enough for this to be a real problem. If we were to phrase the bug as "zooming in causes crash" then I suppose this is a regression from then. I don't know whether it was a problem back in the Java APZ days.
Comment 2•8 years ago
|
||
Layer pixels should be inclusive of zoom. I think the problem is that we're adding it into a CSS pixel value at [1] which is not good. Instead we should convert it to CSS pixels like we do at [2] before adding it to the existing CSS pixel values. Do you want to try that, or should I make a build for you to try? [1] http://searchfox.org/mozilla-central/rev/572e74ee991bbfd812766b4524237eb77577a4b1/gfx/layers/apz/src/AsyncPanZoomController.cpp#2712 [2] http://searchfox.org/mozilla-central/rev/572e74ee991bbfd812766b4524237eb77577a4b1/gfx/layers/client/TiledContentClient.cpp#261
Reporter | ||
Comment 3•8 years ago
|
||
I'll give it a try, thanks!
Updated•8 years ago
|
Blocks: 1277719
status-firefox49:
--- → wontfix
status-firefox50:
--- → fix-optional
status-firefox51:
--- → fix-optional
status-firefox52:
--- → affected
OS: Unspecified → Android
Priority: -- → P2
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → 48 Branch
Comment hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8797712 [details] Bug 1306371 - Use correct units when calculating displayport danger zone size. https://reviewboard.mozilla.org/r/83362/#review82256 This patch looks fine to me with the review comment below addressed. As per our IRC discussion, I tried reproducing the checkerboarding issue you described with this applied and wasn't able to. However it's probably a good idea to hold off landing this until we figure out what's going on there, since you seem to be able to reproduce it pretty easily. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2724 (Diff revision 1) > + * inflated by the danger zone. If this is not the case then the > + * "AboutToCheckerboard" function in TiledContentClient.cpp will return true > + * even in the stable state. > + */ > +static void > +ExpandDisplayPortToDangerZone(CSSSize& aDisplayPortSize, Please make the first argument a const-ref, and have the function return the resulting CSSSize. I'd like to avoid in-out parameters when it's relatively easy to do so since it's not obvious at the call site that the parameter will be modified in-place.
Attachment #8797712 -
Flags: review?(bugmail) → review+
Comment 6•8 years ago
|
||
Actually I took another look at the log you posted and I see some issues. After you're done zooming, the APZC sends a repaint request at 17:17:55.762 with the scroll position s=(61.4317,6333.87) and zoom z=4.03. However the NotifyLayersUpdated call that comes back from the main thread has the same old scroll position s=(259.55,6578.72), even though the resolution has been updated per the repaint request. This indicates that the scroll position change requested by the APZ code is not get applied to gecko, which would explain why the displayport is off from where it's expected to be. In the function at [1] there's only three possible ways in which the scroll position doesn't get applied - either the frame is null (highly unlikely), the !aMetrics.GetScrollOffsetUpdated() condition is true (which it isn't, because the FrameMetrics that logged has u=(3 ...)), or the code detects that the main thread already has a scroll in progress. I'm not sure why exactly that would happen but it seems like the most likely candidate of the lot. Can you add a printf inside the IsScrollInProgress function at [2] to prints each of the three clauses individually, so we can dig into what that function is returning and why? [1] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/gfx/layers/apz/util/APZCCallbackHelper.cpp#70 [2] http://searchfox.org/mozilla-central/rev/8071f94c4d4b66833ad08db39565669faad94dfd/gfx/layers/apz/util/APZCCallbackHelper.cpp#901
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d2a37c0965f3812998193e2e003e5d0c0328d0&selectedJob=28894578 With this patch we fail gfx/layers/apz/test/mochitest/test_bug1277814.html. I had a look, and aFrameMetrics.LayersPixelsPerCSSPixel is (0, 0). Which makes our danger zone (inf, inf). Which makes our displayport (-inf, -inf, inf, inf). Which causes the test to hang. Kats, is (0, 0) a valid value for LayersPixelsPerCSSPixel? If so, how should I scale the danger zone in this case? If not, how should we prevent it?
Flags: needinfo?(bugmail)
Comment 9•8 years ago
|
||
(In reply to Jamie Nicol [:jnicol] from comment #8) > is (0, 0) a valid value for LayersPixelsPerCSSPixel? In light of bug 1277814, yes it is. > If so, how should I scale the danger zone in this case? If LayersPixelsPerCSSPixel is 0, we're not actually displaying anything because there's a transform:scale(0) on the content, so it should be fine to have a danger zone of 0.
Flags: needinfo?(bugmail)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 11•8 years ago
|
||
Thanks Botond! Latest review push contains that fix. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ae6ed82e820060b3483c7295baddab926fe22c0&selectedJob=28916865
Keywords: checkin-needed
Comment 12•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9ae101b3ec75 Use correct units when calculating displayport danger zone size. r=kats
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ae101b3ec75
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8797712 [details] Bug 1306371 - Use correct units when calculating displayport danger zone size. I think it's worth considering uplifting this to beta, as crashing just by zooming in is very bad. But I'd like to know if Kats, Botond, or Markus have an opinion on how risky it is. Approval Request Comment [Feature/regressing bug #]: Bug 1277719 [User impact if declined]: Very easy to crash just by zooming in to any page on android [Describe test coverage new/current, TreeHerder]: try run in comment 11. on nightly [Risks and why]: Medium. Seems to make bug 1308535 more common. However, that will just display the wrong thing until the user scrolls again, which is preferable to crashing. I would imagine that is the case for any problems that could be unearthed by this. [String/UUID change made/needed]: N/A
Attachment #8797712 -
Flags: approval-mozilla-beta?
Attachment #8797712 -
Flags: approval-mozilla-aurora?
Comment 15•8 years ago
|
||
I'm ok uplifting this to beta. I don't think it's too risky.
Comment 16•8 years ago
|
||
+1 from me as well
Comment 17•8 years ago
|
||
I don't have an opinion on it.
Comment on attachment 8797712 [details] Bug 1306371 - Use correct units when calculating displayport danger zone size. Uplift to Aurora51 to stabilize it for a few days, will take it in Fennec 50.0b8.
Attachment #8797712 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b3bccfe462b
Comment on attachment 8797712 [details] Bug 1306371 - Use correct units when calculating displayport danger zone size. Stabilized on Nightly and Aurora for ~2 days, Beta50+
Attachment #8797712 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0db6d7668395
You need to log in
before you can comment on or make changes to this bug.
Description
•