[Qt][WK2] Regression(141310): Page flickers during scale animation

• Component: New Bugs
• Status: RESOLVED INVALID

[Andras Becsi]

[Qt][WK2] Regression(141310): Page flickers during scale animation

[Andras Becsi]

Created attachment 195117 [details]
Patch

[Noam Rosenthal]

Comment on attachment 195117 [details]
Patch

I'm ok with the coordinated graphics changes. Can you ask Kenneth/Jocelyn to also look before asking for a WK2 owner sign off?

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 195117 [details])
> I'm ok with the coordinated graphics changes. Can you ask Kenneth/Jocelyn to also look before asking for a WK2 owner sign off?

Thanks, Jocelyn could you take a look?

[EFL EWS Bot]

Comment on attachment 195117 [details]
Patch

Attachment 195117 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17236651

[Andras Becsi]

(In reply to comment #4)
> (From update of attachment 195117 [details])
> Attachment 195117 [details] did not pass efl-ews (efl):
> Output: http://webkit-commit-queue.appspot.com/results/17236651

Hmm, EFL seems to depend on the wrong behaviour. I'll have to check what's going on there.

[Andras Becsi]

Created attachment 195282 [details]
Patch

[Andras Becsi]

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

Fixed the EFL build.

[Jocelyn Turcotte]

Comment on attachment 195282 [details]
Patch

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

LGTM but here is something that would be nice to verify twice:

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:140
> +    m_drawingAreaProxy->page()->scalePage(scale, roundedIntPoint(rect.location()));
>      m_drawingAreaProxy->page()->process()->send(Messages::CoordinatedLayerTreeHost::SetVisibleContentsRect(rect, trajectoryVector), m_drawingAreaProxy->page()->pageID());

This might still create the issue that the scale change might trigger a layer flush with a visibleRect's size that is not matching the scale.
For example, if you're zommed out and your page coord visible rect is 500x500 with a scale of 0.5 (a window size of 250x250), zooming in would trigger a tile update using a visibleRect of 125x125 with a scale of 2 (again 250x250).

If the scale change does trigger a layer flush before the matching visibleRect update message is handled, you'll end up rendering a 500x500 rect with a scale of 2: 1000x1000 for the viewport. 2000x2000 if you take into account the TiledBackingStore 2x cover rect multiplier.

Could you verify with some printf in TiledBackingStore::createTiles that the tilesToCreateCount isn't growing astronomically when zooming in? We might not hit that case by luck since a flush is scheduled back on the event loop, giving time to the visible rect update message to be handled, but I'm not sure.

[Jocelyn Turcotte]

The PageViewportController::didChangeContentsVisibility call still in the PageViewportControllerClientEfl::setViewportPosition PVC callback could cause problems with this patch. So hopefully somebody will be testing zooming with EFL.

[Andras Becsi]

(In reply to comment #9)
> The PageViewportController::didChangeContentsVisibility call still in the PageViewportControllerClientEfl::setViewportPosition PVC callback could cause problems with this patch. So hopefully somebody will be testing zooming with EFL.

Qt also has that call indirectlz through the contentX and contentY change notifications, but EFL needs to call it directly. I did not see any issues there but it would be good if someone could verify.

[Andras Becsi]

(In reply to comment #8)
> (From update of attachment 195282 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195282&action=review
> 
> LGTM but here is something that would be nice to verify twice:
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:140
> > +    m_drawingAreaProxy->page()->scalePage(scale, roundedIntPoint(rect.location()));
> >      m_drawingAreaProxy->page()->process()->send(Messages::CoordinatedLayerTreeHost::SetVisibleContentsRect(rect, trajectoryVector), m_drawingAreaProxy->page()->pageID());
> 
> This might still create the issue that the scale change might trigger a layer flush with a visibleRect's size that is not matching the scale.
> For example, if you're zommed out and your page coord visible rect is 500x500 with a scale of 0.5 (a window size of 250x250), zooming in would trigger a tile update using a visibleRect of 125x125 with a scale of 2 (again 250x250).
> 
> If the scale change does trigger a layer flush before the matching visibleRect update message is handled, you'll end up rendering a 500x500 rect with a scale of 2: 1000x1000 for the viewport. 2000x2000 if you take into account the TiledBackingStore 2x cover rect multiplier.
> 
> Could you verify with some printf in TiledBackingStore::createTiles that the tilesToCreateCount isn't growing astronomically when zooming in? We might not hit that case by luck since a flush is scheduled back on the event loop, giving time to the visible rect update message to be handled, but I'm not sure.

I couldn't produce such a scenario. The tile creation always started after the visibleRect update was processed but never in between the scale setting and the visible rect update.

I do not have a concept of astronomical in this context but the biggest number of tilesToCreateCount in TiledBackingStore::createTiles was 10 during double-tap zoom-in.

[Jocelyn Turcotte]

Comment on attachment 195282 [details]
Patch

LGTM for Qt, I'm still not sure for EFL so I think it would be best to get their blessing too.

[Jocelyn Turcotte]

(In reply to comment #11)
> I couldn't produce such a scenario. The tile creation always started after the visibleRect update was processed but never in between the scale setting and the visible rect update.
> 
> I do not have a concept of astronomical in this context but the biggest number of tilesToCreateCount in TiledBackingStore::createTiles was 10 during double-tap zoom-in.

Ok in any case the patch is an improvement in that regard, thanks for checking.

[Andras Becsi]

(In reply to comment #12)
> (From update of attachment 195282 [details])
> LGTM for Qt, I'm still not sure for EFL so I think it would be best to get their blessing too.

Kenneth, could you check this for EFL with one of your magic manual tests?
I ran a quick test with an EFL build and did not see suspicious behaviour.

[Andras Becsi]

Ping for review?

[Andras Becsi]

Created attachment 196645 [details]
Patch

[Andras Becsi]

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

Updated to apply on ToT.

[Mikhail Pozdnyakov]

Comment on attachment 196645 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:45
> +    , m_lastSentScale(0)

ah another scale :( but isn't it the same as pagescalefactor from page proxy?

[Andras Becsi]

Comment on attachment 196645 [details]
Patch

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

>> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:45
>> +    , m_lastSentScale(0)
> 
> ah another scale :( but isn't it the same as pagescalefactor from page proxy?

It is technically the same scale factor, but the represented state is different.

In theory I could put a scale and scroll position equality check to WebPageProxy::scalePage, but WebPageProxy::m_pageScaleFactor has to be initialized to 1, since it is exposed, so an initial scale setting of 1 would not reach the web process causing behavior change and potential problems.

I also think that the name makes the state this variable represents clear, but I could rename it to something like m_lastCommittedPageScaleFactor, if you think it confusing.

[Andras Becsi]

Created attachment 197573 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 197573 [details]
Patch

r=me, just make sure you get an WK2 owner to look at it. (CC'd Benjamin)

[Benjamin Poulain]

Comment on attachment 197573 [details]
Patch

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

I have two little issues with this patch:

I think the method is badly named.
Something named "setVisibleContentsRect" should take a rect, not a rect, a scale and a "trajectoryVector" (I assume normalized). You should either split it in three method calls, or look for a more descriptive name.

The scale of 1 passed for the page scale factor is really weird. I don't really understand why that would be correct. For example, a page has a scale of 0.3, the user resize the view, shouldn't the scale be re-computed for the viewport parameters?

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:102
> +    float m_lastSentPageScaleFactor;

float->double (the page scale factor is typed double).

[Andras Becsi]

(In reply to comment #22)
> (From update of attachment 197573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197573&action=review
> 
> I have two little issues with this patch:
> 
> I think the method is badly named.
> Something named "setVisibleContentsRect" should take a rect, not a rect, a scale and a "trajectoryVector" (I assume normalized). 

It is not normalized at this point yet. Normalization happens in TiledBackingStore::setTrajectoryVector.

> You should either split it in three method calls, or look for a more descriptive name.

I'll try to come up with a more descriptive name.

> 
> The scale of 1 passed for the page scale factor is really weird. I don't really understand why that would be correct. For example, a page has a scale of 0.3, the user resize the view, shouldn't the scale be re-computed for the viewport parameters?
> 

We only set the scale to 1 for the "legacy" webview (no fixed layout, i.e. traditional desktop behaviour as well as the QRawWebView).
For the QML WebView (with fixed layout) we recalculate each time something changes (see PageViewportController::syncVisibleContents).

> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:102
> > +    float m_lastSentPageScaleFactor;
> 
> float->double (the page scale factor is typed double).

Thanks for catching this.

[Andras Becsi]

Created attachment 199021 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 199021 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:131
> +void CoordinatedLayerTreeHostProxy::updateVisibleContentsState(const FloatRect& visibleContetsRect, double pageScaleFactor, const FloatPoint& trajectoryVector)

visibleConte*n*tsRect
You could also name the method updateContentsVisibility to match PageViewportController::didChangeContentsVisibility

[Andras Becsi]

Created attachment 200707 [details]
Patch

[EFL EWS Bot]

Comment on attachment 200707 [details]
Patch

Attachment 200707 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/431034

[Andras Becsi]

Created attachment 200710 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 200710 [details]
Patch

lgtm.
Benjamin, could you check again?

[Andras Becsi]

Created attachment 203331 [details]
Patch

[Andras Becsi]

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

Updated to ToT.

[EFL EWS Bot]

Comment on attachment 203331 [details]
Patch

Attachment 203331 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/673339

[Andras Becsi]

Created attachment 203336 [details]
Patch

[Anders Carlsson]

Comment on attachment 203336 [details]
Patch

Qt has been removed, clearing review flags.

[Allan Sandfeld Jensen]

I don't see the flicker currently in qt5x2 branch. Is this patch still needed and should we apply it to branch?

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.