Bug 102501 – [TexMap] Flickering after transitions on Apple HTML5 demo

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

[Alexander Paschenko]

Created attachment 174673 [details]
This test page demonstrates occasional flickering at the end of the animation.

Flickering can be seen occasionally in the end of the animations - past frames appear for a fraction of a second.

To reproduce:

- Open QtTestBrowser
- Go to Apple's HTML5 showcase site (the link provided), choose "Cube" 3D Transition
- Click "Watch it again" link under the rotating image

You will see that quite often the previous image appears instead of the new one - just for a moment, but it's perfectly visible.
Also similar thing can be seen if you open the attached file either in QtTestBrowser or in MiniBrowser. Flickering appears not in the end of each transition, but every now and then - just keep watching.

[Alexander Paschenko]

Created attachment 175362 [details]
Patch

[Noam Rosenthal]

Comment on attachment 175362 [details]
Patch

You need a Changelog when submitting a patch.
Also, in your changelog, please explain why need to modify graphicsLayer.

[Alexander Paschenko]

Created attachment 175589 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175589 [details]
Patch

Attachment 175589 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14937457

New failing tests:
transforms/3d/hit-testing/backface-hit-test.html
transforms/3d/hit-testing/backface-no-transform-hit-test.html

[Alexander Paschenko]

Created attachment 175807 [details]
Patch

[Noam Rosenthal]

Comment on attachment 175807 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:495
> +        graphicsLayer->setTransform(m_state.transform);
> +
> +    if (!m_animations.hasActiveAnimationsOfType(AnimatedPropertyOpacity))
> +        m_state.opacity = graphicsLayer->opacity();
> +    else
> +        graphicsLayer->setOpacity(m_state.opacity);

It seems a bit unusual for TextureMapperLayer to update GraphicsLayerTextureMapper. Is there a cleaner way to do this?

[Alexander Paschenko]

