Bug 104538 – [Texmap] Animation fails on large layers

• Component: New Bugs
• Status: RESOLVED FIXED

[Stephane Cerveau]

Created attachment 178522 [details]
simple version of tuenti.com

In this website there is a bounceInUp which causes a crash with using QT-Directfb implementation.
Indeed this animation is composed of one image and a text indent of 9999em. 
A surface is created in TextureMapperLayer::updateBackingStore with a big size such as 160000x300 which is not supported by directfb, causing a crash after imageBuffer = ImageBuffer::create(dirtyRect.size());
If i remove the text indentation, this is not crashing anymore and the animation is working fine.
In attachment a simplified version of www.tuenti.com
This website is perfectly working with Chromium or Safari.

[Stephane Cerveau]

Seems to be related to this bug:
https://bugs.webkit.org/show_bug.cgi?id=80456

[Allan Sandfeld Jensen]

Created attachment 178531 [details]
Patch

[EFL EWS Bot]

Comment on attachment 178531 [details]
Patch

Attachment 178531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15222732

[kov's GTK+ EWS bot]

Comment on attachment 178531 [details]
Patch

Attachment 178531 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15230604

[Allan Sandfeld Jensen]

Created attachment 178533 [details]
Patch

Missed a file from the patch

[Noam Rosenthal]

Comment on attachment 178533 [details]
Patch

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

This would bring back the "old" performance issue with drawing text several times if it falls on the border between tiles...
We should only do this in the case of large dirtyRects.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:98
> +    void updateContents(TextureMapper*, GraphicsLayer*, const FloatSize&, const IntRect&, BitmapTexture::UpdateContentsFlag);

This function doesn't seem right. I don't like passing all these responsibilities to TextureMapperTile.
I prefer that GraphicsLayerTextureMapper perform all the painting; and if we need to use something like a "Client" approach here.

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:68
> +    virtual IntSize maxTextureSize() const { return IntSize(2048, 2048); }

Seems arbitrary, an enum with some explicit name could help...

[Allan Sandfeld Jensen]

Renamed the bug. It causes crashes on DirectFb, on X11 it simply prevents the any animation on large layers, meaning we get stuck in the from position.

[Allan Sandfeld Jensen]

Created attachment 178576 [details]
Patch

Use the old path for all repaints that fits, and only do tiled painting when necessary. Tiled painting now happens one step lower to actually avoid any intermediate imagebuffers.

[Kenneth Rohde Christiansen]

This doesn't seem Qt specific. Maybe change /[Qt]/[CoordGfx]/

[Noam Rosenthal]

Comment on attachment 178576 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:475
> +    // We need to paint directly when the dirty rect excedes one of the maximum dimensions.

exceed

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:89
> +    virtual void getDebugBorderInfo(Color&, float& width) const OVERRIDE;

This doesn't seem to be used.

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:65
> +    GraphicsContext* context = m_image->context();
> +
> +    IntRect sourceRect(targetRect);
> +    sourceRect.setLocation(sourceOffset);
> +    context->translate(targetRect.x() - sourceOffset.x(), targetRect.y() - sourceOffset.y());
> +    sourceLayer->paintGraphicsLayerContents(*context, sourceRect);

You probably want to clear the rect before you paint directly into it.

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:29
> +static const int s_maximumAllowedImageBufferDimension = 4096;

Can this be inside the function, or in a CPP file?

[Allan Sandfeld Jensen]

(In reply to comment #9)
> This doesn't seem Qt specific. Maybe change /[Qt]/[CoordGfx]/

It is actually TexMap but non-CoordGfx specific. So for Qt only WK1.

[Allan Sandfeld Jensen]

Created attachment 178762 [details]
Patch

[Noam Rosenthal]

Comment on attachment 178762 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:64
> +

Extra line

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:68
> +    context->setCompositeOperation(CompositeCopy);

No... You need to clear and then apply CompositeSourceOver. Otherwise the CompositeCopy state would linger while painting the layer, which would cause different primitives inside the layer to copy on top of each other.

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (From update of attachment 178762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178762&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:64
> > +
> 
> Extra line
> 
That was were clearRect was ;) I couldn't get it to work, but I found out it is because the translations were not reset.

[Allan Sandfeld Jensen]

Created attachment 179001 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179001 [details]
Patch

Clearing flags on attachment: 179001

Committed r137481: <http://trac.webkit.org/changeset/137481>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #16)
> (From update of attachment 179001 [details])
> Clearing flags on attachment: 179001
> 
> Committed r137481: <http://trac.webkit.org/changeset/137481>

It broke all debug builds. You should have build in debug mode too if you add an,asseetion. Could you fix or rollout, please? I can't help, I won't be online for 3-4-5 hours.

[Csaba Osztrogonác]

(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 179001 [details] [details])
> > Clearing flags on attachment: 179001
> > 
> > Committed r137481: <http://trac.webkit.org/changeset/137481>
> 
> It broke all debug builds. You should have build in debug mode too if you add an,asseetion. Could you fix or rollout, please? I can't help, I won't be online for 3-4-5 hours.

It was fixed by https://trac.webkit.org/changeset/137502 in a separated bug.

[Allan Sandfeld Jensen]

(In reply to comment #18)
> (In reply to comment #16)
> > (From update of attachment 179001 [details] [details])
> > Clearing flags on attachment: 179001
> > 
> > Committed r137481: <http://trac.webkit.org/changeset/137481>
> 
> It broke all debug builds. You should have build in debug mode too if you add an,asseetion. Could you fix or rollout, please? I can't help, I won't be online for 3-4-5 hours.

It only broke non-Qt debug builds. I mistakenly un-ifdef'ed an assertion, that would make sense on most platforms but doesn't build on platforms that disable rtti.

[Csaba Osztrogonác]

Yes, it's true, it broke only non-Qt debug builds. Qt debug build was broken by another patch next to this patch - https://trac.webkit.org/changeset/137486.

But you could have avoided breaking others build for 2 hours and 
20 commits if you landed your patch manually and watch the bots.