Bug 95992 - [Qt] Implement pixel snapshot generation in WTR
Summary: [Qt] Implement pixel snapshot generation in WTR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
: 90394 (view as bug list)
Depends on: 98654 99354
Blocks: 94800 99463
  Show dependency treegraph
 
Reported: 2012-09-06 09:48 PDT by Balazs Kelemen
Modified: 2012-11-21 11:31 PST (History)
11 users (show)

See Also:


Attachments
Patch (20.88 KB, patch)
2012-09-06 09:55 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2012-09-10 08:02 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (19.13 KB, patch)
2012-09-24 06:31 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (19.14 KB, patch)
2012-09-24 23:42 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (4.68 KB, patch)
2012-10-08 10:00 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
reupload for ews ouput (4.60 KB, patch)
2012-10-12 01:47 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2012-10-15 00:13 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
use grabWindow directly (2.97 KB, patch)
2012-10-15 03:57 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-09-06 09:48:44 PDT
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.
Comment 1 Balazs Kelemen 2012-09-06 09:55:50 PDT
Created attachment 162529 [details]
Patch
Comment 2 Jocelyn Turcotte 2012-09-06 10:15:22 PDT
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.
Comment 3 Balazs Kelemen 2012-09-06 10:21:42 PDT
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.
Comment 4 Balazs Kelemen 2012-09-06 10:26:22 PDT
(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.
Comment 5 Balazs Kelemen 2012-09-07 09:21:38 PDT
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.
Comment 6 Balazs Kelemen 2012-09-10 07:17:30 PDT
Draft patches for screnegraph rendering:
qt -> https://gist.github.com/3691144
webkit (upon the above) -> https://gist.github.com/3691147
Comment 7 Balazs Kelemen 2012-09-10 08:02:50 PDT
Created attachment 163132 [details]
Patch
Comment 8 Balazs Kelemen 2012-09-10 08:03:54 PDT
(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).
Comment 9 Balazs Kelemen 2012-09-10 08:22:18 PDT
(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.
Comment 10 Balazs Kelemen 2012-09-18 04:32:45 PDT
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?
Comment 11 Jocelyn Turcotte 2012-09-18 07:59:41 PDT
(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.
Comment 12 Noam Rosenthal 2012-09-18 08:06:04 PDT
2c is the best if it's achievable without tampering with QScenegraph in a disruptive way.
Comment 13 Balazs Kelemen 2012-09-24 05:04:00 PDT
Here is the change for Qt: https://codereview.qt-project.org/#change,35490
Comment 14 Balazs Kelemen 2012-09-24 06:31:13 PDT
Created attachment 165367 [details]
Patch
Comment 15 Balazs Kelemen 2012-09-24 06:35:28 PDT
(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).
Comment 16 Early Warning System Bot 2012-09-24 15:01:16 PDT
Comment on attachment 165367 [details]
Patch

Attachment 165367 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13987807
Comment 17 Balazs Kelemen 2012-09-24 23:42:27 PDT
Created attachment 165534 [details]
Patch

buildfix
Comment 18 Balazs Kelemen 2012-10-08 04:12:24 PDT
The new Qt5 snapshot contains the change which is the dependency of this patch, so we can go forward with this. :)
Comment 19 Balazs Kelemen 2012-10-08 09:15:28 PDT
Comment on attachment 165534 [details]
Patch

I'm going to split the two parts of this patch as Jocelyn suggested. First part: bug 98654
Comment 20 Balazs Kelemen 2012-10-08 10:00:46 PDT
Created attachment 167553 [details]
Patch
Comment 21 Balazs Kelemen 2012-10-12 01:47:22 PDT
Created attachment 168378 [details]
reupload for ews ouput
Comment 22 Jocelyn Turcotte 2012-10-12 06:13:49 PDT
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?
Comment 23 Balazs Kelemen 2012-10-15 00:12:24 PDT
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.
Comment 24 Balazs Kelemen 2012-10-15 00:13:14 PDT
Created attachment 168635 [details]
Patch
Comment 25 Simon Hausmann 2012-10-15 00:30:51 PDT
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?
Comment 26 Balazs Kelemen 2012-10-15 03:57:37 PDT
Created attachment 168671 [details]
use grabWindow directly

Note that it won't build on ews's because of the depenedency.
Comment 27 Jocelyn Turcotte 2012-10-15 04:11:05 PDT
Comment on attachment 168671 [details]
use grabWindow directly

Cool, that looks neat now :)
Comment 28 Early Warning System Bot 2012-10-15 04:11:11 PDT
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
Comment 29 Balazs Kelemen 2012-10-15 10:17:01 PDT
Landed in http://trac.webkit.org/changeset/131307
Comment 30 Csaba Osztrogonác 2012-10-15 12:57:17 PDT
(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.
Comment 32 Balazs Kelemen 2012-10-16 01:13:58 PDT
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.
Comment 33 Csaba Osztrogonác 2012-10-16 01:18:45 PDT
(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.
Comment 34 Balazs Kelemen 2012-10-16 02:16:30 PDT
(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).
Comment 35 Balazs Kelemen 2012-10-18 08:16:04 PDT
Landed again in http://trac.webkit.org/changeset/131726
Comment 36 Balazs Kelemen 2012-11-21 11:31:10 PST
*** Bug 90394 has been marked as a duplicate of this bug. ***