1491555 - Some broken bullet points

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Darkspirit]

Attachment: 2018-09-15_16-59-54.mp4

Screencast: left=last good, right=first bad
* Depending on the window size all bullets can be broken at first. (with both last good and first bad)
* bad = The last bullet point of slide 6 shows up with a delay if I snap Nightly to a half of my desktop.

mozregression --good 2018-09-08 --bad 2018-09-15 --pref gfx.webrender.all:true -a https://pcwalton.github.io/slides/201808-webrender/
> 11:50.27 INFO: Last good revision: cc99c9e9db47e1ac6942d5f78262c1f9aabdb410
> 11:50.27 INFO: First bad revision: 224045731c6f2cf9f8905755937a542818f3b316
> 11:50.27 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cc99c9e9db47e1ac6942d5f78262c1f9aabdb410&tochange=224045731c6f2cf9f8905755937a542818f3b316

> 224045731c6f	Emilio Cobos Álvarez — Bug 1477551 - Ensure the clip and the rect that we push for bullets agree. r=jrmuizel

As everything is broken in some way, I can't call this a clear regression.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

There's something very wrong with the clipping as soon as something is translated, trying to reduce it now.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Farily reduced.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9009675 [details]
Farily reduced.

If you remove the transform of the .slide element then the bullets look right. Otherwise it's a whole lot of fun with clipping gone wrong.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Moar reduced.

Nested transforms, perspective, and clipping, yay!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So looks like https://github.com/servo/webrender/compare/5fa5c46e167ca834d8fec3bf662bf420418698f4...69dae1fe743bf3656d532672366e7193b041b6f7 made it go from showing bullets (with the caveat from bug 1477551) to 'not showing bullets'. 

https://github.com/servo/webrender/compare/232550f260fb8d6206c54362f16801ec9f0b154f...59656aa374706f25d7dc1b3b2c44970377efec22 made them show again.

And finally https://github.com/servo/webrender/compare/27e3bc4293d4a5057346739bfe8b7a163cf068f3...04d63e7d73b9661d9eb934a0933c8f9751a9a3db made them cropped.

Glenn, can you take a look at this or should I? Looks a regression from one of your recent coordinate-space changes.

Let me know if you want me to poke at it and I will, though. Thanks!

Comment 6

[Glenn Watson [:gw]]

I won't have cycles in the next couple days to look at it, but could possibly look at it late this week / early next week.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Alright, I can look at it then.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I've been trying to reduce it to a yaml thing I can repro on WR but no such luck so far...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: reduced yaml test-case

Took a bit but here it is.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(Need to run with -p 1 to repro, that being said)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Uncommenting

  https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/gfx/webrender/res/clip_shared.glsl#80

Seems to fix this. Of course I have no idea why yet.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: More reduced

Just for reference during IRC discussion :)

Comment 13

[Glenn Watson [:gw]]

If commenting that line fixes it, it's possible that https://github.com/servo/webrender/pull/3073 (not the nvidia specific fix, but the associated clip snapping fix that is part of the patch) may fix this bug, and would be worth testing.

Comment 14

[Darkspirit]

attachment 9009682 [details] is not fixed in https://github.com/servo/webrender/pull/3073#issuecomment-422234381.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

So I think I understand what's going on.

Before the regressing patch, for the test-case from comment 12 (remember that I need -p 1 to see the wrong clipping, but should be able to come up with a more reliable test-case), the render task rect for the bullet was transformed (35x34). That rectangle came around from the clipped_world_rect that we built while updating the clip task.

Now the render task rect is not transformed (it's 48x48), because we raster it in the same coordinate space. That all makes sense and the math is fine.

However we still pass the transform (note that in this case clip_transform == prim_transform) around, and we do snap using it. We're only accounting for that, not for prim_transform, so we snap incorrectly:

  https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/gfx/webrender/res/clip_shared.glsl#62

In this case given the transforms are the same I think we should avoid snapping... But we probably actually need to account for both...

I guess I'd need to think how this would work if we had two different transforms, one axis-aligned and one that's not. Maybe, I think, what should happen (if I'm understanding correctly) is that we snap using the difference between the prim transform and the clip transform (clip_transform.m * prim_transform.inv_m)? And probably only snap if both are axis-aligned...

Glenn, does that make sense?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

22:15 <gw> emilio: I *think* that makes sense, but probably need to re-read in detail. Sounds like it might be fairly quick to do some experiments and see if that does fix it?
22:16 <emilio> gw: I mean that would fix it, because we'd snap with the identity matrix, which is equivalent to comment out that line, I was concerned about the general case
22:17 <emilio> gw: I think that's the way to go though... if you think that makes sense I may as well submit it to try and see if anything complains.
22:19 <emilio> gw: I'll check it's not totally broken, and then submit it so you can double-check that comment carefully :)
22:19 <gw> emilio: yea, snapping stuff is subtle enough that it's hard to say without a try run, unfortunately :)
22:19 <gw> emilio: sounds good!