Bug 90641 - [Qt] QRawWebView should notify when rendering is done, so that pixel results can be grabbed at the appropriate moment.
Summary: [Qt] QRawWebView should notify when rendering is done, so that pixel results ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on:
Blocks: 90394
  Show dependency treegraph
 
Reported: 2012-07-05 15:59 PDT by Noam Rosenthal
Modified: 2012-07-11 10:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.74 KB, patch)
2012-07-05 16:08 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2012-07-08 12:36 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2012-07-09 10:53 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (11.36 KB, patch)
2012-07-10 11:14 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (11.48 KB, patch)
2012-07-10 13:53 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (11.42 KB, patch)
2012-07-11 10:10 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (11.43 KB, patch)
2012-07-11 10:13 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-07-05 15:59:50 PDT
[Qt] QRawWebView should notify when rendering is done, so that pixel results can be grabbed at the appropriate moment.
Comment 1 Noam Rosenthal 2012-07-05 16:08:45 PDT
Created attachment 150996 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-05 18:39:24 PDT
Comment on attachment 150996 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:284
> +}
> +
> +

one too many newlines

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview.cpp:407
> +    m_client->viewRenderedFrame();

view-rendered frame? maybe viewDidRenderFrame is more descriptive

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p.h:57
> +    virtual void viewNeedsDisplay(const QRect&) { }
> +    virtual void viewRequestedScroll(const QPoint&) { }
> +    virtual void viewProcessCrashed() { }
> +    virtual void viewRenderedFrame() { }
> +    virtual void viewProcessRelaunched() { }
> +    virtual void viewContentSizeChanged(const QSize&) { }

Ah I see you are using it [view] as a namespace, hmm

> Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:128
> +    QCryptographicHash hash(QCryptographicHash::Md5);
> +    for (unsigned row = 0; row < image.height(); ++row)
> +        hash.addData(reinterpret_cast<const char*>(image.constScanLine(row)), image.bytesPerLine());

