Closed Bug 1625816 Opened 4 years ago Closed 4 years ago

Position of background rectangles is affected by the current display port

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: gw, Assigned: gw)

Details

Attachments

(3 files)

Attached file long.html

Primitives supplied to WR (sometimes?) have their position and/or local clip rect affected by what I assume is the current display port.

For example, if you load the supplied html file, the following rectangles are supplied in the display list:

rect=Rect(1898.0×7040.0 at (0.0,0.0)) local_clip_rect=Rect(1898.0×7040.0 at (0.0,0.0))
rect=Rect(150.0×7028.0 at (12.0,12.0)) local_clip_rect=Rect(150.0×7028.0 at (12.0,12.0))

You can scroll for some time, maybe a page or so, and then Gecko sends a new display list, with the following:

rect=Rect(1898.0×7040.0 at (0.0,128.0)) local_clip_rect=Rect(1898.0×7040.0 at (0.0,128.0))
rect=Rect(150.0×7040.0 at (12.0,128.0)) local_clip_rect=Rect(150.0×7040.0 at (12.0,128.0))

And shortly after:

rect=Rect(1898.0×7040.0 at (0.0,256.0)) local_clip_rect=Rect(1898.0×7040.0 at (0.0,256.0))
rect=Rect(150.0×7040.0 at (12.0,256.0)) local_clip_rect=Rect(150.0×7040.0 at (12.0,256.0))

The size of the rectangles doesn't change, but the position of both the primitive rectangle and local clip rect get an offset.

WR has quite a significant amount of code complexity to try and avoid invalidating tiles when this occurs, that also has quite a significant performance penalty.

Does anyone know why this occurs, and if it's possible to change the DL provided to WR to not do this? (ideally there should be no information related to the display port in the WR display lists, and we let WR handle this internally).

Flags: needinfo?(mikokm)
Flags: needinfo?(kats)
Flags: needinfo?(jmuizelaar)

It looks like what's happening here is that a different section of the rect is made "visible" as you scroll, due to the displayport. So for instance with the first rect in your dumps, it's actual size in document coordinates would be something like (1898.0 x very-big-number) at (0.0, 0.0) and the displayport is 7040 pixels high, so the part that's initially in the displayport is from (0,0) to (1898, 7040). Then as you scroll down, the displayport shifts down by 128 pixels, and the part that's now in the displayport is from (0,128) to (1898, 7040+128). So the element hasn't actually moved, but a different part of it is "visible" (i.e. in the displayport). That also explains why the second rect is initially 7028 pixels tall (because it starts at y=12 and goes to y=7040, the bottom of the displayport) and then as the displayport moves down it now intersects the element from y=128 to y=7040+128.

So I'm not entirely sure what you're expecting to happen here, do you want these rects entirely unclipped? Then they might be potentially unbounded in size.

Flags: needinfo?(kats)

Also note that with these giant elements, the painted content at y=0 to y=7040 might actually be different from the painted content y=128 to y=7168. i.e. imagine a background image that's super-tall but is a gradient or image or something. So maybe invalidating is actually the right thing to do in this case? For a flat background color we can probably reuse since the painted content won't be visibly different.

I don't think they'd be unbounded in size, I think they would ideally always be the size of the true page viewport.

It seems like the behavior here is different for rectangle and gradient primitives.

I attached an updated repro case, that has the background viewport rect, and a long rectangle + a long gradient, with the following results:

At the start:

rect: r=Rect(1898.0×7040.0 at (0.0,0.0)) c=Rect(1898.0×7040.0 at (0.0,0.0))
rect: r=Rect(150.0×7028.0 at (15.0,12.0)) c=Rect(150.0×7028.0 at (15.0,12.0))
gradient: r=Rect(150.0×15000.0 at (300.0,12.0)) c=Rect(150.0×7028.0 at (300.0,12.0))

After scrolling down a few pages:

rect: r=Rect(1898.0×7040.0 at (0.0,2560.0)) c=Rect(1898.0×7040.0 at (0.0,2560.0))
rect: r=Rect(150.0×7040.0 at (15.0,2560.0)) c=Rect(150.0×7040.0 at (15.0,2560.0))
gradient: r=Rect(150.0×15000.0 at (300.0,12.0)) c=Rect(150.0×7040.0 at (300.0,2560.0))

Notice how the primitive size and offset of the gradient primitive doesn't change at all based on the display port, but the rectangle does? This seems correct to me, since otherwise that would affect the visual appearance of the gradient? However, the local clip rect of the gradient does still seem to include the display port changes, which isn't ideal either.

Attached file long.html

