Bug 95992 – [Qt] Implement pixel snapshot generation in WTR

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

The goal is to make pixel tests test the code path that we use for the real rendering (i.e. in MiniBrowser or snowshoe). This is a side track from bug 90394. Currently the GL rendering has some problem and we cannot switch to it yet. Also QRawWebView lucks a few features (popup menu, editing support, etc.) so switching to it would reduce our coverage. Given these I would first implement the snapshot generation upon QQuickWebView in the software rendering path, as an intermediate step.

[Balazs Kelemen]

Created attachment 162529 [details]
Patch

[Jocelyn Turcotte]

What would be the best way to focus efforts for those tests? It would be nice to have only one way of doing this and avoid having 3 flaky test runners to maintain instead of a solid one.

If you think that we can get GL rendered pixel tests with QQuickWebView then I think that we should go that way straight and skip the software rendering one.
If you think we can't then I think we should focus testing efforts on QRawWebView, fixing the flakiness and enabling features that we want coverage on.

If you would have to chose one way to do it, which one would it be?
After all, layout tests are mostly there to test cross platform WebCore and WebKit2 code.

[Balazs Kelemen]

We cannot get GL rendered pixels currently because there is some bug in textmap. But if we fix this we could do it with qquickwebview. In fact I also made a patch for this but it has the same issue that the one based on QRawWebView. So at the end I am not sure we should switch to QRawWebView at all.

[Balazs Kelemen]

(In reply to comment #3)
> We cannot get GL rendered pixels currently because there is some bug in textmap. But if we fix this we could do it with qquickwebview. In fact I also made a patch for this but it has the same issue that the one based on QRawWebView. So at the end I am not sure we should switch to QRawWebView at all.

To make it clear technically: binding a custom qglcontext to our QQuickView and grabbing pixels does work. I guess the reason why it does not mess up with the context it get from the scenegraph is that we don't actually show our window.

[Balazs Kelemen]

As discussed on IRC we can probably want to go through the scenegraph via grabWindow() so we can actually cover the code paths we use normally. For this I made up a patch that basically works locally but I'm not sure the change I had to made in Qt is acceptable. I'm going to find it out next week.

[Balazs Kelemen]

Draft patches for screnegraph rendering:
qt -> https://gist.github.com/3691144
webkit (upon the above) -> https://gist.github.com/3691147

[Balazs Kelemen]

Created attachment 163132 [details]
Patch

[Balazs Kelemen]

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

I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe).

[Balazs Kelemen]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=163132) [details] [details]
> > Patch
> 
> I added a timer for this because otherwise we won't release memory if we become inactive (ui proc stop sending rendernextframe).

Err, sorry, wrong bug.

[Balazs Kelemen]

I would like to get some input about how to go forward with this.
So, here is the choices:
1. rendering path
   a/ software
   b/ gl

This is not really a question, since it's clear we want to test the gl path but as long as it is not stable enough for running a whole test session we can use software rendering as a temporary solution.

2. method
  a/ QRawWebView - like the patch in bug 90394
  b/ QQuickWebView without scenegraph, i.e. calling LayerTreeRenderer::paintToCurrentGLContext manually from viewNeedsDisplay, this has been implemented in the patch here
  c/ QQUickWebView with scenegraph, via QQuickWindow::grabWindow

I don't really see what is the advance of QRWV, we can use the same method to grab pixels with QQWV as well, plus the latter has some features already implemented that is needed for some tests (popups, editing support, etc.). Making it go through the scenegraph is a bit tricky, and need some changes in Qt. I'm not sure we can push these changes to Qt. Basically the change is to allow grabbing pixels even if there no showed window at all.

My opinion is that the best for now is to go with 1.a + 2.b. Later we can investigate in moving to gl and using grabWindow.

Noam? Simon? Others?

[Jocelyn Turcotte]

