Bug 112582 – [CoordinatedGraphics] serviceScriptedAnimations expects time in seconds

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rafael Brandao]

[CoordinatedGraphics] serviceScriptedAnimations expects time in seconds

[Rafael Brandao]

Created attachment 193580 [details]
Patch

[Jesus Sanchez-Palencia]

Comment on attachment 193580 [details]
Patch

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

Is EFL using a different LayerTreeHost? They don't seem to be affected by this. Also, you could add a test here...

> Source/WebKit2/ChangeLog:9
> +        which takes the time delta and multiply for 1000 to get the time in ms.

"multiply by (...)" ?!

[Rafael Brandao]

Created attachment 193805 [details]
Patch

[Rafael Brandao]

(In reply to comment #2)
> (From update of attachment 193580 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193580&action=review
> 
> Is EFL using a different LayerTreeHost? They don't seem to be affected by this. Also, you could add a test here...

They're using the same layer tree host, but EFL use a timer to simulate rAF with USE(REQUEST_ANIMATION_FRAME_TIMER). The code change here is on the code path that doesn't use it. I've added a layout test.

> 
> > Source/WebKit2/ChangeLog:9
> > +        which takes the time delta and multiply for 1000 to get the time in ms.
> 
> "multiply by (...)" ?!

Thanks!

[Rafael Brandao]

Created attachment 193826 [details]
Patch

[Rafael Brandao]

Modified test thanks to kbalazs suggestions. :-)

[Noam Rosenthal]

Comment on attachment 193826 [details]
Patch

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

Need to fix changelog. Fix looks ok to me, please ask a WK2 owner for r.

> Source/WebKit/qt/ChangeLog:9
> +        The tip came from ScriptedAnimationController::serviceScriptedAnimations
> +        which takes the time delta and multiply by 1000 to get the time in ms.

You don't need this in the Changelog.

> Source/WebKit/qt/ChangeLog:11
> +        We've been exposing different time unit for requestAnimationFrame because
> +        of this wrong conversion and web apps could get unexpected results.

... fixed to use seconds instead of milliseconds.

[Rafael Brandao]

Created attachment 193829 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193829 [details]
Patch

Attachment 193829 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17230180

New failing tests:
fast/animation/request-animation-frame-time-unit.html

[Rafael Brandao]

Created attachment 193853 [details]
Patch

I was too otimistic to set the tolerance to only 5ms, I'll try again with 200ms and see what bots think.

[WebKit Review Bot]

Comment on attachment 193853 [details]
Patch

Attachment 193853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17190618

New failing tests:
fast/animation/request-animation-frame-time-unit.html

[Rafael Brandao]

Created attachment 194124 [details]
Patch

[Rafael Brandao]

(In reply to comment #12)
> Created an attachment (id=194124) [details]
> Patch

Thanks to jamesr for the tip, there was a missing testRunner.display() call after the second requestAnimationFrame which avoided it to be executed.

Is there any WK2 owner available for review?

[Benjamin Poulain]

Comment on attachment 194124 [details]
Patch

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

No problem with the change, but the test needs work.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:3
> +var toleranceInMs = 50;

Why does this need to be global?

50 seems huge.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:7
> +var callbackTimeRef = 0;
> +var timeRefInMs = 0;

Ditto.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:18
> +    if (window.testRunner)
> +        testRunner.display();

I am surprised this is inside the callback and not in the test setup.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:26
> +setTimeout(function() {
> +    shouldBeTrue("isTimeUnitInMs");
> +}, 250);

Nope, this should be done from the second callback of window.requestAnimationFrame.
With this, you would introduce a mandatory 250 ms delay for this test, while it could be ~34ms.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:29
> +if (window.testRunner)
> +    testRunner.waitUntilDone();

js-tests have a variable for this, you should not do it manually.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:35
> +setTimeout(function() {
> +    isSuccessfullyParsed();
> +    if (window.testRunner)
> +        testRunner.notifyDone();
> +}, 300);

Wait what?

[Rafael Brandao]

(In reply to comment #14)
> (From update of attachment 194124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194124&action=review
> 
> No problem with the change, but the test needs work.
> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:3
> > +var toleranceInMs = 50;
> 
> Why does this need to be global?
> 
> 50 seems huge.

I'll use 5 instead.

> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:7
> > +var callbackTimeRef = 0;
> > +var timeRefInMs = 0;
> 
> Ditto.

Ok.

> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:18
> > +    if (window.testRunner)
> > +        testRunner.display();
> 
> I am surprised this is inside the callback and not in the test setup.

I was surprised too, but it seems that "display()" is what should move requestAnimationFrame calls. A browser would continuously call display() and then this would not be needed. This is what solved the trick to chromium-linux pass the test.

> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:26
> > +setTimeout(function() {
> > +    shouldBeTrue("isTimeUnitInMs");
> > +}, 250);
> 
> Nope, this should be done from the second callback of window.requestAnimationFrame.
> With this, you would introduce a mandatory 250 ms delay for this test, while it could be ~34ms.

Indeed, excellent idea!

> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:29
> > +if (window.testRunner)
> > +    testRunner.waitUntilDone();
> 
> js-tests have a variable for this, you should not do it manually.

Ok, I'll look into this.

> 
> > LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:35
> > +setTimeout(function() {
> > +    isSuccessfullyParsed();
> > +    if (window.testRunner)
> > +        testRunner.notifyDone();
> > +}, 300);
> 
> Wait what?

Got it.

I've suddenly figured out that nested testRunner.display() with rAF does not work, or it is a problem with notifyDone(). See how it looks like now:

var isTimeUnitInMs = false;

window.requestAnimationFrame(function(time) {
    var callbackTimeRef = time;
    var timeRefInMs = Date.now();
    window.requestAnimationFrame(function(time) {
        var delta = time - callbackTimeRef;
        var deltaTimeInMs = Date.now() - timeRefInMs;
        var toleranceInMs = 5;
        isTimeUnitInMs = Math.abs(delta - deltaTimeInMs) <= toleranceInMs;
        shouldBeTrue("isTimeUnitInMs");
        isSuccessfullyParsed();
        if (window.testRunner)
            testRunner.notifyDone();
    });

    if (window.testRunner)
        testRunner.display();
});

if (window.testRunner)
    testRunner.display();

if (window.testRunner)
    testRunner.waitUntilDone();


When I leave the notifyDone() inside the nested rAF, even chromium stops working and I only get test timeouts. If I move notifyDone to a timeout check like it was before, then it works. Do you know if there's anything obvious that I'm not following in this workflow? Here I'm following up the logic of "testRunner.display()" after each rAF to simulate a paint on the browser and then consequently triggering rAF callback afterwards.

Also ccing jamesr since as he knows a lot about rAF. :-)

[Balazs Kelemen]

In my understanding rAF should make the browser schedule a repaint, so you should not need to use testRunner.display() at all. If that's not the case for chrome or other ports it might be a bug, so you should probably add an expected fail expectation for it. I wonder what do others think.

[James Robinson]

In a browser, yes.  In DumpRenderTree, no.  The repaint tests depend on the test having full control over when frames are produced so we can't schedule frames to happen at other indeterminate times.

[Rafael Brandao]

(In reply to comment #17)
> In a browser, yes.  In DumpRenderTree, no.  The repaint tests depend on the test having full control over when frames are produced so we can't schedule frames to happen at other indeterminate times.

I've reported the trouble with display-driven test to bug 112946, I invite you all to take a look.

Since apparently the solution that would work for everyone is to add a timeout to call notifyDone, I'm willing to do this here for the time being. Benjamin, would you mind with this?

[Rafael Brandao]

Created attachment 194348 [details]
Patch

[Rafael Brandao]

This is the best I could do to make it work everywhere I've tested. It seems that setTimeout drives things through time inside a layout test, but there's certainly something odd on all of this. I've separated in another bug for more investigation.

[Noam Rosenthal]

Comment on attachment 194348 [details]
Patch

LGTM, can one of the WK2 owners sign off?

[Benjamin Poulain]

Comment on attachment 194348 [details]
Patch

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

signed off by me, but I am sad about the test.

> LayoutTests/fast/animation/script-tests/request-animation-frame-time-unit.js:9
> +    // FIXME: We should not need to use any timeout here (bug 112946).

Please remove this. Realistically, most of those tests needs this fixme.

[Rafael Brandao]

Created attachment 196463 [details]
Patch

Not sure if I could land this right away or if I need to get an official r+ first, so I decided to upload it again. Could someone approve it?

[Rafael Brandao]

Committed r148022: <http://trac.webkit.org/changeset/148022>