Bug 104538

Summary: [Texmap] Animation fails on large layers
Product: WebKit Reporter: Stephane Cerveau <scerveau>
Component: New BugsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Major CC: allan.jensen, gtk-ews, kenneth, noam, ossy, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 104826    
Bug Blocks: 103105    
Attachments:
Description Flags
simple version of tuenti.com
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Stephane Cerveau
Reported 2012-12-10 05:39:46 PST
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.
Attachments
simple version of tuenti.com (1.90 KB, text/html)
2012-12-10 05:39 PST, Stephane Cerveau 		no flags
Details
Patch (9.98 KB, patch)
2012-12-10 06:18 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (10.97 KB, patch)
2012-12-10 06:29 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (17.22 KB, patch)
2012-12-10 09:55 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (17.00 KB, patch)
2012-12-11 03:03 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (17.03 KB, patch)
2012-12-12 01:44 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Stephane Cerveau
Comment 1 2012-12-10 06:03:09 PST
Seems to be related to this bug: https://bugs.webkit.org/show_bug.cgi?id=80456
Allan Sandfeld Jensen
Comment 2 2012-12-10 06:18:07 PST
Created attachment 178531 [details] Patch
EFL EWS Bot
Comment 3 2012-12-10 06:24:37 PST
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 4 2012-12-10 06:25:45 PST
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
Comment 5 2012-12-10 06:29:13 PST
Created attachment 178533 [details] Patch Missed a file from the patch
Noam Rosenthal
Comment 6 2012-12-10 07:39:31 PST
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
Comment 7 2012-12-10 08:32:17 PST
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
Comment 8 2012-12-10 09:55:58 PST
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
Comment 9 2012-12-10 10:02:36 PST
This doesn't seem Qt specific. Maybe change /[Qt]/[CoordGfx]/
Noam Rosenthal
Comment 10 2012-12-10 10:06:24 PST
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
Comment 11 2012-12-11 02:23:09 PST
(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
Comment 12 2012-12-11 03:03:33 PST
Created attachment 178762 [details] Patch
Noam Rosenthal
Comment 13 2012-12-11 13:28:31 PST
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
Comment 14 2012-12-12 01:43:18 PST
(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
Comment 15 2012-12-12 01:44:54 PST
Created attachment 179001 [details] Patch
WebKit Review Bot
Comment 16 2012-12-12 10:33:02 PST
Comment on attachment 179001 [details] Patch Clearing flags on attachment: 179001 Committed r137481: <http://trac.webkit.org/changeset/137481>
WebKit Review Bot
Comment 17 2012-12-12 10:33:07 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 18 2012-12-12 21:32:15 PST
(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
Comment 19 2012-12-13 01:00:14 PST
(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
Comment 20 2012-12-13 02:13:37 PST
(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
Comment 21 2012-12-13 02:48:11 PST
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.
Note You need to log in before you can comment on or make changes to this bug.