(In reply to comment #10)

My opinion on this is that 2.b isn't worth it, since it relies on an half-using QtQuick and this _will_ present problems. To me it feels like a hack, can break sooner or later and may prevent us from re-architecting the internals.
So I would ultimately go with 2.c if we could get grabWindow work with a hidden window, or 2.a if not possible.
I'm not worried about pushing changes to Qt, if it makes sense we have the people we need to make it happen.

As for using software rendering meanwhile I personally don't see an advantage to it, it will produce slightly different results that we'll have to rebase later, will leave dead code that we won't dare removing, and making the GL path work shouldn't be that complicated if we put the efforts to it. This is just an opinion though.

[Noam Rosenthal]

2c is the best if it's achievable without tampering with QScenegraph in a disruptive way.

[Balazs Kelemen]

Here is the change for Qt: https://codereview.qt-project.org/#change,35490

[Balazs Kelemen]

Created attachment 165367 [details]
Patch

[Balazs Kelemen]

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

... and here is the WebKit part. Fortunately this seems to be relatively stable (no every test fail from a point effect).

[Early Warning System Bot]

Comment on attachment 165367 [details]
Patch

Attachment 165367 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13987807

[Balazs Kelemen]

Created attachment 165534 [details]
Patch

buildfix

[Balazs Kelemen]

The new Qt5 snapshot contains the change which is the dependency of this patch, so we can go forward with this. :)

[Balazs Kelemen]

Comment on attachment 165534 [details]
Patch

I'm going to split the two parts of this patch as Jocelyn suggested. First part: bug 98654

[Balazs Kelemen]

Created attachment 167553 [details]
Patch

[Balazs Kelemen]

Created attachment 168378 [details]
reupload for ews ouput

[Jocelyn Turcotte]

Comment on attachment 168378 [details]
reupload for ews ouput

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

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:36
> +#include <QtGui/qopenglframebufferobject.h>

QOpenGLFramebufferObject or QtGui/QOpenGLFramebufferObject should work

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:53
> +        connect(this, SIGNAL(beforeRendering()), SLOT(onBeforeRendering()), Qt::DirectConnection);

I believe there is no need to specify that it's a direct connection if both objects are on the same thread.

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:59
> +        return m_fbo ? m_fbo->toImage() : QImage();

It should never be null here no?

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:74
> +        setSurfaceType(OpenGLSurface);
> +        create();

My feeling would be that it's better to create as late as possible, at least after setGeometry. Is there a reason to do so that early?

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:100
> +        m_fbo->release();

Are you sure you need to release if you're binding right after?

[Balazs Kelemen]

It has been turned out that we don't really need to use an fbo, in contrast with the comment at setRenderWithoutShowing. I asked Gunnar about it and he agreed with that, mentioned that the comment is outdated. This means the next patch will be quite trivial.

[Balazs Kelemen]

Created attachment 168635 [details]
Patch

[Simon Hausmann]

Comment on attachment 168635 [details]
Patch

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

Looks great :) But I'll let Jocelyn do the final call.

> Tools/WebKitTestRunner/qt/PlatformWebViewQt.cpp:56
> +    QImage snapshotImage()
> +    {
> +        return grabWindow();
> +    }

Any particular reason for this method? Why not let the caller use grabWindow() directly?

[Balazs Kelemen]

Created attachment 168671 [details]
use grabWindow directly

Note that it won't build on ews's because of the depenedency.

[Jocelyn Turcotte]

Comment on attachment 168671 [details]
use grabWindow directly

Cool, that looks neat now :)

[Early Warning System Bot]

Comment on attachment 168671 [details]
use grabWindow directly

Attachment 168671 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14295599

[Balazs Kelemen]

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

[Csaba Osztrogonác]

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

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]

I guess that's happening because this way of pixel generating does not work in xvfb which is revealed by ref tests. I think we need some environment variable quirk to be able to enable it only when running in a suitable environment.

[Csaba Osztrogonác]

(In reply to comment #32)
> I guess that's happening because this way of pixel generating does not work in xvfb which is revealed by ref tests. I think we need some environment variable quirk to be able to enable it only when running in a suitable environment.

Not only reftest became slow, but all tests.

[Balazs Kelemen]

(In reply to comment #33)
> (In reply to comment #32)
> > I guess that's happening because this way of pixel generating does not work in xvfb which is revealed by ref tests. I think we need some environment variable quirk to be able to enable it only when running in a suitable environment.
> 
> Not only reftest became slow, but all tests.

Strange. I just run the tests at the revision of the second patch, it took 30 minutes although I had a lot of failures and timeouts. Btw, locally if I run the tests in xvfb than pixel and ref tests are crashing (ref tests without -p as well of course).

[Balazs Kelemen]

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

[Balazs Kelemen]

*** Bug 90394 has been marked as a duplicate of this bug. ***