Some broken bullet points

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: darkspirit, Assigned: emilio)

Tracking

(Blocks 2 bugs, {correctness, nightly-community})

Trunk
mozilla64
Desktop
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 disabled, firefox64 fixed)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

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.
Reporter

Updated

9 months ago
OS: Linux → All
Hardware: x86_64 → Desktop
Assignee

Updated

9 months ago
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Assignee

Comment 1

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

Comment 2

9 months ago
Posted file Farily reduced. (obsolete) —
Flags: needinfo?(emilio)
Assignee

Comment 3

9 months ago
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.
Attachment #9009675 - Attachment mime type: text/plain → text/html
Assignee

Comment 4

9 months ago
Posted file Moar reduced. (obsolete) —
Nested transforms, perspective, and clipping, yay!
Attachment #9009675 - Attachment is obsolete: true
Assignee

Updated

9 months ago
Attachment #9009682 - Attachment mime type: text/plain → text/html
Assignee

Comment 5

9 months ago
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!
Flags: needinfo?(gwatson)

Comment 6

9 months ago
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.
Flags: needinfo?(gwatson)
Assignee

Comment 7

9 months ago
Alright, I can look at it then.
Flags: needinfo?(emilio)
Assignee

Comment 8

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

Comment 9

9 months ago
Posted file reduced yaml test-case (obsolete) —
Took a bit but here it is.
Assignee

Comment 10

9 months ago
(Need to run with -p 1 to repro, that being said)
Assignee

Comment 12

9 months ago
Posted file More reduced
Just for reference during IRC discussion :)
Attachment #9009682 - Attachment is obsolete: true
Attachment #9009954 - Attachment is obsolete: true
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.
Assignee

Comment 15

9 months ago
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?
Flags: needinfo?(emilio) → needinfo?(gwatson)
Assignee

Comment 16

9 months ago
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!
Flags: needinfo?(gwatson)
Reporter

Updated

9 months ago
Depends on: 1492880
Reporter

Updated

9 months ago
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.