Bug 61446 - [Texmap][Qt] Accelerated animation is not paused properly.
Summary: [Texmap][Qt] Accelerated animation is not paused properly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 47068
  Show dependency treegraph
 
Reported: 2011-05-25 09:57 PDT by Young Han Lee
Modified: 2011-06-07 09:58 PDT (History)
9 users (show)

See Also:


Attachments
Testcase (1.98 KB, text/html)
2011-05-25 09:57 PDT, Young Han Lee
no flags Details
Patch (17.82 KB, patch)
2011-05-25 18:25 PDT, Young Han Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.17 MB, application/zip)
2011-05-26 01:41 PDT, WebKit Review Bot
no flags Details
Patch (8.91 KB, patch)
2011-05-26 08:55 PDT, Young Han Lee
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2011-06-01 09:03 PDT, Young Han Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Young Han Lee 2011-05-25 09:57:28 PDT
Created attachment 94799 [details]
Testcase

This bug can be reproduced with texture mapper.
Comment 1 Young Han Lee 2011-05-25 10:30:47 PDT
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?
Comment 2 Noam Rosenthal 2011-05-25 11:02:31 PDT
Probably doesn't need to be reverted if we're paused, I think you're right.
Comment 3 Young Han Lee 2011-05-25 18:25:46 PDT
Created attachment 94897 [details]
Patch
Comment 4 Young Han Lee 2011-05-25 18:27:53 PDT
(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.
Comment 5 Noam Rosenthal 2011-05-25 20:45:00 PDT
The content looks good to me.
Comment 6 WebKit Review Bot 2011-05-26 01:41:44 PDT
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
Comment 7 WebKit Review Bot 2011-05-26 01:41:49 PDT
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
Comment 8 Young Han Lee 2011-05-26 08:55:43 PDT
Created attachment 94981 [details]
Patch
Comment 9 Young Han Lee 2011-05-26 08:58:00 PDT
Test case is slightly modified to appease the chromium bot.
Comment 10 Kenneth Rohde Christiansen 2011-05-31 01:24:51 PDT
No'am can you have a look at this?
Comment 11 Noam Rosenthal 2011-05-31 06:13:09 PDT
My LGTM from above still stands, since the only change in this patch is in the test :)
Comment 12 Kenneth Rohde Christiansen 2011-05-31 06:15:45 PDT
(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 :-)
Comment 13 Noam Rosenthal 2011-05-31 06:46:21 PDT
(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.
Comment 14 Kenneth Rohde Christiansen 2011-05-31 06:46:47 PDT
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.
Comment 15 Young Han Lee 2011-05-31 07:31:49 PDT
(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. :(
Comment 16 Young Han Lee 2011-05-31 22:12:02 PDT
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
Comment 17 Young Han Lee 2011-05-31 22:48:56 PDT
It seems that I have to run the test in chromium-mac port and figure out why it fails.
Comment 18 Eric Seidel (no email) 2011-05-31 23:25:34 PDT
animations/play-state-paused.html -> failed
 was failing (flaky?) while trying to commit this.
Comment 19 Young Han Lee 2011-06-01 09:03:50 PDT
Created attachment 95610 [details]
Patch
Comment 20 Young Han Lee 2011-06-01 09:04:39 PDT
Testcase has been changed to use runAnimationTest() to avoid flakiness, and the WebCore part remains unchanged.
Comment 21 WebKit Commit Bot 2011-06-07 09:58:37 PDT
Comment on attachment 95610 [details]
Patch

Clearing flags on attachment: 95610

Committed r88247: <http://trac.webkit.org/changeset/88247>
Comment 22 WebKit Commit Bot 2011-06-07 09:58:46 PDT
All reviewed patches have been landed.  Closing bug.