1241917 - Display port for scrollable subframes can be too large

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Jamie Nicol [:jnicol]]

Subframes' displayport base rects are clipped to their scrollport (in ScrollFrameHelper::DecideScrollableLayer). This is fine when they have small scrollports. However, sometimes scrollports can be very large.

For example, https://wiki.openstreetmap.org/wiki/FR:%C3%89l%C3%A9ments_cartographiques on fennec. On a narrow phone screen the tables become scrollable subframes which scroll horizontally. Vertically, however, the tables are displayed in full, meaning their scrollport is many screens tall. This results in the table layers having very large display ports - much larger than that of the page's main layer.

Should a frame's base displayport size be restricted even further to the size of the parent frame's base displayport? Or something else?

Comment 1

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

Yeah, it should be restricted to the "root composition size". There used to be code that does this, added in bug 986413. The relevant hunks seem to be missing in m-c, not sure when it got taken out... I'll dig around a bit.

Comment 2

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

Oh, actually it's still there, it just got refactored. So the call to aFrameMetrics.CalculateBoundedCompositedSizeInCssPixels in CalculatePendingDisplayPort should be clamping the displayport to the root composition size, although perhaps that code path isn't always getting the correct root composition size?

Comment 3

[Jamie Nicol [:jnicol]]

In CalculatePendingDisplayPort we use the bounded composited size when calculating the display port size. Then we use the display port size along with the bounded composited size (again) to calculate the margins.

Instead, I think we should be using the non-bounded composited size when calculating the margins. This will give the negative margins we talked about on irc, and a sensible final display port size.

Sound correct? I'll whip up a patch if so.

Comment 4

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

That sounds reasonable to me. tnikkel, what do you think? As far as i can tell, the displayport base (to which the margins are applied on the layout side) is not getting bounded, so this should work.

Comment 5

[Jamie Nicol [:jnicol]]

The other option would be to bound the display port base to the root composition size? I don't really know this code, but intuitively that would make more sense to me if it's possible.

Comment 6

[Botond Ballo [:botond]]

(In reply to Jamie Nicol [:jnicol] from comment #5)
> The other option would be to bound the display port base to the root
> composition size? I don't really know this code, but intuitively that would
> make more sense to me if it's possible.

I agree. I think we could do this fairly easily by bounding the display port base just after reading it in order to combine it with the margins [1]. We can calculate the root composition at that size by calling nsLayoutUtils::CalculateBasicFrameMetrics [2] and getting the root composition size from the resulting FrameMetrics.

[1] https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/layout/base/nsLayoutUtils.cpp#891
[2] https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/layout/base/nsLayoutUtils.h#2654

Comment 7

[Timothy Nikkel (:tnikkel)]

The displayport base (for subframes) is the intersection of the dirty rect and the scroll port. If the document doesn't have any other display ports then the dirty rect would be less than or equal to the size of the root scroll frame (basically the root composition size). But we do have display ports, so the dirty rect takes into account those as well (ie it contains what is rendered of the subframe in the displayport of the parent scroll frame, not what is visible after the parent scroll frames clips). I would consider this a bug as in the design the displayport base was intended to be what is visible, not what is rendered.

So fixing that bug would be ideal, instead of hacking around it in other places.

Comment 8

[Timothy Nikkel (:tnikkel)]

As for effectively doing that perhaps this is a good approach:
1) get the composition bounds of the root scroll frame of the root content doc (or root doc if there isn't a root content doc)
2) transform those bounds to our local scroll frame using something like nsLayoutUtil::TransformFrameRectToAncestor (we'd need from ancestor descendant instead though)
3) only do this if there is a displayport set to avoid the overhead when its not needed

Comment 9

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

I did some digging into gfx memory usage, and on amazon.com we use 650MB or so in GL textures on a Nexus 6P (which has a 2k screen). This is pretty unreasonable. Layer dump below. Botond thinks it is due to this bug. Note the gigantic subframe region bounds.

