1179535 - mouse movement during off-main-thread (OMT) animation causes unnecessary repainting

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

While debugging bug 1176969, I noticed that mouse movement during off-main-thread (OMT) animation was triggering unnecessary repainting.  (This is, in fact, a key part of the original steps-to-reproduce in the bug.)

This happens because mousemove handling calls FlushPendingEvents:
https://hg.mozilla.org/mozilla-central/file/c36f68439496/dom/events/EventStateManager.cpp#l642
which in turn calls FlushPendingNotifications, which processes style changes, including those that have been suppressed because of the throttling of main thread updates of animations that are running on the compositor thread.

(I'm actually not sure why this wasn't triggered earlier by this code:
https://hg.mozilla.org/mozilla-central/file/c36f68439496/layout/base/nsPresShell.cpp#l7172 )

This, in turn, leads us to process style changes to transform, which in turn leads to a completely unnecessary repaint.  (I'd think it's a relatively cheap repaint, but we still do a bit of work.)

This, in turn, pokes the layer activity timer, I think on a descendant that has an inactive layer (I guess activating it and causing a real repaint)... which then resets a few hundred milliseconds later, causing another repaint.


We should find a way to avoid doing all this unnecessary work.

Comment 1

[Markus Stange [:mstange]]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #0)
> This, in turn, pokes the layer activity timer, I think on a descendant that
> has an inactive layer (I guess activating it and causing a real repaint)...
> which then resets a few hundred milliseconds later, causing another repaint.

This is the most worrying part of all this, I think. If a layer is animated, it should be active, and if it's not animated, flushing shouldn't be able to activate it.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Yeah, the layer activity mess reminds me of bug 1149848.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⌚UTC+9 [busy, returning November 2] from comment #0)
> While debugging bug 1176969, I noticed that mouse movement during
> off-main-thread (OMT) animation was triggering unnecessary repainting.

Was this extra-repainting exposed by paint flashing? (and is it still an issue?)  If yes&yes, do you have a testcase which triggers it?

I tried the "simple testcase" on bug 1176969, edited to remove all 3d-related styles (to avoid benefiting from its patch, which was about an exemption for 3D scenes).  But I don't see any paint flashing when I move my mouse over the playing animation, in current Nightly w/ fresh profile. (Maybe there's an about:config tweak that I'm missing, though.)

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I can look in a little more detail later, but I'll note that when I saw this originally, I was debugging using breakpoints in gdb, and I didn't test what paint flashing showed.  (That said, I'd think that paint flashing would show repaints resulting from de/re-layerization, although I'm not 100% sure.)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I had been seeing this when I had tried to implement Frame Timing API. At that time I don't remember exactly this issue happened on non-E10S. I did not use paint-flashing but mViewManagerFlushIsPending was actually true there.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> I tried the "simple testcase" on bug 1176969, edited to remove all
> 3d-related styles (to avoid benefiting from its patch, which was about an
> exemption for 3D scenes).  But I don't see any paint flashing when I move my
> mouse over the playing animation, in current Nightly w/ fresh profile.
> (Maybe there's an about:config tweak that I'm missing, though.)

I'm not sure this was the right editing to do, since removing the 3-d stuff would make things eligible for compositor-thread animation, and having something that *wasn't* being animated on the compositor was important to the steps to reproduce the bug.

I suspect that to reproduce the bug you'd instead want to modify the testcase to have a transform animation on an ancestor of the preserve-3d scene, but keep the animations on the things in the scene as well.  Or something more like that.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

... and note that *that* will in turn work only as long as bug 779598 is unfixed.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

attachment 8689804 [details] is an example where this is visible due to a still-extant bug.  The red outline only redraws when I move my mouse.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

It looks necessary paint to me. That's because the transform animation in attachment 8689804 [details] causes overflow.

mouse moving -> UpdateOnlyAnimationStyles() -> overflow -> paint red outline effected by the overflow.

A problem is that the transform animation is not unthrottled every 200ms?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> It looks necessary paint to me. That's because the transform animation in
> attachment 8689804 [details] causes overflow.
> 
> mouse moving -> UpdateOnlyAnimationStyles() -> overflow -> paint red outline
> effected by the overflow.
> 
> A problem is that the transform animation is not unthrottled every 200ms?

is *unthrottled*.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > It looks necessary paint to me. That's because the transform animation in
> > attachment 8689804 [details] causes overflow.
> > 
> > mouse moving -> UpdateOnlyAnimationStyles() -> overflow -> paint red outline
> > effected by the overflow.
> > 
> > A problem is that the transform animation is not unthrottled every 200ms?
> 
> is *unthrottled*.

Wrong! "not unthrottled" is what I wanted to say.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> It looks necessary paint to me. That's because the transform animation in
> attachment 8689804 [details] causes overflow.

That's why I said that testcase was related to a still-extant bug.  But a bug different from this one.  This is still a bug, and a pretty serious bug.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #0)
> While debugging bug 1176969, I noticed that mouse movement during
> off-main-thread (OMT) animation was triggering unnecessary repainting. 
> (This is, in fact, a key part of the original steps-to-reproduce in the bug.)
> 
> This happens because mousemove handling calls FlushPendingEvents:
> https://hg.mozilla.org/mozilla-central/file/c36f68439496/dom/events/
> EventStateManager.cpp#l642

The FlushPendingEvents will be suppressed by attachment 8690008 [details] [diff] [review] for bug 1219236, but I found there is another code path which leads to unnecessary paint.

The code path is:
FlushThrottledStyles
 RestyleManager::UpdateOnlyAnimationStyles
  RestyleTracker::DoProcessRestyles
   RestyleTracker::ProcessOneRestyle
    RestyleManager::RestyleElement
     RestyleManager::ComputeAndProcessStyleChange
      RestyleManager::ProcessRestyledFrames
       ApplyRenderingChangeToTree
        DoApplyRenderingChangeToTree
         nsIFrame::SchedulePaint

We should fix UpdateOnlyAnimationStyles to flush only styles.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> The code path is:
> FlushThrottledStyles
>  RestyleManager::UpdateOnlyAnimationStyles
[...]
> 
> We should fix UpdateOnlyAnimationStyles to flush only styles.

No, UpdateOnlyAnimationStyles needs to process the change hints; otherwise we'll be showing something that's incorrect, and it might not get corrected later.

And, in general, we should expect FlushThrottledStyles to cause real style changes.  That's what it's for.  If this was a point when you didn't expect the throttled styles to be flushed (which it sounds like it was), you should investigate why FlushThrottledStyles was called.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Ah, then we should do tweaks in DoApplyRenderingChangeToTree[1] in case of processing restyles caused by events?

[1] https://dxr.mozilla.org/mozilla-central/rev/d3d286102ba7f8801e9dfe12d534f49554ba50c0/layout/base/RestyleManager.cpp#298,309

Comment 16

[Hiroyuki Ikezoe (:hiro)]

OK. An easy way I can think of is to add a flag which indicates that event is being handled to RestyleManager, and set the flag before calling UpdateOnlyAnimationStyles (and clear the flag after UpdateOnlyAnimationStyles) in FlushThrottledStyles.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> OK. An easy way I can think of is to add a flag which indicates that event
> is being handled to RestyleManager, and set the flag before calling
> UpdateOnlyAnimationStyles (and clear the flag after
> UpdateOnlyAnimationStyles) in FlushThrottledStyles.

And if the flag is set, SchedulePaint is skipped in DoApplyRenderingChangeToTree.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Don't paint compositor animations while handling mouse event.

This is the patch I described in comment #16 and #17.

With this patch attachment 8689804 [details] does not cause paint while mouse movements.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Don't paint compositor animations while handling mouse event.

I am sorry the previous patch did not include important files.

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8691681 [details] [diff] [review]
Don't paint compositor animations while handling mouse event.

I don't think this is a safe approach.  If we drop style changes that require invalidation on the floor, and the change in question was the last change of that type, then we might never invalidate, when we in fact need to do so.

I think we need a different approach here.

One thought is that perhaps we could consider the current state of transform animations that are running on the compositor when choosing event targets.  (This sounds crazy enough to me that we should probably at least run it by some other people, e.g. roc, before doing it.)

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I also think there are two separate problems to fix here:

 (1) we should fix the layer activity mess independently of anything we're doing to fix mouse events specifically

 (2) we should consider seeing if we can avoid flushing compositor-running animations for mouse events, or if we can't, try to reduce the work we do when doing so.

I think (1) is going to keep coming back to bite us in other contexts until we fix it properly.