Closed Bug 1435282 Opened 2 years ago Closed 2 years ago

Enable test_group_hittest with webrender

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Prior to turning on gfx.webrender.hit-test in bug 1421380, I was able to run test_group_hittest.html in isolation and it worked fine. However, when running it as part of the entire suite would fail.

After much digging and investigation I figured out the chain of events causing this:
- The test would start and run, but WR would still have the state for the previous test, and so the hit-testing would fail.
- This in turn was caused by a backlog of GenerateFrame transactions that got sent to WR and had not yet been processed by the RenderBackend thread.
- This in turn was caused by a previous test calling advanceTimeAndRefresh(16) a lot of times, which would cause a lot of FlushRendering(false) calls at [1] - since this sets mForceRendering, it would forcibly make a GenerateFrame transaction, even if there were already pending transactions. It also didn't wait for those transactions to actually finish processing, because it doesn't call mApi->WaitFlushed in the async case. This resulted in the backlog - I was seeing "pending frame" counts as high as 65 in some cases.

After thinking about this a bunch I came to the conclusion that:

(a) it doesn't make sense to set mForceRendering=true unless we are also going to wait for the renderer. Basically, when FlushRendering is called in async mode, we only care about triggering the flush, and don't care *when* it happens. So we don't need to set mForceRendering because even without it, if a composite is needed, it will get scheduled (either there is already a pending one, or one will get scheduled at the vsync following the pending one getting processed).

(b) the advanceTimeAndRefresh function used in mochitests should actually block and wait for the rendering to happen. This is the behaviour expected in the non-WR case (although there is one less thread involved) so we should match that in the WR case.

[1] https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/gfx/layers/ipc/CompositorBridgeParent.cpp#1301
[2] https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/gfx/layers/wr/WebRenderBridgeParent.cpp#1350
Comment on attachment 8947895 [details]
Bug 1435282 - Document that function arguments are always null.

https://reviewboard.mozilla.org/r/217558/#review223480
Attachment #8947895 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8947896 [details]
Bug 1435282 - Add a helper FlushPendingComposite function.

https://reviewboard.mozilla.org/r/217560/#review223482
Attachment #8947896 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8947898 [details]
Bug 1435282 - Make SetTestSampleTime force a sync flush.

https://reviewboard.mozilla.org/r/217564/#review223484
Attachment #8947898 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8947897 - Flags: review?(sotaro.ikeda.g)
Apparently I forgot to request review on the third patch also. Sorry!
Comment on attachment 8947897 [details]
Bug 1435282 - Don't force the render transaction to happen immediately if we're doing an async flush.

https://reviewboard.mozilla.org/r/217562/#review223518
Attachment #8947897 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30b7617cb903
Document that function arguments are always null. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/ed13e2aac8f6
Add a helper FlushPendingComposite function. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/cf517d879754
Don't force the render transaction to happen immediately if we're doing an async flush. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/9a1800172967
Make SetTestSampleTime force a sync flush. r=sotaro
You need to log in before you can comment on or make changes to this bug.