Closed
Bug 1491555
Opened 6 years ago
Closed 6 years ago
Some broken bullet points
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla64
People
(Reporter: jan, Assigned: emilio)
References
(Blocks 1 open bug, )
Details
(Keywords: correctness, nightly-community)
Attachments
(2 files, 3 obsolete files)
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•6 years ago
|
OS: Linux → All
Hardware: x86_64 → Desktop
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Assignee | ||
Comment 1•6 years ago
|
||
There's something very wrong with the clipping as soon as something is translated, trying to reduce it now.
Assignee | ||
Comment 2•6 years ago
|
||
Flags: needinfo?(emilio)
Assignee | ||
Comment 3•6 years 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•6 years ago
|
||
Nested transforms, perspective, and clipping, yay!
Attachment #9009675 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #9009682 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 5•6 years 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•6 years 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 8•6 years 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•6 years ago
|
||
Took a bit but here it is.
Assignee | ||
Comment 10•6 years ago
|
||
(Need to run with -p 1 to repro, that being said)
Assignee | ||
Comment 11•6 years ago
|
||
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.
Assignee | ||
Comment 12•6 years ago
|
||
Just for reference during IRC discussion :)
Attachment #9009682 -
Attachment is obsolete: true
Attachment #9009954 -
Attachment is obsolete: true
Comment 13•6 years ago
|
||
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.
Reporter | ||
Comment 14•6 years ago
|
||
Assignee | ||
Comment 15•6 years 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•6 years 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•6 years ago
|
See Also: → https://github.com/servo/webrender/pull/3082
Reporter | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•