Bug 61446 – [Texmap][Qt] Accelerated animation is not paused properly.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Young Han Lee]

Created attachment 94799 [details]
Testcase

This bug can be reproduced with texture mapper.

[Young Han Lee]

void TextureMapperNode::syncCompositingStateSelf(GraphicsLayerTextureMapper* graphicsLayer, TextureMapper* textureMapper)
{
	......
    if (!hasRunningOpacityAnimation())
        m_opacity = m_state.opacity;
    if (!hasRunningTransformAnimation()) {
        m_transforms.base = m_state.transform;
    }
}

The transform matrix of an accelerated animation is initialized to zero by above code after the animation is paused.

Does the transform matrix really have to be synced with GraphicsLayer even when the node has a paused animation?
I wonder if it should be synced only when it doesn't have any animations.

Any idea?

[Noam Rosenthal]

Probably doesn't need to be reverted if we're paused, I think you're right.

[Young Han Lee]

Created attachment 94897 [details]
Patch

[Young Han Lee]

(In reply to comment #2)
> Probably doesn't need to be reverted if we're paused, I think you're right.

Thanks for the comment :)
I uploaded a patch with new test.

[Noam Rosenthal]

The content looks good to me.

[WebKit Review Bot]

Comment on attachment 94897 [details]
Patch

Attachment 94897 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8737123

New failing tests:
animations/play-state-paused.html

[WebKit Review Bot]

Created attachment 94941 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Young Han Lee]

Created attachment 94981 [details]
Patch

[Young Han Lee]

Test case is slightly modified to appease the chromium bot.

[Kenneth Rohde Christiansen]

No'am can you have a look at this?

[Noam Rosenthal]

My LGTM from above still stands, since the only change in this patch is in the test :)

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> My LGTM from above still stands, since the only change in this patch is in the test :)

Not really... Please look again :-)

[Noam Rosenthal]

(In reply to comment #12)
> (In reply to comment #11)
> > My LGTM from above still stands, since the only change in this patch is in the test :)
> 
> Not really... Please look again :-)

Looked again. Patch looks OK.

[Kenneth Rohde Christiansen]

Comment on attachment 94981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94981&action=review

> LayoutTests/ChangeLog:8
> +        The transform matrix of an accelerated animation shouldn't be synced when the animation is paused.

Looking at the png they seem pretty sync'ed.

[Young Han Lee]

(In reply to comment #14)
> (From update of attachment 94981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94981&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        The transform matrix of an accelerated animation shouldn't be synced when the animation is paused.
> 
> Looking at the png they seem pretty sync'ed.

Thanks for the review, Kenneth.

The changeLog means that the transform matrix of an accelerated animation shouldn't be synced with 'the matrix of the GraphicsLayer' and is not about the png.

Well, that log could be a bit confused. :(

[Young Han Lee]

cq-, because the patch has made commit-bots fail over and over again. (Oops..)

Patch 94981 from bug 61446: 3 minutes ago (cr-jail-8) Patch does not pass tests [results]
Patch 94981 from bug 61446: 7 minutes ago (cr-jail-7) Patch does not pass tests [results]
Patch 94981 from bug 61446: 17 minutes ago (cr-jail-8) Patch does not pass tests [results]
Patch 94981 from bug 61446: 21 minutes ago (cr-jail-7) Patch does not pass tests [results]
Patch 94981 from bug 61446: 32 minutes ago (cr-jail-8) Built patch

[Young Han Lee]

It seems that I have to run the test in chromium-mac port and figure out why it fails.

[Eric Seidel (no email)]

animations/play-state-paused.html -> failed
 was failing (flaky?) while trying to commit this.

[Young Han Lee]

Created attachment 95610 [details]
Patch

[Young Han Lee]

Testcase has been changed to use runAnimationTest() to avoid flakiness, and the WebCore part remains unchanged.

[WebKit Commit Bot]

Comment on attachment 95610 [details]
Patch

Clearing flags on attachment: 95610

Committed r88247: <http://trac.webkit.org/changeset/88247>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.