1435282 - Enable test_group_hittest with webrender

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

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 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73b533e6846b78a7f524a17ff89d4ea72c74746b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=220b992b2d29c8d38d31b5f109578881dba1e191

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435282 - Document that function arguments are always null.

Review commit: https://reviewboard.mozilla.org/r/217558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217558/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435282 - Add a helper FlushPendingComposite function.

This extracts a code pattern that appears a couple of times in the code.
It occurs in CompositorBridgeParent as well but there's some extra stuff
involved there with the mOverrideComposeReadiness flag that I don't
understand so I'm leaving that as-is for now.

Review commit: https://reviewboard.mozilla.org/r/217560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217560/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435282 - Don't force the render transaction to happen immediately if we're doing an async flush.

This splits the FlushRendering function into sync and async versions
just for a bit more clarity. In the async version we don't want to set
the mForceRendering flag at all because we don't need to force a
rendering - if there is one already pending then that's good enough. And
anyway in practice the async version seems to only ever be invoked by
CompositorBridgeParent::SetTestSampleTime which I'll be changing in the
next patch.

Review commit: https://reviewboard.mozilla.org/r/217562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217562/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1435282 - Make SetTestSampleTime force a sync flush.

SetTestSampleTime is called from tests via the advanceTimeAndRefresh API
on DOMWindowUtils, and the expectation is that after it is done, the
time has been advanced and a composite has happened. So we need to
ensure that is the case with WebRender as well. This fixes the issue I
was seeing with test_group_hittest.html and makes it pass.

Review commit: https://reviewboard.mozilla.org/r/217564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217564/

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947895 [details]
Bug 1435282 - Document that function arguments are always null.

https://reviewboard.mozilla.org/r/217558/#review223480

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947896 [details]
Bug 1435282 - Add a helper FlushPendingComposite function.

https://reviewboard.mozilla.org/r/217560/#review223482

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8947898 [details]
Bug 1435282 - Make SetTestSampleTime force a sync flush.

https://reviewboard.mozilla.org/r/217564/#review223484

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Apparently I forgot to request review on the third patch also. Sorry!

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/30b7617cb903
https://hg.mozilla.org/mozilla-central/rev/ed13e2aac8f6
https://hg.mozilla.org/mozilla-central/rev/cf517d879754
https://hg.mozilla.org/mozilla-central/rev/9a1800172967