Ah, interesting. In that case the likely explanation is that somewhere in the WR DL building code on the gecko side, we are not using the display item's true size. For example here we pass in r for both the item's rect and the clip rect, which would explain what you're seeing. By contrast, the place we create linear gradients there is a different rect for the item (gradientBounds) and the clip (clipBounds). If you can track down which PushRect calls are generating the "bad" rects we can probably fix them on the gecko side to use a better rect.

Actually pretty much all the PushRect calls use the same value for rect and clip. :miko, maybe you know which rect we should be using instead? I suspect nsIFrame::GetRect() but I'm not sure if that's in the same coordinate space or not.

Flags: needinfo?(jmuizelaar)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)

Actually pretty much all the PushRect calls use the same value for rect and clip. :miko, maybe you know which rect we should be using instead? I suspect nsIFrame::GetRect() but I'm not sure if that's in the same coordinate space or not.

I think that if the intention is to clip primitives to display item bounds, nsDisplayItem::GetClip() or nsDisplayItem::GetClippedBounds() might work.

Flags: needinfo?(mikokm)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)

If you can track down which PushRect calls are generating the "bad" rects we can probably fix them on the gecko side to use a better rect.

:gw since you have a setup already to reproduce this it might be easiest for you to experiment with this - track down the PushRect call on the gecko side that is creating the rect in question and try providing one of the other rects instead. I'm not sure if we want the display item bounds like Miko suggested, or the frame bounds. I was thinking the latter but I could be wrong.

Assignee: nobody → gwatson

OK, I have a slightly better understanding what's going on here now.

Some time ago, optimizations were made to the display list format such that rectangles (and some other primitives?) only supply a single rectangle for their geometry. This rectangle is the intersection of the primitive's bounds and clip rect [1].

In this case, the correct (full height) rectangle is being passed as the bounds and clip rect into PushRect [2]. However, there is code inside PushRect that intersects the supplied primitive clip rect with mClipChainLeaf [3].

It appears that mClipChainLeaf contains a clip rect that is defined (or affected) by the current display port, which is what's causing the rectangle that eventually gets supplied to wr_dp_push_rect to show unwanted changes as the display port moves.

There are two possible fixes I can see:

(1) Change the WR internals so that rectangles are represented with both a geometry bounds and clip rect. This would allow us to pass the true geometry bounds of the rectangle that we get in PushRect through WR, which would fix this specific problem.

(2) Change the way the clip leaf / clip chain handling works in Gecko so that display port clips are excluded from what is passed to WR clip chains. This would mean that the bounds of the rect as supplied to WR would no longer be affected by the display port.

Option (2) is preferable, I think. The way that clip rect sizes change based on the display port is effectively the same problem as this, in that it causes unnecessary invalidations of WR content tiles (and extra interning overhead etc, as the clip values change). If we're able to implement (2) in Gecko, that would solve both of these problems in one go. However, I'm not sure how simple / feasible (2) is to implement in Gecko?

[1] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/gfx/webrender_bindings/src/bindings.rs#2579
[2] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/gfx/webrender_bindings/WebRenderAPI.cpp#1131
[3] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/gfx/webrender_bindings/WebRenderAPI.cpp#1135

Flags: needinfo?(kats)
Flags: needinfo?(mstange)

Who would apply the display port clip, then? We do want the contents of a scroll frame to be constrained to the display port, otherwise we can have random elements sticking out from the display port. We'd rather have checkerboarding (i.e. blank content) outside the display port than to show an inconsistent set of items in those areas.
Maybe we can set the display port as some kind of attribute on the scroll frame, and have WR apply the clip?

Flags: needinfo?(mstange)
Flags: needinfo?(gwatson)

Having a clip on the scroll frame does sound like it might be a good way to do the display port clip, intriguing.

However, since that would be a bigger change and require a bit more thought, I think I'll go with option (1) for now, to pass the full rect through.

That will solve this specific problem, which will unblock some optimization work I'm doing, and we can revisit later when I address the clip-related invalidation issues mentioned above.

It will probably regress performance of dl_mutate but it shouldn't have any noticeable effect on real world pages.

Flags: needinfo?(gwatson)

This can cause extra invalidations in picture caching.

(In reply to Glenn Watson [:gw] from comment #9)

(2) Change the way the clip leaf / clip chain handling works in Gecko so that display port clips are excluded from what is passed to WR clip chains. This would mean that the bounds of the rect as supplied to WR would no longer be affected by the display port.

Interestingly, if we did this, it would also probably solve bug 1424714 where I basically did this but just for sticky elements, and rather hackily at that.

I agree that setting the displayport clip as a separate property on the scroll frame might be a good way to go, but will require a bunch of work.

Flags: needinfo?(kats)
Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c71a7f69ab
Fix rectangle display item bounds being affected by display port clip. r=nical
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: