Closed
Bug 1421380
Opened 6 years ago
Closed 6 years ago
Turn on webrender hit-test by default
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [wr-reserve] [gfx-noted])
Attachments
(2 files)
Making a new meta-ish bug for the WR hit-test stuff. There are some more correctness issues that need fixing, and I'll hang them off this because I have other patches I want to land in bug 1389146. Also we'll need the webrender-side issues with hit-testing fixed before we can turn it on, and this bug can track those as well.
Assignee | ||
Comment 1•6 years ago
|
||
This can go into the reserve list for now.
Whiteboard: [gfx-noted][wr-mvp][triage] → [gfx-noted][wr-reserve][triage]
Updated•6 years ago
|
Whiteboard: [gfx-noted][wr-reserve][triage] → [wr-reserve] [gfx-noted]
Updated•6 years ago
|
Blocks: stage-wr-nightly
Comment 2•6 years ago
|
||
Kats, do you have performance numbers for EndTransaction times with and without this change?
Assignee | ||
Comment 3•6 years ago
|
||
No, I didn't do any benchmarking beyond just comparing MazeSolver and some MotionMark subtest numbers.
Updated•6 years ago
|
Priority: P3 → P1
Updated•6 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 4•6 years ago
|
||
IMO the only thing I really want to fix before turning this on is bug 1425451, because that race condition can produce unreliable behaviour that is difficult to diagnose and debug. The mochitest failures I'm ok with just marking fails-if for now.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19c915bca9b095c3e87ba66561d4762ceb6aa4ea Looks good. For the record I wanted to turn on the test_group_hittest.html because it now passes reliably for me locally. However my previous try push [2] still showed it failing. The screenshot of the failure shows that the test page hasn't even loaded, it's still showing the previous test - so I think the failure is not due to the hit-testing code per se, but more so the framework around loading the pages and waiting for APZ and whatnot. For now I'm leaving that test disabled as well in the interests of getting this feature turned on sooner. [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e9e325b3b15004aac08090482a92026a99eb32a&selectedJob=158295886
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8945503 [details] Bug 1421380 - Enable gfx.webrender.hit-test by default. https://reviewboard.mozilla.org/r/215656/#review221298
Attachment #8945503 -
Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/48cdd9ef5878 Enable gfx.webrender.hit-test by default. r=jrmuizel
Assignee | ||
Comment 9•6 years ago
|
||
I've requested that this be backed out until we have a chance to get to the bottom of bug 1433191.
Depends on: 1433191
Comment 10•6 years ago
|
||
Backed out 1 changesets (bug 1421380) on request from kats Backout: https://hg.mozilla.org/integration/autoland/rev/67722a0f7ad90ffe4f34c022f4120cbfb009d463
Updated•6 years ago
|
Flags: needinfo?(bugmail)
Comment 12•6 years ago
|
||
bugherder could-have-been-funny |
https://hg.mozilla.org/mozilla-central/rev/48cdd9ef5878
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 13•6 years ago
|
||
The backout merged to m-c also. https://hg.mozilla.org/mozilla-central/rev/67722a0f7ad9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•6 years ago
|
||
I haven't relanded this yet because my latest try push is showing debug assertions across the board: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0c423c7c58065cd3d92a8345dda9762b0f1bd4c Will need to investigate.
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14) > I haven't relanded this yet because my latest try push is showing debug > assertions across the board: The problem is simple enough - the WR hit-test triggers rendering/publishing of frames, which sends NewFrameReady notifications to gecko. However gecko assumes that all the frames it gets were requested by it, and manages two counters (the pending frame count and the rendering frame count) based on this assumption. The extra frames produced by hit-testing violates the assumptions here and causes an assertion failure. At first I wanted to fix this by suppressing the NewFrameReady activity on the gecko side when we're inside a hit-test call (via a RAII helper or similar). The problem with this is that we have recently sent a GenerateFrame request from the gecko side and the NewFrameReady notification from that might arrive while we're inside the suppression block. In this case we would incorrectly suppress a frame ready notification that we should actually be handling. If this works it would be highly dependent on which messages are sent from which threads which is a pretty brittle solution and I don't want to go down that road. Another solution, which seems more viable, is to update the call at [1] so that it is either skipped, or the last arg is set to false, in the case where the render is due to hit-testing. Seems a bit like cheating but it would be a band-aid until we get the "proper" fix of doing build_scene on a worker thread (and then eliminating this render-due-to-hit-testing entirely). [1] https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/gfx/webrender/src/render_backend.rs#706
Assignee | ||
Comment 16•6 years ago
|
||
Try push looks good, tested the build locally as well and it seems good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e99579c9d97cb7825cd51b49ab174db241d6c272
Depends on: 1433567
Assignee | ||
Updated•6 years ago
|
See Also: → https://github.com/servo/webrender/pull/2358
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8946421 [details] Bug 1421380 - Don't do a composite of WR rendered frames unless a composite is requested. https://reviewboard.mozilla.org/r/216372/#review222484 Makes sense.
Attachment #8946421 -
Flags: review?(nical.bugzilla) → review+
Comment 20•6 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c182e492a2e7 Don't do a composite of WR rendered frames unless a composite is requested. r=nical https://hg.mozilla.org/integration/autoland/rev/5fc179e245d5 Enable gfx.webrender.hit-test by default. r=jrmuizel
![]() |
||
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c182e492a2e7 https://hg.mozilla.org/mozilla-central/rev/5fc179e245d5
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•