(In reply to comment #6)
> (From update of attachment 175807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175807&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:495
> > +        graphicsLayer->setTransform(m_state.transform);
> > +
> > +    if (!m_animations.hasActiveAnimationsOfType(AnimatedPropertyOpacity))
> > +        m_state.opacity = graphicsLayer->opacity();
> > +    else
> > +        graphicsLayer->setOpacity(m_state.opacity);
> 
> It seems a bit unusual for TextureMapperLayer to update GraphicsLayerTextureMapper. Is there a cleaner way to do this?

This is the only way to sync TextureMapperLayer and GraphicsLayerTextureMapper known to us at the moment, although it appears to be a bit messy. If there's a better way to do that could you please share your thoughts then? If it's critical we could try searching for other ways. But nevertheless we'd like to point that GraphicsLayerTextureMapper doesn't get passed to flushCompositingStateSelf as const, and that's why it's prone to changes inside this method.

[Noam Rosenthal]

Comment on attachment 175807 [details]
Patch

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

The contract between GraphicsLayerTextureMapper and TextureMapperLayer is the following:
- m_state should mirror the state of GLTM after the last sync.
- m_opacity and m_transform should hold the transform and opacity to paint with, including animations.

This patch breaks that contract; We should think harder to find a way to fix the flicking problem without breaking that contract. I don't have a precise idea right now but I'd give it some thought.

>>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:495
>>> +        graphicsLayer->setOpacity(m_state.opacity);
>> 
>> It seems a bit unusual for TextureMapperLayer to update GraphicsLayerTextureMapper. Is there a cleaner way to do this?
> 
> This is the only way to sync TextureMapperLayer and GraphicsLayerTextureMapper known to us at the moment, although it appears to be a bit messy. If there's a better way to do that could you please share your thoughts then? If it's critical we could try searching for other ways. But nevertheless we'd like to point that GraphicsLayerTextureMapper doesn't get passed to flushCompositingStateSelf as const, and that's why it's prone to changes inside this method.

Are you sure this would work with fill modes? I have a feeling this kind of code was there before and had some issues...

[Helder Correia]

Tested on EFL, I can still see the flickering (at least) after the slide-in and multi-flip animations from Apple's HTML5 showcase. Also, it doesn't add any improvement to the flickering in Sencha Touch's Kitchen Sink: http://dev.sencha.com/deploy/touch/examples/production/kitchensink/

[Alexander Paschenko]

All right, we will try to figure something out about this bug.
As for Sencha Examples, we believe that its flickering could be caused by a different issue and we're currently working on a patch for it.

[Alexander Paschenko]

Created attachment 179907 [details]
Patch

[Alexander Paschenko]

A new patch has been proposed - please have a look if you're interested.

Please note, however, that, strangely, on the latest debug build of QtWebKit *without* this patch there seems to be a problem with displaying http://www.apple.com/html5/showcase/transitions/ - no transitions show at all, except "Rotate In": the latter shows the flickering but generally works while all others seem to show no animation at all, just the first and the last frame, in other words, the previous picture shows before the animation and then the next picture shows after the animation. Probably there's been some regression lately?
After applying this patch, flickering in "Rotate In" is gone, behavior of all other transitions is unchanged.

[Alexander Paschenko]

Update: MiniBrowser shows all transitions just fine after applying the patch, no flickering to be seen. The above problem seems to appear only in QtTestBrowser, the reasons of it are unknown.

[Alexander Paschenko]

Created attachment 180329 [details]
Patch

[Noam Rosenthal]

Comment on attachment 180329 [details]
Patch

Nice!

[Kenneth Rohde Christiansen]

Comment on attachment 180329 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:186
> +    bool m_shouldUpdateTransformFromGraphicsLayer;
> +    bool m_shouldUpdateOpacityFromGraphicsLayer;
> +

why are these not initialized in the ctor?

[Alexander Paschenko]

Created attachment 181864 [details]
Patch

[Alexander Paschenko]

(In reply to comment #16)
> (From update of attachment 180329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180329&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:186
> > +    bool m_shouldUpdateTransformFromGraphicsLayer;
> > +    bool m_shouldUpdateOpacityFromGraphicsLayer;
> > +
> 
> why are these not initialized in the ctor?

Fixed. Please advise.

[WebKit Review Bot]

Comment on attachment 181864 [details]
Patch

Rejecting attachment 181864 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
apperLayer.cpp.rej
patching file Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h
Hunk #1 succeeded at 80 with fuzz 2.
Hunk #2 FAILED at 90.
Hunk #3 FAILED at 131.
Hunk #4 succeeded at 160 (offset -6 lines).
2 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Noam Rosenthal']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16035037

[Alexander Paschenko]

Created attachment 184483 [details]
Patch

[Noam Rosenthal]

Comment on attachment 184483 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        And these flags themselves are set based on GraphicsLayerTextureMapper's changeMask which indicates what details of the state have been changed since the last sync.

Remove the first word "And" :)

[Alexander Paschenko]

Created attachment 184486 [details]
Patch

[Alexander Paschenko]

(In reply to comment #21)
> (From update of attachment 184483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184483&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        And these flags themselves are set based on GraphicsLayerTextureMapper's changeMask which indicates what details of the state have been changed since the last sync.
> 
> Remove the first word "And" :)

Done :)
We would like also to point out that there is a potential problem with m_currentFilters which currently gets updated in a manner which is fixed with this patch, and so it appears to be prone to undesired affections as well. What do you think, should we enhance the patch or just leave m_currentFilters alone for now?

[Noam Rosenthal]

(In reply to comment #23)
> (In reply to comment #21)
> > (From update of attachment 184483 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184483&action=review
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        And these flags themselves are set based on GraphicsLayerTextureMapper's changeMask which indicates what details of the state have been changed since the last sync.
> > 
> > Remove the first word "And" :)
> 
> Done :)
> We would like also to point out that there is a potential problem with m_currentFilters which currently gets updated in a manner which is fixed with this patch, and so it appears to be prone to undesired affections as well. What do you think, should we enhance the patch or just leave m_currentFilters alone for now?

It's a good idea to deal with filters here as well.

[Alexander Paschenko]

Created attachment 184721 [details]
Patch

[Alexander Paschenko]

(In reply to comment #24)
> It's a good idea to deal with filters here as well.
Done. Please advise.
Hope this one will be the final one for this bug :)

[Noam Rosenthal]

Comment on attachment 184721 [details]
Patch

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

Great! Please update the changelog...

> Source/WebCore/ChangeLog:22
> +        This patch solves the problem by introducing two additional private flags to TextureMapperLayer: m_shouldUpdateCurrentTransformFromGraphicsLayer and m_shouldUpdateCurrentOpacityFromGraphicsLayer.
> +        On these flags' basis, TextureMapperLayer is able to decide whether to update its inner state or not.
> +        These flags themselves are set based on GraphicsLayerTextureMapper's changeMask which indicates what details of the state have been changed since the last sync.
> +        Also, to avoid similar problems with m_currentFilters in the future, a flag with similar meaning named m_shouldUpdateCurrentFiltersFromGraphicsLayer has been introduced. Corresponding methods have been updated accordingly.
> +
> +        No new tests - this doesn't expose any testable surface. Eyes-only check has been made to ensure that the problem is gone now.
> +
> +        * platform/graphics/texmap/TextureMapperLayer.cpp:
> +        (WebCore::TextureMapperLayer::setAnimatedTransform): sets m_shouldUpdateCurrentTransformFromGraphicsLayer to false and updates m_currentTransform based on the updated state from GraphicsLayerAnimation.
> +        (WebCore):
> +        (WebCore::TextureMapperLayer::setAnimatedOpacity): sets m_shouldUpdateCurrentOpacityFromGraphicsLayer to false and updates m_currentOpacity based on the updated state from GraphicsLayerAnimation.
> +        (WebCore::TextureMapperLayer::setAnimatedFilters): sets m_shouldUpdateCurrentFiltersFromGraphicsLayer to false and updates m_currentFilters based on the updated state from GraphicsLayerAnimation.
> +        (WebCore::TextureMapperLayer::flushCompositingStateForThisLayerOnly): sets m_shouldUpdateCurrent* flags based on GLTM's changeMask. Also illegal modification of m_currentTransform that caused flickering has been removed from this method.
> +        (WebCore::TextureMapperLayer::syncAnimations): updates m_currentTransform and/or m_currentOpacity and/or m_currentFilters if corresponding flags allow to do so.

Please keep Changelog lines at max of 100.
Also, you say "two" additional flags, where we have 3 :)

[Alexander Paschenko]

Created attachment 184732 [details]
Patch

[Alexander Paschenko]

(In reply to comment #27)
> Please keep Changelog lines at max of 100.
> Also, you say "two" additional flags, where we have 3 :)

Done, please advise :)

[Alexander Paschenko]

Created attachment 184733 [details]
Patch

[Alexander Paschenko]

(In reply to comment #29)
> (In reply to comment #27)
> > Please keep Changelog lines at max of 100.
> > Also, you say "two" additional flags, where we have 3 :)
> 
> Done, please advise :)
My bad, previous one would've messed the ChangeLog a little.

[WebKit Review Bot]

Comment on attachment 184733 [details]
Patch

Rejecting attachment 184733 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 184733 [details]
Patch

Clearing flags on attachment: 184733

Committed r140825: <http://trac.webkit.org/changeset/140825>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Allan Sandfeld Jensen]

Reopen, the fix turned out to be wrong and rolled back by the fix for bug 110685 r143946

[Allan Sandfeld Jensen]

Created attachment 190499 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 190513 [details]
Patch

[Allan Sandfeld Jensen]

Landed in https://trac.webkit.org/r144183