Bug 98654 – [Qt][WTR] Do a forced repaint before generating pixel results

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

The CoordinatedGraphics rendering backend needs an extra step of synchronisation in order to generate reliable pixel snapshots. Currently pixel results are generated at the end of the test run synchronously so we should slightly change WTR in order to allow delaying the dump after WKPageForceRepaint call us back.

[Balazs Kelemen]

Created attachment 167549 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 167549 [details]
Patch

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

> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:117
> +    if (!s_watchDog)
> +        s_watchDog = new ForceRepaintDoneWatchDog(this);
> +    s_watchDog->start();

It feels like you're basically implementing what runUntil already does.
How about adding the extra async step to the chain of steps that we have in TestInvocation::invoke, keep textOutput, pixelResult and repaintRects in member variables, and use runUntil there to wait until the platform code obtained the dumped pixels, and then dump the results?

That would also prevent other platforms that want to use pixel tests with coordinated graphics classes to have to re-implement all this in their TestController.

[Balazs Kelemen]

Created attachment 167777 [details]
Patch

[Balazs Kelemen]

Created attachment 168028 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 168028 [details]
Patch

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

> Tools/WebKitTestRunner/TestInvocation.h:57
> +    // Some platforms want to force a repaint before generating pixel result.
> +    static void forceRepaintDoneCallback(WKErrorRef, void* context)
> +    {
> +        reinterpret_cast<TestInvocation*>(context)->forceRepaintDone();
> +    }

This can go in the .cpp

> Tools/WebKitTestRunner/TestInvocation.h:58
> +    void forceRepaintDone();

Maybe put this under #if PLATFORM(QT)?

> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:78
> +    TestController::shared().runUntil(m_gotRepaint, TestController::ShortTimeout);

This is using the same instance of TestController and are running a runUntil on top of another runUntil, including the full stack up to didReceiveMessageFromInjectedBundle.
If it really is working well this might be fine since it's simpler for now, but if it starts smelling bad we could pick parts of your last patch to have this runUntil running after the previous one is finished.

> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:81
> +    if (m_gotRepaint) {

In which cases is this false, can't we just assert?

[Balazs Kelemen]

> This is using the same instance of TestController and are running a runUntil on top of another runUntil, including the full stack up to didReceiveMessageFromInjectedBundle.
> If it really is working well this might be fine since it's simpler for now, but if it starts smelling bad we could pick parts of your last patch to have this runUntil running after the previous one is finished.

Ok, I'm going to check how it affects performance and whether a nested runLoop could work for all platforms.

> 
> > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:81
> > +    if (m_gotRepaint) {
> 
> In which cases is this false, can't we just assert?

It is false if smg is wrong with the synchronisation. Given that it is a bit fragile according to our experiences in connection this work, and that we want to run this on bots I believe it's better to handle this case gracefully.

[Balazs Kelemen]

Created attachment 168202 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 168202 [details]
Patch

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

> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:70
> +    reinterpret_cast<TestInvocation*>(context)->m_gotRepaint = true;

static_cast

> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:90
> +    } else {
> +        // The test harness expects an image so we output an empty one.
> +        WKRect windowRect = TestController::shared().mainWebView()->windowFrame();
> +        image = QImage(QSize(windowRect.size.width, windowRect.size.height), QImage::Format_ARGB32_Premultiplied);
> +        image.fill(Qt::black);
> +    }

That case is pretty silent, we could try to make it obvious from the test results by printing it red. We could also print an error if you end up delaying the stderr #EOF like you proposed.

[Balazs Kelemen]

Created attachment 168235 [details]
Patch

[Balazs Kelemen]

> > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:70
> > +    reinterpret_cast<TestInvocation*>(context)->m_gotRepaint = true;
> 
> static_cast

Fixed.

> 
> > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:90
> > +    } else {
> > +        // The test harness expects an image so we output an empty one.
> > +        WKRect windowRect = TestController::shared().mainWebView()->windowFrame();
> > +        image = QImage(QSize(windowRect.size.width, windowRect.size.height), QImage::Format_ARGB32_Premultiplied);
> > +        image.fill(Qt::black);
> > +    }
> 
> That case is pretty silent, we could try to make it obvious from the test results by printing it red. We could also print an error if you end up delaying the stderr #EOF like you proposed.

Ok, changed it to red. Later we can add the error message.

[Balazs Kelemen]

Comment on attachment 168235 [details]
Patch

Clearing flags on attachment: 168235

Committed r131160: <http://trac.webkit.org/changeset/131160>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #11)
> (From update of attachment 168235 [details])
> Clearing flags on attachment: 168235
> 
> Committed r131160: <http://trac.webkit.org/changeset/131160>

It made layout testing extremely slow - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/9671/steps/layout-test

2.5 hours - 9800 tests :((((

[WebKit Review Bot]

Re-opened since this is blocked by bug 99163

[Balazs Kelemen]

(In reply to comment #14)
> Re-opened since this is blocked by bug 99163

Sorry, I realized that it should not be landed without the patch in bug 95992 because it makes pixel and ref tests time out.

[Balazs Kelemen]

Let's do one more iteration as I realized that I am leaking memory.

[Balazs Kelemen]

Created attachment 168433 [details]
Patch

Fixed memory leak by correctly adopting the snapshot image.

[Balazs Kelemen]

Landed in http://trac.webkit.org/changeset/131306

[WebKit Review Bot]

Re-opened since this is blocked by bug 99354

[Csaba Osztrogonác]

(In reply to comment #18)
> Landed in http://trac.webkit.org/changeset/131306

Rolled out, because it broke layout testing again - http://trac.webkit.org/changeset/131339

I stopped the test after 3 hours 8 minutes (after an editing 
test). Normally all tests take only 25 minutes.

[Csaba Osztrogonác]

See http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/9742 for details.

[Balazs Kelemen]

Landed again in http://trac.webkit.org/changeset/131725