Good idea, but what about fuzzyness? These have to be strictly the same?
Comment 3 Luiz Agostini 2012-07-05 19:27:13 PDT
Comment on attachment 150996 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p_p.h:34
> +class QRawWebViewPrivate : public WebKit::PageClient, public virtual WebKit::LayerTreeCoordinatorProxy::Observer {

Why to use virtual inheritance?
Comment 4 Noam Rosenthal 2012-07-05 22:17:17 PDT
(In reply to comment #3)
> (From update of attachment 150996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/raw/qrawwebview_p_p.h:34
> > +class QRawWebViewPrivate : public WebKit::PageClient, public virtual WebKit::LayerTreeCoordinatorProxy::Observer {
> 
> Why to use virtual inheritance?

Old habit when i use multiple inheritance... doesn't really matter.
Comment 5 Noam Rosenthal 2012-07-05 22:17:59 PDT
(In reply to comment #2)
> > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:128
> > +    QCryptographicHash hash(QCryptographicHash::Md5);
> > +    for (unsigned row = 0; row < image.height(); ++row)
> > +        hash.addData(reinterpret_cast<const char*>(image.constScanLine(row)), image.bytesPerLine());
> 
> Good idea, but what about fuzzyness? These have to be strictly the same?

I think maybe I'll leave this to another patch. We need a better way to test this.
Comment 6 Jocelyn Turcotte 2012-07-06 01:22:16 PDT
Comment on attachment 150996 [details]
Patch

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

> Source/WebKit2/ChangeLog:11
> +        Allow QRawWebViewClient to receive a notification when rendering is done (DidRenderFrame),
> +        and allow QRawWebView to send a message to the web process to render a new frame
> +        programmatically, by exposing LayerTreeCoordinator::scheduleLayerFlush via a WebKit2
> +        message.

You might have issues that scheduleLayerFlush will not always return you a DidRenderFrame, i.e. if any of the early returns are hit in LayerTreeCoordinator::performScheduledLayerFlush.
If m_isSuspended, m_waitingForUIProcess are true, or if there was nothing dirty and m_shouldSyncFrame wasn't set to true.
It feels that if notifyDone is called inside a timer where no document modification was done, you could still wait infinitely while grabbing.

> Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:119
>  void WebView::finishLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void *clientInfo)
>  {

`
Comment 7 Balazs Kelemen 2012-07-06 02:47:51 PDT
Comment on attachment 150996 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:100
> +    virtual void viewRenderedFrame()
> +    {
>          if (m_frameLoaded)
>              emit loaded();
>      }

Shouldn't we actually paint here?
Comment 8 Noam Rosenthal 2012-07-06 06:59:07 PDT
(In reply to comment #6)
> (From update of attachment 150996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        Allow QRawWebViewClient to receive a notification when rendering is done (DidRenderFrame),
> > +        and allow QRawWebView to send a message to the web process to render a new frame
> > +        programmatically, by exposing LayerTreeCoordinator::scheduleLayerFlush via a WebKit2
> > +        message.
> 
> You might have issues that scheduleLayerFlush will not always return you a DidRenderFrame, i.e. if any of the early returns are hit in LayerTreeCoordinator::performScheduledLayerFlush.
> If m_isSuspended, m_waitingForUIProcess are true, or if there was nothing dirty and m_shouldSyncFrame wasn't set to true.
You're right, I need to send a NothingToRender in the case when m_shouldSyncFrame is false. m_waitingForUIProcess is not a problem because another flush would be trigger in the next RenderNextFrame call, and m_isSuspended should not be on during a grab - I think it's safe to wait to the next DidRenderFrame that would happen after the next resume.

> It feels that if notifyDone is called inside a timer where no document modification was done, you could still wait infinitely while grabbing.
> 
Right, I still need the NothingToRender event or something similar.
Comment 9 Noam Rosenthal 2012-07-06 06:59:24 PDT
(In reply to comment #7)
> (From update of attachment 150996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150996&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/tests/qrawwebview/tst_qrawwebview.cpp:100
> > +    virtual void viewRenderedFrame()
> > +    {
> >          if (m_frameLoaded)
> >              emit loaded();
> >      }
> 
> Shouldn't we actually paint here?

No need, since we already paint on viewNeedsDisplay.
Comment 10 Noam Rosenthal 2012-07-06 07:20:22 PDT
Comment on attachment 150996 [details]
Patch

Clearing flags, working on a new patch.
Comment 11 Balazs Kelemen 2012-07-06 09:30:23 PDT
Just for the record: I tried but could not get reliable pixel results with the patch. I tried it like this: https://gist.github.com/3061148. It was working for a few basic tests when I called wtr from the command line, also with old-run-webkit-tests, but not with new-run-webkit-tests. Even by programatically saving the pixel result to file the results in nrwt was different (empty blackness). I could not find anything about nrwt that could cause this, so I guess the method provided by the patch here is still not very reliable (but I'm still very puzzled about the fact that it works with one test harness and not with another).
Comment 12 Noam Rosenthal 2012-07-08 12:36:54 PDT
Created attachment 151164 [details]
Patch
Comment 13 Noam Rosenthal 2012-07-09 08:01:41 PDT
Comment on attachment 151164 [details]
Patch

There's a problem with this patch, clearing flags for now.
Comment 14 Noam Rosenthal 2012-07-09 10:53:47 PDT
Created attachment 151277 [details]
Patch
Comment 15 Jocelyn Turcotte 2012-07-10 03:24:40 PDT
Comment on attachment 151277 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:178
>      scheduleLayerFlush();
> +    flushPendingLayerChanges();

Why both?

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:242
> +    didSync = didSync || m_shouldSyncFrame;

Is it a good idea to mix those two? syncCompositingStateIncludingSubframes can return true even if no modification was made to layers I think, and this would send DidRenderFrame messages unnecessarily.

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:136
> +    bool m_isPerformingLayerFlush;

Unused.
Comment 16 Noam Rosenthal 2012-07-10 06:07:12 PDT
(In reply to comment #15)
> (From update of attachment 151277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151277&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:178
> >      scheduleLayerFlush();
> > +    flushPendingLayerChanges();
> 
> Why both?
I'm not sure. This code is copied from GraphicsLayerCA.

> > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:242
> > +    didSync = didSync || m_shouldSyncFrame;
> 
> Is it a good idea to mix those two? syncCompositingStateIncludingSubframes can return true even if no modification was made to layers I think, and this would send DidRenderFrame messages unnecessarily.

You're right. I'll fix this.
Comment 17 Noam Rosenthal 2012-07-10 11:14:20 PDT
Created attachment 151490 [details]
Patch
Comment 18 Jocelyn Turcotte 2012-07-10 12:05:41 PDT
Comment on attachment 151490 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238
> +    m_shouldSyncFrame = false;

I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer.

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:258
> +    m_waitingForUIProcess = true;

m_waitingForUIProcess state changes have to be in sync with DidRenderFrame messages, it would be clearer to update it within the same scope.
Comment 19 Noam Rosenthal 2012-07-10 13:46:09 PDT
(In reply to comment #18)
> (From update of attachment 151490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151490&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238
> > +    m_shouldSyncFrame = false;
> 
> I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer.
>
We set m_shouldSyncFrame for every tile update. That hasn't changed in this patch, or I don't understand your comment.
Comment 20 Noam Rosenthal 2012-07-10 13:50:21 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 151490 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151490&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:238
> > > +    m_shouldSyncFrame = false;
> > 
> > I don't think that m_shouldSyncFrame should now be checked only for non-composited contents. This will have the effect of not sending a DidRenderFrame message if a tile was updated only in a composited layer.
> >
> We set m_shouldSyncFrame for every tile update. That hasn't changed in this patch, or I don't understand your comment.

Oh, I see. There's a bug there.
Comment 21 Noam Rosenthal 2012-07-10 13:53:48 PDT
Created attachment 151516 [details]
Patch
Comment 22 Jocelyn Turcotte 2012-07-11 02:03:15 PDT
Comment on attachment 151516 [details]
Patch

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

r=me with one readability issue.

> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:255
> +    if (!didSync)
> +        return false;
> +
> +    if (!m_shouldSyncFrame)
> +        return true;

This part hardly makes sense if you just look at the variable names.
I think it would read better as:
if (!m_shouldSyncFrame)
    return didSync;

And at the end, return didSync; instead of true (or remove the "didSync = didSync || m_shouldSyncFrame;" line and keep the return true).

Else it's impossible to see that m_shouldSyncFrame is related to the DidRenderFrame message.
You could also rename m_shouldSyncFrame to m_shouldSendDidRenderFrame or something like this.
Comment 23 Noam Rosenthal 2012-07-11 10:10:20 PDT
Created attachment 151724 [details]
Patch for landing
Comment 24 Noam Rosenthal 2012-07-11 10:13:52 PDT
Created attachment 151726 [details]
Patch for landing
Comment 25 WebKit Review Bot 2012-07-11 10:50:22 PDT
Comment on attachment 151726 [details]
Patch for landing

Clearing flags on attachment: 151726

Committed r122340: <http://trac.webkit.org/changeset/122340>
Comment 26 WebKit Review Bot 2012-07-11 10:50:29 PDT
All reviewed patches have been landed.  Closing bug.