01-26 17:29:36.378  8345  8407 I Gecko   : LayerManager (0xcab462e0)
01-26 17:29:36.378  8345  8407 I Gecko   :   ContainerLayerComposite (0xc743f400) [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [visible=< (x=0, y=0, w=1440, h=2140); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=2140.000000)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=2] [z=3.53] }]
01-26 17:29:36.378  8345  8407 I Gecko   :     PaintedLayerComposite (0xbf0f3c00) [not visible] { hitregion=< (x=0, y=0, w=1440, h=2140); >} [opaqueContent]
01-26 17:29:36.378  8345  8407 I Gecko   :     ContainerLayerComposite (0xc7440000) [shadow-clip=(x=0, y=0, w=1440, h=2140)] [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [clip=(x=0, y=0, w=1440, h=2140)] [visible=< (x=0, y=0, w=1440, h=2140); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=2140.000000)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=1885.866699)] [color=rgba(255, 255, 255, 1.000000)] [scrollId=4] [scrollParent=2] [rcd] [z=3.53] }] [force-dtc]
01-26 17:29:36.378  8345  8407 I Gecko   :       PaintedLayerComposite (0xbf0ef800) [not visible] { hitregion=< (x=0, y=0, w=1440, h=2140); >} [opaqueContent]
01-26 17:29:36.378  8345  8407 I Gecko   :       PaintedLayerComposite (0xc3284800) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible]
01-26 17:29:36.378  8345  8407 I Gecko   :       ColorLayerComposite (0xc3284c00) [bounds=(x=0, y=0, w=1440, h=10520)] [visible=< (x=0, y=0, w=1440, h=10520); >] { hitregion=< (x=0, y=0, w=1440, h=10520); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=10520); >} [opaqueContent] [color=rgba(255, 255, 255, 1.000000)] [bounds=(x=0, y=0, w=1440, h=10520)]
01-26 17:29:36.378  8345  8407 I Gecko   :       PaintedLayerComposite (0xc9406000) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [bounds=(x=-5, y=0, w=1450, h=10520)] [visible=< (x=0, y=0, w=1440, h=4402); (x=0, y=4402, w=21, h=671); (x=699, y=4402, w=44, h=671); (x=1420, y=4402, w=20, h=671); (x=0, y=5073, w=1440, h=3); (x=0, y=5076, w=21, h=670); (x=699, y=5076, w=44, h=670); (x=1420, y=5076, w=20, h=670); (x=0, y=5746, w=1440, h=4774); >] { hitregion=< (x=0, y=0, w=1440, h=10520); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=10520); >} [opaqueContent] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=0, y=0, w=1440, h=4402); (x=0, y=4402, w=21, h=671); (x=699, y=4402, w=44
01-26 17:29:36.378  8345  8407 I Gecko   :         TiledContentHost (0xc4c6eb60)
01-26 17:29:36.378  8345  8407 I Gecko   :       ContainerLayerComposite (0xc7f28c00) [shadow-clip=(x=0, y=480, w=1440, h=441)] [shadow-transform=[ 1 0; 0 1; 0 480; ]] [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [clip=(x=0, y=480, w=1440, h=441)] [transform=[ 1 0; 0 1; 0 480; ]] [visible=< (x=0, y=0, w=1440, h=441); >] [backfaceHidden] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }]
01-26 17:29:36.378  8345  8407 I Gecko   :         PaintedLayerComposite (0xbf0ec800) [not visible] { hitregion=< (x=0, y=0, w=1440, h=442); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=442); >} [opaqueContent] [backfaceHidden]
01-26 17:29:36.378  8345  8407 I Gecko   :         ContainerLayerComposite (0xc7fbbc00) [shadow-clip=(x=0, y=0, w=1440, h=441)] [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [clip=(x=0, y=0, w=1440, h=441)] [visible=< (x=0, y=0, w=1440, h=442); >] [backfaceHidden]
01-26 17:29:36.378  8345  8407 I Gecko   :           ContainerLayerComposite (0xc85e8000) [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [visible=< (x=0, y=0, w=1440, h=442); >] [backfaceHidden]
01-26 17:29:36.378  8345  8407 I Gecko   :             PaintedLayerComposite (0xbf0ec400) [not visible] { hitregion=< (x=0, y=0, w=8640, h=442); > dispatchtocontentregion=< (x=0, y=0, w=8640, h=442); >} [opaqueContent] [backfaceHidden]
01-26 17:29:36.378  8345  8407 I Gecko   :             PaintedLayerComposite (0xc85e9800) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible]
01-26 17:29:36.378  8345  8407 I Gecko   :             ImageLayerComposite (0xc9401800) [shadow-clip=(x=0, y=0, w=1440, h=441)] [shadow-transform=[ 1.17647 0; 0 1.17647; -10.5882 0; ]] [shadow-visible=< (x=8, y=0, w=1225, h=375); >] [clip=(x=0, y=0, w=1440, h=441)] [transform=[ 1.17647 0; 0 1.17647; -10.5882 0; ]] [bounds=(x=-11, y=0, w=1462, h=442)] [visible=< (x=8, y=0, w=1225, h=375); >] { hitregion=< (x=8, y=0, w=1225, h=376); > dispatchtocontentregion=< (x=8, y=0, w=1225, h=376); >} [opaqueContent]
01-26 17:29:36.378  8345  8407 I Gecko   :               ImageHost (0xc4ca6e70)
01-26 17:29:36.378  8345  8407 I Gecko   :                 MemoryTextureHost (0xceca1500) [size=(w=1242, h=375)] [format=SurfaceFormat::B8G8R8A8] [flags=NoFlags] [picture-rect=(x=0, y=0, w=1242, h=375)]
01-26 17:29:36.378  8345  8407 I Gecko   :       PaintedLayerComposite (0xc3284000) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [bounds=(x=7, y=4402, w=1427, h=1344)] [visible=< (x=7, y=4402, w=706, h=671); (x=729, y=4402, w=705, h=671); (x=7, y=5076, w=706, h=670); (x=729, y=5076, w=705, h=670); >] { hitregion=< (x=7, y=4402, w=706, h=671); (x=728, y=4402, w=707, h=671); (x=7, y=5075, w=706, h=672); (x=728, y=5075, w=707, h=672); > dispatchtocontentregion=< (x=7, y=4402, w=706, h=671); (x=728, y=4402, w=707, h=671); (x=7, y=5075, w=706, h=672); (x=728, y=5075, w=707, h=672); >} [backfaceHidden] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=7, y=4402, w=706, h=671); (x=729, y=4402, w=705, h=671); (x=7, y=5076, w=706, h=670
01-26 17:29:36.378  8345  8407 I Gecko   :         TiledContentHost (0xc3390b70)
01-26 17:29:36.378  8345  8407 I Gecko   :       PaintedLayerComposite (0xc323a400) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [bounds=(x=41, y=4909, w=1305, h=803)] [visible=< (x=42, y=4910, w=1108, h=65); (x=42, y=4975, w=1112, h=64); (x=42, y=5584, w=1039, h=64); (x=42, y=5648, w=1304, h=64); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=42, y=4910, w=1108, h=65); (x=42, y=4975, w=1112, h=64); (x=42, y=5584, w=1039, h=64); (x=42, y=5648, w=1304, h=64); >]
01-26 17:29:36.379  8345  8407 I Gecko   :         TiledContentHost (0xc3390c60)

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Patch v1

