Closed Bug 1320608 Opened 8 years ago Closed 7 years ago

Transform animation on table element does not run on the compositor

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug)

Details

Attachments

(9 files)

A sample
270 bytes, text/html
Details
Bug 1320608 - Add forward declaration for nsIFrame in AnimationCollection.h.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Rename GetAnimationFrame to GetStyleFrame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Use the primary frame for AnimationInfo::GetGenerationFromFrame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Use style frame for animation stuff in AddAnimationsForProperty.
59 bytes, text/x-review-board-request
birtles
: review+
mattwoodrow
: review+
Details
Bug 1320608 - Drop the check for return value of EffectCompositor::GetAnimationElementAndPseudoForFrame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Add an assertion that checks AnimationInfo::GetGenerationFromFrame() receives the primary frame or the first continuation frame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Test case for transform animation on table element.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1320608 - Make sure we wait for the next frame in the case where the animation started at the current frame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file A sample —
No description provided.
Priority: -- → P3
I believe fixing this bug makes our nsIFrame handling in animation code proper, and then it will fix bug 1437036.
Blocks: 1437036
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8973575 [details] Bug 1320608 - Add forward declaration for nsIFrame in AnimationCollection.h. https://reviewboard.mozilla.org/r/241922/#review247780
Attachment #8973575 - Flags: review?(bbirtles) → review+
Comment on attachment 8973576 [details] Bug 1320608 - Rename GetAnimationFrame to GetStyleFrame. https://reviewboard.mozilla.org/r/241924/#review247782
Attachment #8973576 - Flags: review?(bbirtles) → review+
Comment on attachment 8973577 [details] Bug 1320608 - Use the primary frame for AnimationInfo::GetGenerationFromFrame. https://reviewboard.mozilla.org/r/241926/#review247784
Attachment #8973577 - Flags: review?(bbirtles) → review+
Comment on attachment 8973578 [details] Bug 1320608 - Use style frame for animation stuff in AddAnimationsForProperty. https://reviewboard.mozilla.org/r/241928/#review247786 This patch could use a little more explanation. ::: layout/painting/nsDisplayList.cpp:608 (Diff revision 1) > + nsIFrame* styleFrame = > + nsLayoutUtils::GetStyleFrame(const_cast<nsIFrame*>(aFrame)); Why is this const_cast necessary? ::: layout/painting/nsDisplayList.cpp:619 (Diff revision 1) > uint64_t animationGeneration = > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > + RestyleManager::GetAnimationGenerationForFrame(styleFrame); I don't follow this. I thought we switched to using the primary frame for the animation generation in the previous patch?
Comment on attachment 8973579 [details] Bug 1320608 - Drop the check for return value of EffectCompositor::GetAnimationElementAndPseudoForFrame. https://reviewboard.mozilla.org/r/241930/#review247788 ::: dom/animation/EffectCompositor.cpp:170 (Diff revision 1) > // basically a no-op. > Maybe<NonOwningAnimationTarget> pseudoElement = > EffectCompositor::GetAnimationElementAndPseudoForFrame(aFrame); > - if (pseudoElement) { > + MOZ_ASSERT(pseudoElement, > + "We have a valid element for the frame, if we don't we should " > + "have bailed out at above effect set check"); s/at above effect set check/after the call to EffectSet::GetEffectSet/
Attachment #8973579 - Flags: review?(bbirtles) → review+
Comment on attachment 8973580 [details] Bug 1320608 - Add an assertion that checks AnimationInfo::GetGenerationFromFrame() receives the primary frame or the first continuation frame. https://reviewboard.mozilla.org/r/241932/#review247790
Attachment #8973580 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #13) > ::: layout/painting/nsDisplayList.cpp:619 > (Diff revision 1) > > uint64_t animationGeneration = > > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > > + RestyleManager::GetAnimationGenerationForFrame(styleFrame); > > I don't follow this. I thought we switched to using the primary frame for > the animation generation in the previous patch? This is a confusing point. RestyleManager::GetAnimationGenerationForFrame() calls EffectSet::GetEffectSet(), so as you know, GetEffectSets() expects the style frame instead of the primary frame. Whereas, layer stuff (and WebRender stuff) expects the primary frame in FrameLayerBuild::GetDedicatedLayer() or GetWebRenderUserData(). I should have left a comment there.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16) > (In reply to Brian Birtles (:birtles) from comment #13) > > ::: layout/painting/nsDisplayList.cpp:619 > > (Diff revision 1) > > > uint64_t animationGeneration = > > > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > > > + RestyleManager::GetAnimationGenerationForFrame(styleFrame); > > > > I don't follow this. I thought we switched to using the primary frame for > > the animation generation in the previous patch? > > This is a confusing point. RestyleManager::GetAnimationGenerationForFrame() > calls EffectSet::GetEffectSet(), so as you know, GetEffectSets() expects the > style frame instead of the primary frame. Whereas, layer stuff (and > WebRender stuff) expects the primary frame in > FrameLayerBuild::GetDedicatedLayer() or GetWebRenderUserData(). I should > have left a comment there. I did want to add an assertion in GetEffectSets() or GetAnimationElementAndPseudoForFrame() if we receive non-style frame, but couldn't. Because there are lots of callers of GetEffectSets() to check whether there is an animation or not, regardless of whether style frame or not. That's said, now I noticed that we can add an assertion in AnimationInfo::GetGenerationFromFrame() in the case of table frame, the frame is the wrapper frame not the style frame. I've already add an assertion there in bug 1459388 (as Matt suggested), it should add another condition for this bug.
Blocks: 1459533
Comment on attachment 8973583 [details] Bug 1320608 - Test case for transform animation on table element. https://reviewboard.mozilla.org/r/241934/#review247802 Pretty sure I r+'ed this already. Maybe MozReview got confused.
Attachment #8973583 - Flags: review?(bbirtles) → review+
Did you want me to review the last patch? I'm not sure I understand it. Why would it take more than one frame to send the animation to the compositor?
Comment on attachment 8973578 [details] Bug 1320608 - Use style frame for animation stuff in AddAnimationsForProperty. https://reviewboard.mozilla.org/r/241928/#review247804 ::: layout/painting/nsDisplayList.cpp:609 (Diff revision 2) > } else { > aAnimationInfo.ClearAnimations(); > } > > + nsIFrame* styleFrame = > + nsLayoutUtils::GetStyleFrame(const_cast<nsIFrame*>(aFrame)); As discussed, we can drop the const_cast here. ::: layout/painting/nsDisplayList.cpp:620 (Diff revision 2) > // any early returns since even if we don't add any animations to the > // layer, we still need to mark it as up-to-date with regards to animations. > // Otherwise, in RestyleManager we'll notice the discrepancy between the > // animation generation numbers and update the layer indefinitely. > uint64_t animationGeneration = > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > + // Note the GetAnimationGenerationForFrame() calles EffectSet::GetEffectSet s/Note the/Note that/ (or simply s/Note the//) s/calles/calls/ ::: layout/painting/nsDisplayList.cpp:621 (Diff revision 2) > // layer, we still need to mark it as up-to-date with regards to animations. > // Otherwise, in RestyleManager we'll notice the discrepancy between the > // animation generation numbers and update the layer indefinitely. > uint64_t animationGeneration = > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > + // Note the GetAnimationGenerationForFrame() calles EffectSet::GetEffectSet > + // that supposes to work with the style frame instead of the primary frame. s/that supposes to work/that expects to work/ (This sounds like something we should fix somewhere, but I'm not sure where. Please file a bug if you have any ideas.)
Attachment #8973578 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #25) > Did you want me to review the last patch? I'm not sure I understand it. Why > would it take more than one frame to send the animation to the compositor? I was holding the review request until the try finished (I thought I did leave a comment in this bug, actually I didn't). https://treeherder.mozilla.org/#/jobs?repo=try&revision=a487f99e240170d7a852a7c0b5ff9e5aac323b13 Now the tweak works fine.
Attachment #8973584 - Flags: review?(bbirtles)
Comment on attachment 8973584 [details] Bug 1320608 - Make sure we wait for the next frame in the case where the animation started at the current frame. https://reviewboard.mozilla.org/r/241936/#review247820 ::: commit-message-10eb6:3 (Diff revision 1) > +There are cases that we can't send the animation on the compositor even if > +we wait for one more frame in the case where the animation had been pending. Just curious, why is that?
(In reply to Brian Birtles (:birtles) from comment #28) > Comment on attachment 8973584 [details] > Bug 1320608 - Wait to start animation before waiting for painting the > animation. > > https://reviewboard.mozilla.org/r/241936/#review247820 > > ::: commit-message-10eb6:3 > (Diff revision 1) > > +There are cases that we can't send the animation on the compositor even if > > +we wait for one more frame in the case where the animation had been pending. > > Just curious, why is that? I don't know. The failure happened on CCOV linux, so it's just due to slowness on the build.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29) > I don't know. The failure happened on CCOV linux, so it's just due to > slowness on the build. I meant 'probably' it's caused by the slowness.
Comment on attachment 8973584 [details] Bug 1320608 - Make sure we wait for the next frame in the case where the animation started at the current frame. https://reviewboard.mozilla.org/r/241936/#review247824 ::: dom/animation/test/chrome/test_running_on_compositor.html:981 (Diff revision 1) > - if (animationStartsRightNow(animation)) { > + await waitToStartAnimation(animation); > - await waitForFrame(); > - } I was trying to work out how animationStartRightNow could possible return true on separate frames given that it is comparing `aAnimation.startTime === aAnimation.timeline.currentTime`. Assuming that aAnimation.timeline.currentTime changes between frames, that would mean startTime is updated on each frame but I don't think we do that. I think what is happening here is that we are in the animation.ready promise callback and when we wait for a frame we end up in the same frame. So I think the fix here is just to use waitForNextFrame instead of waitForFrame. Does that sound right?
(In reply to Brian Birtles (:birtles) from comment #31) > Comment on attachment 8973584 [details] > Bug 1320608 - Wait to start animation before waiting for painting the > animation. > > https://reviewboard.mozilla.org/r/241936/#review247824 > > ::: dom/animation/test/chrome/test_running_on_compositor.html:981 > (Diff revision 1) > > - if (animationStartsRightNow(animation)) { > > + await waitToStartAnimation(animation); > > - await waitForFrame(); > > - } > > I was trying to work out how animationStartRightNow could possible return > true on separate frames given that it is comparing `aAnimation.startTime === > aAnimation.timeline.currentTime`. > > Assuming that aAnimation.timeline.currentTime changes between frames, that > would mean startTime is updated on each frame but I don't think we do that. > > I think what is happening here is that we are in the animation.ready promise > callback and when we wait for a frame we end up in the same frame. So I > think the fix here is just to use waitForNextFrame instead of waitForFrame. > > Does that sound right? Oh right, that's plausible. I will try another try with it. And we need to fix the first test case in the same file. I did copy it from there.
Comment on attachment 8973578 [details] Bug 1320608 - Use style frame for animation stuff in AddAnimationsForProperty. https://reviewboard.mozilla.org/r/241928/#review248076
Attachment #8973578 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8973584 [details] Bug 1320608 - Make sure we wait for the next frame in the case where the animation started at the current frame. https://reviewboard.mozilla.org/r/241936/#review248078 Personally, I'd probably prefer not creating a separate function (since I suspect if I'm reading the test I'll end up looking up the function definition anyway) but it's fine either way. ::: dom/animation/test/testcommon.js:426 (Diff revision 2) > function animationStartsRightNow(aAnimation) { > return aAnimation.startTime === aAnimation.timeline.currentTime && > aAnimation.currentTime === 0; > } > > +// Waits until the animation has surely started (e.g. the animation has been s/e.g./i.e./
Attachment #8973584 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #26) > ::: layout/painting/nsDisplayList.cpp:621 > (Diff revision 2) > > // layer, we still need to mark it as up-to-date with regards to animations. > > // Otherwise, in RestyleManager we'll notice the discrepancy between the > > // animation generation numbers and update the layer indefinitely. > > uint64_t animationGeneration = > > - RestyleManager::GetAnimationGenerationForFrame(aFrame); > > + // Note the GetAnimationGenerationForFrame() calles EffectSet::GetEffectSet > > + // that supposes to work with the style frame instead of the primary frame. > > s/that supposes to work/that expects to work/ > > (This sounds like something we should fix somewhere, but I'm not sure where. > Please file a bug if you have any ideas.) I don't have any clear idea for that yet, but filed a relevant bug (bug 1459776).
I am holding to land these patches since one of the patches (attachment attachment 8973580 [details] conflicts with a patch for bug 1459388).
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30687af2ab06 Add forward declaration for nsIFrame in AnimationCollection.h. r=birtles https://hg.mozilla.org/integration/autoland/rev/f560387becbb Rename GetAnimationFrame to GetStyleFrame. r=birtles https://hg.mozilla.org/integration/autoland/rev/fd669b8a95ae Use the primary frame for AnimationInfo::GetGenerationFromFrame. r=birtles https://hg.mozilla.org/integration/autoland/rev/1c22a7d103dd Add an assertion that checks AnimationInfo::GetGenerationFromFrame() receives the primary frame or the first continuation frame. r=birtles https://hg.mozilla.org/integration/autoland/rev/30449867a680 Use style frame for animation stuff in AddAnimationsForProperty. r=birtles,mattwoodrow https://hg.mozilla.org/integration/autoland/rev/2681e94e4c67 Drop the check for return value of EffectCompositor::GetAnimationElementAndPseudoForFrame. r=birtles https://hg.mozilla.org/integration/autoland/rev/a9f8b9115f75 Test case for transform animation on table element. r=birtles https://hg.mozilla.org/integration/autoland/rev/201a77bfe3bc Make sure we wait for the next frame in the case where the animation started at the current frame. r=birtles
Depends on: 1467638
Depends on: 1508127
Depends on: 1518816
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: