1467765 - Sample OMTA transforms during the sampling step prior to rendering

Status: RESOLVED FIXED

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

Description

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

Right now, OMTA sampling happens at the time that we send the GenerateFrame transaction from WebRenderBridgeParent to webrender [1]. This sampling runs on the compositor thread, and the transaction is dispatched to the render backend thread. The render backend thread ideally should handle it right away, but it may be busy doing other stuff, and might not handle the transaction until a little bit later. In that case, the sampled OMTA values might be a little stale.

It would be better if we moved the sampling to happen as part of the sampling step at [2], which runs on the render backend thread at the time that the transaction is actually getting handled. This is where APZ transforms are sampled, and so it would be good for consistency to also sample OMTA at this point.

I left a todo in the code for this at [3], and this bug is tracking that work. The only tricky part will be ensuring that the OMTA stuff is properly threadsafe or locked because this sampling will happen on the render backend thread, while the data structures get updated on the compositor thread.

[1] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/gfx/layers/wr/WebRenderBridgeParent.cpp#1432
[2] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/gfx/webrender/src/render_backend.rs#1026
[3] https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/gfx/webrender_bindings/src/bindings.rs#767

Comment 1

[Sotaro Ikeda [:sotaro]]

I take a look.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

\o/

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> The only tricky part will be ensuring that the OMTA stuff is properly
> threadsafe or locked because this sampling will happen on the render backend
> thread, while the data structures get updated on the compositor thread.

I am bit concerned about this.  It seems to me that the situation commonly happens when user is moving mouse cursor on a content having transform/opacity animations.  That's because we do update animations on the compositor for hit testing.  Thus, we do repeatedly update the animation data on the compositor thread during the user is moving mouse cursor.  It might cause janky animations due to the lock?

Comment 5

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

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #4)
> I am bit concerned about this.  It seems to me that the situation commonly
> happens when user is moving mouse cursor on a content having
> transform/opacity animations.  That's because we do update animations on the
> compositor for hit testing.  Thus, we do repeatedly update the animation
> data on the compositor thread during the user is moving mouse cursor.  It
> might cause janky animations due to the lock?

I think it's unlikely the lock will cause animation jank; the lock should only be held for sub-millisecond times and we shouldn't be blocking on anything inside the lock. However I also don't think we need to update the animations on the compositor for hit-testing, the compositor should be able to hit-test OMTA just fine. So maybe we should spin off another bug to investigate and stop doing that if we don't need it.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Rebased.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

A bit clean up.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #4)
> 
> I am bit concerned about this.  It seems to me that the situation commonly
> happens when user is moving mouse cursor on a content having
> transform/opacity animations.  That's because we do update animations on the
> compositor for hit testing.  Thus, we do repeatedly update the animation
> data on the compositor thread during the user is moving mouse cursor.  It
> might cause janky animations due to the lock?

attachment 8998480 [details] [diff] [review] seems not affect to talos result.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ffe27a9a315ee2495be237cb4cfdd5de2e7e367c&newProject=try&newRevision=f27dac7efe0c97ec1f2278dccbe958ce24c89755&framework=1

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #4)
> > 
> > I am bit concerned about this.  It seems to me that the situation commonly
> > happens when user is moving mouse cursor on a content having
> > transform/opacity animations.  That's because we do update animations on the
> > compositor for hit testing.  Thus, we do repeatedly update the animation
> > data on the compositor thread during the user is moving mouse cursor.  It
> > might cause janky animations due to the lock?
> 
> attachment 8998480 [details] [diff] [review] seems not affect to talos
> result.
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=ffe27a9a315ee2495be237cb4cfdd5de
> 2e7e367c&newProject=try&newRevision=f27dac7efe0c97ec1f2278dccbe958ce24c89755&
> framework=1

As far as I know there is no such test case in our talos.

Comment 12

[Sotaro Ikeda [:sotaro]]

If the lock took long time, then it affected to RenderBackend thread performance and could affect to talos results. The result seems to say that lock did not took long time.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

It's slightly different what I am concerned. My concern was somewhat lock contention.  Anyway if you are convinced that no janky animations happen on a content having a bunch of transform animations during moving mouse, I am fine with the locks. :)

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Kats, what should the priority of this work be?

Comment 15

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

Hm, I would classify it as a "nice to have" for the MVP, but the patch will likely also introduce some risk of threading problems, race conditions etc. So let's bump it to wr-next. For context the main user-visible benefit here is that OMT animations will be more in sync with APZ scrolling, if they are happening concurrently. However I don't think anybody has complained about this issue yet so it's probably not hit frequently in practice to the point where people notice.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Blocking scroll timeline (bug 1321428) and jank partial prerender transform for WebRender (bug 1638152). At the time Botond implemented the initial scroll timeline, we didn't have WebRender backends, but now we have it.
I am going to work on this for bug 1638152.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: A rotation animation on (80vw, 80vh) sized element

I was suspecting that this change will make reftest-no-flush reftests flaky on WebRender, but it actually didn't at least on this try run (I re-triggered a bunch of times for the reftests in question there). https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2e8f2cba8c6f15f86930ecca2f180b493b76067

I did also try to see whether there will appear jankiness or not (that was my concern in comment 13), attaching file has a transform animation on relatively large frame, with this file as far as I can tell there is no jankiness happening at all, probably if there are tons of animations, jankiness might happen though.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1467765 - Factor out clear animation resources stuff. r?kats

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1467765 - Factor out resetting previous time stamp stuff. r?kats

|mPreviousFrameTimeStamp| will be moved into the sampler for OTMA in the next
commit though.

Depends on D79980

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1467765 - Sample off-main-thread animations on the render backend thread in case of WebRender. r?kats

Now CompositorAnimationStorage::GetAnimatedValue() and SetAnimatedValue()s
are called on the sampler thread in case of WebRender, are called on the
compositor thread in case of non WebRender, so we drop assertions of
IsInCompositorThread check there. A mLock.AssertCurrentThreadOwns call in
each function should make sure the function gets called on the
sampler/compositor thread with acquiring the lock.

One caveat in this change is that in case we try to get an animation value via
nsIDOMWindowUtils.getOMTAStyle(), we do sample animations on the compositor
thread and we never call UpdateDynamicProperties, which means if it gets called
in testing refresh driver mode, visual results will differ from what the value
returned by the getOMTAStyle should look like. But it should be fine because we
disallow using any chrome priviledge APIs in reftests and also we will never use
the testing refresh driver mode in the reftest harness. Also in mochitests the
visual results' differences might make people confusing if the person can notice
it, but in principal getOMTAStyle() is designed to get computed animating values
so that it doesn't matter what the visual result is.

Depends on D79981

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b43c4bccbe3d
Factor out clear animation resources stuff. r=kats
https://hg.mozilla.org/integration/autoland/rev/37cf2bf562f8
Factor out resetting previous time stamp stuff. r=kats
https://hg.mozilla.org/integration/autoland/rev/bd42396d45d7
Sample off-main-thread animations on the render backend thread in case of WebRender. r=kats

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b43c4bccbe3d
https://hg.mozilla.org/mozilla-central/rev/37cf2bf562f8
https://hg.mozilla.org/mozilla-central/rev/bd42396d45d7