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)

48 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jnicol, Unassigned)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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?
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.
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
I'll give it a try, thanks!
Blocks: 1277719
OS: Unspecified → Android
Priority: -- → P2
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → 48 Branch
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+
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
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)
(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)
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
https://hg.mozilla.org/mozilla-central/rev/9ae101b3ec75
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
I'm ok uplifting this to beta. I don't think it's too risky.
+1 from me as well
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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: