Bug 24227 - Implement checkForSolidColor in ImageQt.cpp
Summary: Implement checkForSolidColor in ImageQt.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-27 09:28 PST by Adam Treat
Modified: 2009-03-02 07:37 PST (History)
1 user (show)

See Also:


Attachments
Implement (2.22 KB, patch)
2009-02-27 09:28 PST, Adam Treat
eric: review+
Details | Formatted Diff | Diff
Implements the Qt version and also fixes a bug in x-platform version (7.23 KB, patch)
2009-02-27 12:09 PST, Adam Treat
eric: review+
Details | Formatted Diff | Diff
Adds some comments and asserts (7.56 KB, patch)
2009-02-27 12:30 PST, Adam Treat
no flags Details | Formatted Diff | Diff
New version that removes wrong assert (7.80 KB, patch)
2009-03-02 07:18 PST, Adam Treat
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-02-27 09:28:03 PST
Hi,

The mac port has a nifty optimization for 1x1 images.  It simply converts any painting of such into a fill of a solid color.  The following patch implements this for the Qt port and also enables it for drawTiled as well which results in dropping a 1x1 background image from taking ~50ms to ~0ms on my system.
Comment 1 Adam Treat 2009-02-27 09:28:55 PST
Created attachment 28075 [details]
Implement
Comment 2 Eric Seidel (no email) 2009-02-27 09:59:22 PST
Comment on attachment 28075 [details]
Implement

LGTM
Comment 3 Adam Treat 2009-02-27 12:09:27 PST
Created attachment 28097 [details]
Implements the Qt version and also fixes a bug in x-platform version

I've since noticed that the x-platform version of drawPattern tries to use this optimization, but does so incorrectly.  This version fixes that so that the optimization always works when valid.
Comment 4 Eric Seidel (no email) 2009-02-27 12:16:49 PST
Comment on attachment 28097 [details]
Implements the Qt version and also fixes a bug in x-platform version

if (!m_checkedForSolidColor) checkForSolidColor(); should be two lines.  Otherwise looks good.

You shoudl add an ASSERT to mayFillWithSolidColor() and explain somewhere why this works with images which are not loaded yet.  Perhaps in teh header next to checkForSolidColor() // this shoudl check regardless of whether m_checkedForSolidColor is set, as the frame may have changed.
Comment 5 Adam Treat 2009-02-27 12:30:14 PST
Created attachment 28100 [details]
Adds some comments and asserts
Comment 6 Adam Treat 2009-02-27 12:43:09 PST
Landed with r41295.
Comment 7 Anders Carlsson 2009-02-27 14:47:02 PST
I had to back this out because it lead to assertions in the mac port.

0x0000000101378136 in WebCore::BitmapImage::mayFillWithSolidColor (this=0x11bc56050) at BitmapImage.h:218
218	            ASSERT(m_haveFrameCount);
(gdb) bt
#0  0x0000000101378136 in WebCore::BitmapImage::mayFillWithSolidColor (this=0x11bc56050) at BitmapImage.h:218
#1  0x0000000101708e4e in WebCore::Image::drawTiled (this=0x11bc56050, ctxt=0x7fff5fbfb9b0, destRect=@0x7fff5fbfa950, srcPoint=@0x7fff5fbfa970, scaledTileSize=@0x7fff5fbfa960, op=WebCore::CompositeSourceOver) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/platform/graphics/Image.cpp:109
#2  0x0000000101662f1a in WebCore::GraphicsContext::drawTiledImage (this=0x7fff5fbfb9b0, image=0x11bc56050, rect=@0x7fff5fbfaa30, srcPoint=@0x7fff5fbfab10, tileSize=@0x7fff5fbfab00, op=WebCore::CompositeSourceOver) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/platform/graphics/GraphicsContext.cpp:449
#3  0x00000001019b9cd6 in WebCore::RenderBoxModelObject::paintFillLayerExtended (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, c=@0x11bcb8a80, bgLayer=0x11bcb8a58, clipY=250, clipH=231, tx=14, ty=250, w=236, h=231, box=0x0, op=WebCore::CompositeSourceOver) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBoxModelObject.cpp:354
#4  0x00000001019a65f2 in WebCore::RenderBox::paintFillLayer (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, c=@0x11bcb8a80, fillLayer=0x11bcb8a58, clipY=250, clipH=231, tx=14, ty=250, width=236, height=231, op=WebCore::CompositeSourceOver) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBox.cpp:762
#5  0x00000001019a66c8 in WebCore::RenderBox::paintFillLayers (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, c=@0x11bcb8a80, fillLayer=0x11bcb8a58, clipY=250, clipH=231, tx=14, ty=250, width=236, height=231, op=WebCore::CompositeSourceOver) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBox.cpp:756
#6  0x00000001019b080b in WebCore::RenderBox::paintBoxDecorations (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, tx=14, ty=250) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBox.cpp:671
#7  0x000000010199060b in WebCore::RenderBlock::paintObject (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, tx=14, ty=250) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBlock.cpp:1748
#8  0x000000010198bf46 in WebCore::RenderBlock::paint (this=0x1174dade8, paintInfo=@0x7fff5fbfb130, tx=14, ty=250) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderBlock.cpp:1578
#9  0x00000001019e0f04 in WebCore::RenderLayer::paintLayer (this=0x1174daec8, rootLayer=0x1066a1078, p=0x7fff5fbfb9b0, paintDirtyRect=@0x7fff5fbfb9a0, haveTransparency=false, paintRestriction=WebCore::PaintRestrictionNone, paintingRoot=0x0, appliedTransform=false, temporaryClipRects=false) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderLayer.cpp:1955
#10 0x00000001019e1381 in WebCore::RenderLayer::paintLayer (this=0x1066d1848, rootLayer=0x1066a1078, p=0x7fff5fbfb9b0, paintDirtyRect=@0x7fff5fbfb9a0, haveTransparency=false, paintRestriction=WebCore::PaintRestrictionNone, paintingRoot=0x0, appliedTransform=false, temporaryClipRects=false) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderLayer.cpp:2009
#11 0x00000001019e1381 in WebCore::RenderLayer::paintLayer (this=0x1066a1078, rootLayer=0x1066a1078, p=0x7fff5fbfb9b0, paintDirtyRect=@0x7fff5fbfb9a0, haveTransparency=false, paintRestriction=WebCore::PaintRestrictionNone, paintingRoot=0x0, appliedTransform=false, temporaryClipRects=false) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderLayer.cpp:2009
#12 0x00000001019e1567 in WebCore::RenderLayer::paint (this=0x1066a1078, p=0x7fff5fbfb9b0, damageRect=@0x7fff5fbfb9a0, paintRestriction=WebCore::PaintRestrictionNone, paintingRoot=0x0) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/rendering/RenderLayer.cpp:1818
#13 0x000000010164d6d6 in WebCore::FrameView::paintContents (this=0x1073540f0, p=0x7fff5fbfb9b0, rect=@0x7fff5fbfb9a0) at /Volumes/Shared/Source/WebKit/OpenSource/WebCore/page/FrameView.cpp:1226
#14 0x00000001009e8aab in -[WebFrame(WebInternal) _drawRect:contentsOnly:] (self=0x1073443b0, _cmd=0x7fff84c4cc8e, rect={origin = {x = 0, y = 0}, size = {width = 1009, height = 1011}}, contentsOnly=1 '\001') at /Volumes/Shared/Source/WebKit/OpenSource/WebKit/mac/WebView/WebFrame.mm:575
#15 0x0000000100a0d3e0 in -[WebHTMLView drawSingleRect:] (self=0x1073534e0, _cmd=0x7fff8353b7c0, rect={origin = {x = 0, y = 0}, size = {width = 1009, height = 1011}}) at /Volumes/Shared/Source/WebKit/OpenSource/WebKit/mac/WebView/WebHTMLView.mm:3017
#16 0x0000000100a068e8 in -[WebHTMLView drawRect:] (self=0x1073534e0, _cmd=0x7fff87f1a490, rect={origin = {x = 0, y = 0}, size = {width = 1009, height = 1011}}) at /Volumes/Shared/Source/WebKit/OpenSource/WebKit/mac/WebView/WebHTMLView.mm:3060
#17 0x00007fff878f56ef in -[NSView _drawRect:clip:] ()
#18 0x00007fff878f4392 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#19 0x0000000100a09b91 in -[WebHTMLView(WebPrivate) _recursiveDisplayAllDirtyWithLockFocus:visRect:] (self=0x1073534e0, _cmd=0x7fff87f1b064, needsLockFocus=1 '\001', visRect={origin = {x = 0, y = 0}, size = {width = 1009, height = 1011}}) at /Volumes/Shared/Source/WebKit/OpenSource/WebKit/mac/WebView/WebHTMLView.mm:1307
#20 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#21 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#22 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#23 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#24 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#25 0x00007fff878f46fc in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#26 0x00007fff878f2b51 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#27 0x00007fff878f3a05 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#28 0x00007fff878f3a05 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#29 0x00007fff878f24b7 in -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#30 0x00007fff878eecd4 in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] ()
#31 0x00007fff8783f142 in -[NSView displayIfNeeded] ()
Comment 8 Simon Fraser (smfr) 2009-02-27 14:54:17 PST
The assertion fires because you have to call frameCount() before m_haveFrameCount is true. Who'd a thunk?
Comment 9 Jan Alonzo 2009-02-28 05:16:21 PST
Comment on attachment 28100 [details]
Adds some comments and asserts

Hi Adam

>      virtual void startAnimation(bool /*catchUpIfNecessary*/ = true) { }
> diff --git a/WebCore/platform/graphics/cairo/ImageCairo.cpp b/WebCore/platform/graphics/cairo/ImageCairo.cpp
> index 2850488..d4dd8ce 100644
> --- a/WebCore/platform/graphics/cairo/ImageCairo.cpp
> +++ b/WebCore/platform/graphics/cairo/ImageCairo.cpp
> @@ -60,6 +60,7 @@ BitmapImage::BitmapImage(cairo_surface_t* surface, ImageObserver* observer)
>      , m_repetitionCountStatus(Unknown)
>      , m_repetitionsComplete(0)
>      , m_isSolidColor(false)
> +    , m_checkedForSolidColor(false)
>      , m_animationFinished(true)
>      , m_allDataReceived(true)
>      , m_haveSize(true)

Should we also set m_checkedForSolidColor to true once BitmapImage::checkForSolidColor has been called for the Cairo port?

Without this it will trigger the assert in line 220 of BitmapImage.h

Thanks
Comment 10 Adam Treat 2009-03-02 07:18:10 PST
Created attachment 28172 [details]
New version that removes wrong assert

It seems the assert is bogus since it is set immediately if 'frameCount()' is called which it is inside of 'checkForSolidColor' so I removed it.  Also, I made sure the Cairo version sets the m_checkedForSolidColor flag as it was overlooked.
Comment 11 Adam Roben (:aroben) 2009-03-02 07:30:39 PST
Comment on attachment 28172 [details]
New version that removes wrong assert

It's a shame to make mayFillWithSolidColor non-const. Can we make m_checkedForSolidColor mutable and make checkForSolidColor const, too?

r=me with the change you described to mayFillWithSolidColor on IRC.
Comment 12 Adam Treat 2009-03-02 07:37:22 PST
Landed again with r41358.