Closed Bug 1467765 Opened 2 years ago Closed 4 months ago

Sample OMTA transforms during the sampling step prior to rendering

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: kats, Assigned: hiro)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 4 obsolete files)

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
I take a look.
Assignee: nobody → sotaro.ikeda.g
\o/
Attached patch wip patch (obsolete) — Splinter Review
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
(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.
Attached patch wip patchSplinter Review
A bit clean up.
Attachment #8998466 - Attachment is obsolete: true
(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
Blocks: 1481991
(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.
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.
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. :)
No longer blocks: 1481991
See Also: → 1481991
Assignee: sotaro.ikeda.g → nobody
Priority: P2 → P3
Kats, what should the priority of this work be?
Flags: needinfo?(kats)
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.
Blocks: stage-wr-next
No longer blocks: stage-wr-trains
Flags: needinfo?(kats)

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.

Assignee: nobody → hikezoe.birchill
Blocks: 1321428
Status: NEW → ASSIGNED
Blocks: 1638152
Depends on: 1646263

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.

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

Depends on D79980

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

Pushed by hikezoe.birchill@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
You need to log in before you can comment on or make changes to this bug.