Summary: | [Texmap] Animation fails on large layers | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephane Cerveau <scerveau> | ||||||||||||||
Component: | New Bugs | Assignee: | 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: |
|
Seems to be related to this bug: https://bugs.webkit.org/show_bug.cgi?id=80456 Created attachment 178531 [details]
Patch
Comment on attachment 178531 [details] Patch Attachment 178531 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15222732 Comment on attachment 178531 [details] Patch Attachment 178531 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15230604 Created attachment 178533 [details]
Patch
Missed a file from the patch
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... 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. 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.
This doesn't seem Qt specific. Maybe change /[Qt]/[CoordGfx]/ 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? (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. Created attachment 178762 [details]
Patch
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. (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. Created attachment 179001 [details]
Patch
Comment on attachment 179001 [details] Patch Clearing flags on attachment: 179001 Committed r137481: <http://trac.webkit.org/changeset/137481> All reviewed patches have been landed. Closing bug. (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. (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. (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. 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. |
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.