Bug 75780 - [Qt][WK2] Accelerated and non-accelerated animations need to be synchronized
Summary: [Qt][WK2] Accelerated and non-accelerated animations need to be synchronized
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: Qt
Depends on: 82522 82523 82525 82533 82534
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 09:44 PST by Noam Rosenthal
Modified: 2012-04-09 14:59 PDT (History)
6 users (show)

See Also:


Attachments
Test (689 bytes, text/html)
2012-02-29 14:58 PST, Noam Rosenthal
no flags Details
Patch (43.65 KB, patch)
2012-03-28 10:39 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (12.79 KB, patch)
2012-03-28 15:12 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (12.80 KB, patch)
2012-04-09 10:52 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-01-07 09:44:54 PST
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.
Comment 1 Noam Rosenthal 2012-02-29 14:58:33 PST
Created attachment 129525 [details]
Test
Comment 2 Noam Rosenthal 2012-03-28 10:39:32 PDT
Created attachment 134331 [details]
Patch
Comment 3 Jocelyn Turcotte 2012-03-28 12:22:17 PDT
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()
Comment 4 Noam Rosenthal 2012-03-28 12:55:40 PDT
Comment on attachment 134331 [details]
Patch

Separating to a few patches.
Comment 5 Noam Rosenthal 2012-03-28 15:12:32 PDT
Created attachment 134418 [details]
Patch
Comment 6 Jocelyn Turcotte 2012-03-29 04:32:40 PDT
Comment on attachment 134418 [details]
Patch

Much softer on the brain, thanks :)
This one LGTM.
Comment 7 Jocelyn Turcotte 2012-03-29 05:07:13 PDT
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.
Comment 8 Noam Rosenthal 2012-03-29 06:42:48 PDT
(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.
Comment 9 Kenneth Rohde Christiansen 2012-04-03 07:09:01 PDT
Comment on attachment 134418 [details]
Patch

dont land before we have the whole feature reviewed :)
Comment 10 Noam Rosenthal 2012-04-09 10:52:55 PDT
Created attachment 136266 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-04-09 14:59:29 PDT
Comment on attachment 136266 [details]
Patch for landing

Clearing flags on attachment: 136266

Committed r113628: <http://trac.webkit.org/changeset/113628>
Comment 12 WebKit Review Bot 2012-04-09 14:59:35 PDT
All reviewed patches have been landed.  Closing bug.