I think I've done what you suggested in comment 8. I've verified that it does as intended - memory usage is down and the apz minimap looks sensible.

Comment 11

[Jamie Nicol [:jnicol]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71f86c7b22cd

Ludovic, could you check whether you can crash with that build? (https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-71f86c7b22cd3e94b4139791768e5f21ff5506db/try-android-api-11/fennec-47.0a1.en-US.android-arm.apk)

Comment 12

[Ludovic Hirlimann [:Usul]]

(In reply to Jamie Nicol [:jnicol] from comment #11)
> Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71f86c7b22cd
> 
> Ludovic, could you check whether you can crash with that build?
> (https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 71f86c7b22cd3e94b4139791768e5f21ff5506db/try-android-api-11/fennec-47.0a1.en-
> US.android-arm.apk)

not crashing.

Comment 13

[Timothy Nikkel (:tnikkel)]

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

>Previously they were restricted only to the scrollport of the subframe,
>which could lead to very large displayports in cases where the
>scrollport was larger than the root composition bounds.

This explanation is misleading because it makes it seem like the only restriction on the displayport base is the scrollport. I'd suggest something like this:

"Previously displayport bases were computed as the intersection of the scrollport with the dirtyrect. However the dirtyrect covers what is rendered, and with displayports what we render can be much larger than what is visible. With displayport bases intended to represent what was visible, this was a problem. By restricting them to the root composition size this makes them more closely match what is visible. To do this more properly we'd want to intersect the dirtyrect with the scroll clip of every ancestor scroll frame, not just the root composition bounds."

>+        if (rootPresContext) {
>+          const nsIPresShell* const rootPresShell = rootPresContext->PresShell();
>+          nsIFrame* const rootScrollFrame = rootPresShell->GetRootScrollFrame();
>+          if (rootScrollFrame) {

If there is no root scroll frame (XUL documents) then you should use the root frame.

Comment 14

[Jamie Nicol [:jnicol]]

Attachment: Patch v2

Use the root frame if there is no root scroll frame, and updated commit message.

Comment 15

[Jamie Nicol [:jnicol]]

Try run on patch v2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a23f8648060&selectedJob=16347264

few unrelated intermittents but it looks good.

Requesting checkin of patch v2 please.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/046c7007ac8e

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/046c7007ac8e

Comment 18

[Jamie Nicol [:jnicol]]

Kats, do we need to uplift this to 46?

(Note to self: if we do, then we also need to remember to uplift the performance regression fix in 1246443, once it's landed)

Comment 19

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

I don't see a strong reason to uplift to 46 at the moment. This patch helps a lot for Fennec but so far we haven't run into specific cases that would be helped by it on desktop. So if it rides on 47 that should be fine, because APZ is only turned on for desktop in 46 and will only get enabled for Fennec in 47 (or later). Unless something comes up for desktop that this fixes, I'm inclined to just leave it out of 46 and let it ride.