[WPE][GTK] Content disappearing when using CSS transforms

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Steffen Deusch]

Hello everyone,
I noticed a rather critical bug using the WPE WebKit port. I already opened an issue in the WPEWebKit repo on GitHub, but Wouter van Boessschoten ([e-mail]) told me to file a report here too.
The original issue is located here: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/434#issue-286382861

Summary: When using CSS transforms to create a scrolling element animation, the animated content simply disappears after a while:
https://s3.eu-central-1.amazonaws.com/steffend/public/wpe-test-rpi3/rss-scroller/vid.mp4
Notice, that as soon as another element changes, the content appears again (keep an eye on the 'random' value), until it abruptly stops again.
Repro: https://s3.eu-central-1.amazonaws.com/steffend/public/wpe-test-rpi3/rss-scroller/rss.html

Another example with the same issue: https://s3.eu-central-1.amazonaws.com/steffend/public/webkitbug/WPEScrollBug.mp4 (this should be an endlessly scrolling table)
Repro: https://s3.eu-central-1.amazonaws.com/steffend/public/webkitbug/scrolling.html

Opening these pages in Safari/Firefox/Chromium works as expected (apart from https://bugs.webkit.org/show_bug.cgi?id=181709).

Hardware: Raspberry Pi 3
Build method: WPE Buildroot (https://github.com/WebPlatformForEmbedded/buildroot), same using Yocto (https://github.com/WebPlatformForEmbedded/meta-wpe) + Westeros.
WPE commit id: 959d9b1d2555da0ce65bd0198bc509dd08e9f910 (doesn't matter, same problem in every version I tested, master and stable)

As I am using these animation techniques in production, I am currently forced to use Chromium-based boxes, but I'd appreciate using a custom-built RPi image much more.

Thanks for any help!
Steffen Deusch

[Michael Catanzaro]

I can reproduce with WebKitGTK+, with both trunk and 2.18.5. Looks like the problem is caused by accelerated compositing mode, so if you need a workaround you can set the environment variable WEBKIT_DISABLE_COMPOSITING_MODE=1.

(In reply to Steffen Deusch from comment #0)
> https://s3.eu-central-1.amazonaws.com/steffend/public/wpe-test-rpi3/rss-
> scroller/vid.mp4

If you need to post videos in the future, please use .webm so we can all watch them. Doesn't matter this time, since you have a nice test page that shows the issue clearly.

[Steffen Deusch]

> if you need a workaround you can set the environment variable WEBKIT_DISABLE_COMPOSITING_MODE=1
I tried adding export WEBKIT_DISABLE_COMPOSITING_MODE=1 to /usr/bin/wpe, but this didn't work. Manually exporting it and using WPELauncher didn't work either. Setting other env vars in /usr/bin/wpe does work as I can enable and disable WEBKIT_SHOW_FPS.

Disabling HW Compositing would make performance much worse, wouldn't it? I guess I'll have to wait until someone finds the root cause of this issue.

> please use .webm so we can all watch them
This is actually a problem, since Safari users cannot play .webm files :/
Quite sad that video codecs are still such an issue on the web. Uploading them to Google Drive or YouTube should work, so maybe I'll do this the next time!

[Michael Catanzaro]

(In reply to Steffen Deusch from comment #2)
> > if you need a workaround you can set the environment variable WEBKIT_DISABLE_COMPOSITING_MODE=1
> I tried adding export WEBKIT_DISABLE_COMPOSITING_MODE=1 to /usr/bin/wpe, but
> this didn't work. Manually exporting it and using WPELauncher didn't work
> either. Setting other env vars in /usr/bin/wpe does work as I can enable and
> disable WEBKIT_SHOW_FPS.

Apparently this is a GTK-specific environment variable. Come to think of it, I think it's not actually possible to disable AC mode in WPE: I notice our hardware acceleration API does not exist there, so likely not. Possibly for the better.

But at least we now know that the problem lies somewhere in that part of the code.

> Disabling HW Compositing would make performance much worse, wouldn't it?

On a Raspberry Pi, probably yes. I'm afraid users are starting to think of it as a magic environment variable that makes WebKit work properly. Even some applications have started using it, which is sad....

> This is actually a problem, since Safari users cannot play .webm files :/
> Quite sad that video codecs are still such an issue on the web. Uploading
> them to Google Drive or YouTube should work, so maybe I'll do this the next
> time!

:(

[Carlos Alberto Lopez Perez]

WPE doesn't support running without accelerated composing mode (AFAIK).

[Miguel Gomez]

(In reply to Carlos Alberto Lopez Perez from comment #4)
> WPE doesn't support running without accelerated composing mode (AFAIK).

It doesn't. AC is always enabled and cannot be disabled.

[Miguel Gomez]

This is reproducible in wkgtk as well, and doesn't happen disabling AC there. At first look, it seems that there is a problem updating the tiles of the backingStore where the scrolling text is shown. Maybe the tiles are not properly drawn by cairo or maybe they are not properly uploaded for the composition.

Seems to happen every now and then, but only the first time a tile is shown. If something causes a redraw (like the random number changing), they are refreshed and the content is properly shown. I'll dig into it.

[Miguel Gomez]

I found the cause of the problem. It happens when animating a layer that has a backingStore and an animation that causes the visible rect of the backingStore to change.

In a normal case, a layer is animated by the compositing thread, recalculating its transformation matrix as needed in its TextureMapperLayer, and no layer flushes are required for this.

But when using a backingStore whose visible rect can change due to the animation, we need to perform a layer flush because that's what recalculates the visible rect and creates the needed tiles in the backingStore. As we are not doing so, once the animation advances and the visible rect goes out of the initially created tiles nothing gets rendered inside the layer.

When the random number in the page changes, that causes a layer flush and updates the tiles in the animated layer, that's why it becomes visible at that point. Eventually the visible rect moves out of the current tiles again and the content disappears again.

We need to force a layer flush in the case of animations where the visible rect changes and require creating new tiles.

I'm lacking time to prepare a fix for this now, but I'll keep working on it as soon as I have time.

[Miguel Gomez]

Created attachment 333278 [details]
Patch

[Michael Catanzaro]

Comment on attachment 333278 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:48
> +// tiles, and to use layer flushes on layers bigget than that so we don't waste too much memory.

bigger

[Carlos Alberto Lopez Perez]

Comment on attachment 333278 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +
> +        No new tests, no behavior change.
> +

I think "no behaviour change" is not accurate.
Either this is covered by existing tests (and this patch makes failing tests to start passing) or some new test should be added.
Hopefully is relatively easy to convert the test case added here as repro to a layout test one.

[Zan Dobersek]

Comment on attachment 333278 [details]
Patch

I don't like this approach too much because of the extra timer, continuous flushing, continuous updating originating from the compositor thread, extra layer tree traversal via descendantsAndSelfAnimationsCanBeRunOnCompositor(). I don't have an alternative to offer, but I want to understand the backing store management in CoordinatedGraphicsLayer before proceeding here.

[Miguel Gomez]

(In reply to Zan Dobersek from comment #11)
> Comment on attachment 333278 [details]
> Patch
> 
> I don't like this approach too much because of the extra timer, continuous
> flushing, continuous updating originating from the compositor thread, extra
> layer tree traversal via descendantsAndSelfAnimationsCanBeRunOnCompositor().
> I don't have an alternative to offer, but I want to understand the backing
> store management in CoordinatedGraphicsLayer before proceeding here.

Another option could be having a timer in CoordinatedGraphicsLayer that checked whether the rendered rectangle is covered by the backingStore tiles, and caused a layer flush when needed.

[Sylvain Garrigues]

I confirm the attached patch solves this problem, although it seems to introduce a slightly visible performance penalty.

I filmed the bug as it appeared:
https://www.sylvaingarrigues.com/tv.mp4

And now with this patch:
https://www.sylvaingarrigues.com/tv-patched.mp4
You can see some performance problems in the ease-out part of the transition.

The test website is:
https://www.sylvaingarrigues.com/CladBox/index.html

[Sylvain Garrigues]

(my issue originally reported here: https://github.com/WebPlatformForEmbedded/WPEWebKit/issues/511)

[Nikola Veljkovic]

Hi, 
We are still hitting this issue on WPE 2.22.

@Zan, @Miguel: Is there a plan to fix this issue in a better way?

[Miguel Gomez]

(In reply to Nikola Veljkovic from comment #15)
> Hi, 
> We are still hitting this issue on WPE 2.22.
> 
> @Zan, @Miguel: Is there a plan to fix this issue in a better way?

I'm working on a fix, but I don't have a lot of time for it. Give me some days more.

[Miguel Gomez]

Created attachment 367306 [details]
Patch

[Miguel Gomez]

Created attachment 367307 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 367307 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:691
> +    m_animations.apply(futureApplicationResults, time + Seconds::fromMilliseconds(50));

50_ms

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:650
> +class CoordinatedAnimatedBackingStoreClient : public AnimatedBackingStoreClient {

final

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:652
> +    CoordinatedAnimatedBackingStoreClient(RefPtr<CoordinatedGraphicsLayer::AnimatedBackingStoreHost>&& host, FloatRect visibleRect, FloatRect coverRect, FloatSize size, float contentsScale)

use const FloatRect& for the FloatRect parameters, or && if they are transferred, but I don't think that's the case.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:658
> +    { }

Since it's refcounted, I would provide a create() rerturning a Ref<> and make the constructor private.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:661
> +    void setCoverRect(IntRect rect) { m_coverRect = rect; }

const IntRect&

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:662
> +    void requestBackingStoreUpdateIfNeeded(TransformationMatrix& transform) final

is transform expected to be changed here, or should it be const?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:754
> +        m_nicosia.animatedBackingStoreClient = adoptRef(new CoordinatedAnimatedBackingStoreClient(m_animatedBackingStoreHost.copyRef(), m_coordinator->visibleContentsRect(), IntRect(0, 0, 0, 0), m_size, effectiveContentsScale()));

I think you can use { } instead of IntRect(0, 0, 0, 0)

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:130
> +        AnimatedBackingStoreHost(CoordinatedGraphicsLayer* layer)

explicit. I think it should receive a reference, even when we store the pointer because it's made nullptr later, but the constructor is always expected to receive a non null layer, right?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:132
> +        { }

I would add a create() here too.

[Miguel Gomez]

Created attachment 367319 [details]
Patch

[Zan Dobersek]

Comment on attachment 367319 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:691
> +    m_animations.apply(futureApplicationResults, time + 50_ms);

This second call into TextureMapperAnimation::apply() can still change the state of m_animations. I think that's possibly problematic.

> Source/WebCore/platform/graphics/texmap/coordinated/AnimatedBackingStoreClient.h:35
> +namespace WebCore {

This interface should be in the Nicosia namespace, and under platform/graphics/nicosia/. This way it won't cause problems in non-CoordinatedGraphics configurations.

> Source/WebCore/platform/graphics/texmap/coordinated/AnimatedBackingStoreClient.h:39
> +class AnimatedBackingStoreClient : public RefCounted<AnimatedBackingStoreClient> {

ThreadSafeRefCounted

> Source/WebCore/platform/graphics/texmap/coordinated/AnimatedBackingStoreClient.h:41
> +    virtual ~AnimatedBackingStoreClient() { };

= default;

[Miguel Gomez]

Created attachment 372118 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372118 [details]
Patch

Clearing flags on attachment: 372118

Committed r246963: <https://trac.webkit.org/changeset/246963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.