Bug 75780 – [Qt][WK2] Accelerated and non-accelerated animations need to be synchronized

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

In order to avoid an overload of IPC messages during each animation frame, we've moved the animation logic to the UI process. However, this makes the animation go out of sync when there's both an accelerated and a non-accelerated animation running at the same time.
The solution may include keeping the animation logic in the UI process, but moving the frame timers to the web-process, sending timed frame events to the UI process without the rest of the animation info.

[Noam Rosenthal]

Created attachment 129525 [details]
Test

[Noam Rosenthal]

Created attachment 134331 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 134331 [details]
Patch

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

> Source/WebKit2/Shared/WebLayerTreeInfo.cpp:35
> +    SimpleArgumentCoder<WebLayerInfo>::encode(encoder, *this);

Could you add a comment somewhere that this requires WebLayerInfo to be a POD type?

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:-128
> -void WebLayerTreeRenderer::syncAnimations()

It would be very nice if the animation removal was a separate patch, or the other way around.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:-176
> -    bool needsToUpdateImageTiles = layerInfo.imageIsUpdated || (layerInfo.contentsRect != layer->contentsRect() && layerInfo.imageBackingStoreID);

You could remove the imageIsUpdated flag from WebLayerInfo if you don't use it anymore.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:169
> +            child = createLayer(childID).leakPtr();

Is this layer deleted anywhere on destruction? Anyway this looks like moved code.

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:279
> +    QImage image = bitmap->createQImage();

Is this intentional?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:79
> +    m_shouldUpdateVisibleRect = true;

I don't see this used anywhere, am I missing something?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:464
> +    float scale = shouldUseTiledBackingStore() ? m_contentsScale : 1;

The way this function is used here is misleading, its name could be improved or a comment might help.
This could also go in a different patch.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:556
>  void WebGraphicsLayer::updateContentBuffers()
>  {
> +    if (m_image) {
> +        if (!m_layerInfo.imageBackingStoreID)
> +            m_layerInfo.imageBackingStoreID = m_webGraphicsLayerClient->adoptImageBackingStore(m_image.get());
> +    }
> +

Is it voluntary to move it back there?

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:645
> +bool WebGraphicsLayer::selfOrAncestorMightHaveNonAffineTransforms()

I would remove "Might".

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:650
> -    if (!m_transformAnimations.isEmpty())
> +    if (!m_layerTransform.combined().isAffine())
>          return true;
>  
> -    if (parent())
> -        return toWebGraphicsLayer(parent())->selfOrAncestorHasActiveTransformAnimations();
> -
>      return false;

return !m_layerTranform.combined().isAffine()

[Noam Rosenthal]

Comment on attachment 134331 [details]
Patch

Separating to a few patches.

[Noam Rosenthal]

Created attachment 134418 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 134418 [details]
Patch

Much softer on the brain, thanks :)
This one LGTM.

[Jocelyn Turcotte]

Comment on attachment 134418 [details]
Patch

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

Sorry, forgot a small nitpick.

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:642
> -    if (!m_transformAnimations.isEmpty())
> -        return true;
> -
>      if (!m_layerTransform.combined().isAffine())
>          return true;

return !m_layerTransform.combined().isAffine(); would look better here. Same comment as before, so just tell me if you think it doesn't make sense.

[Noam Rosenthal]

(In reply to comment #7)
> (From update of attachment 134418 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134418&action=review
> 
> Sorry, forgot a small nitpick.
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:642
> > -    if (!m_transformAnimations.isEmpty())
> > -        return true;
> > -
> >      if (!m_layerTransform.combined().isAffine())
> >          return true;
> 
> return !m_layerTransform.combined().isAffine(); would look better here. Same comment as before, so just tell me if you think it doesn't make sense.

I agree, just missed that.

[Kenneth Rohde Christiansen]

Comment on attachment 134418 [details]
Patch

dont land before we have the whole feature reviewed :)

[Noam Rosenthal]

Created attachment 136266 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136266 [details]
Patch for landing

Clearing flags on attachment: 136266

Committed r113628: <http://trac.webkit.org/changeset/113628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.