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.
Created attachment 175362 [details] Patch
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.
Created attachment 175589 [details] Patch
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
Created attachment 175807 [details] Patch
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?
(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.
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...
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/
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.
Created attachment 179907 [details] Patch
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.
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.
Created attachment 180329 [details] Patch
Comment on attachment 180329 [details] Patch Nice!
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?
Created attachment 181864 [details] Patch
(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.
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
Created attachment 184483 [details] Patch
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" :)
Created attachment 184486 [details] Patch
(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?
(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.
Created attachment 184721 [details] Patch
(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 :)
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 :)
Created attachment 184732 [details] Patch
(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 :)
Created attachment 184733 [details] Patch
(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.
Comment on attachment 184733 [details] Patch Rejecting attachment 184733 [details] from commit-queue. alexander.pashenko@lge.com 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.
Comment on attachment 184733 [details] Patch Clearing flags on attachment: 184733 Committed r140825: <http://trac.webkit.org/changeset/140825>
All reviewed patches have been landed. Closing bug.
Reopen, the fix turned out to be wrong and rolled back by the fix for bug 110685 r143946
Created attachment 190499 [details] Patch
Created attachment 190513 [details] Patch
Landed in https://trac.webkit.org/r144183