Bug 90641 – [Qt] QRawWebView should notify when rendering is done, so that pixel results can be grabbed at the appropriate moment.

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

[Noam Rosenthal]

[Qt] QRawWebView should notify when rendering is done, so that pixel results can be grabbed at the appropriate moment.

[Noam Rosenthal]

Created attachment 150996 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Luiz Agostini]

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?

[Noam Rosenthal]

(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.

[Noam Rosenthal]

(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.

[Jocelyn Turcotte]

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)
>  {

`

[Balazs Kelemen]

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?

[Noam Rosenthal]

(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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Comment on attachment 150996 [details]
Patch

Clearing flags, working on a new patch.

[Balazs Kelemen]

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).

[Noam Rosenthal]

Created attachment 151164 [details]
Patch

[Noam Rosenthal]

Comment on attachment 151164 [details]
Patch

There's a problem with this patch, clearing flags for now.

[Noam Rosenthal]

Created attachment 151277 [details]
Patch

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 151490 [details]
Patch

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 151516 [details]
Patch

[Jocelyn Turcotte]

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.

[Noam Rosenthal]

Created attachment 151724 [details]
Patch for landing

[Noam Rosenthal]

Created attachment 151726 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151726 [details]
Patch for landing

Clearing flags on attachment: 151726

Committed r122340: <http://trac.webkit.org/changeset/122340>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.