Bug 24227 – Implement checkForSolidColor in ImageQt.cpp

• Component: Images
• Status: RESOLVED FIXED

[Adam Treat]

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.

[Adam Treat]

Created attachment 28075 [details]
Implement

[Eric Seidel (no email)]

Comment on attachment 28075 [details]
Implement

LGTM

[Adam Treat]

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.

[Eric Seidel (no email)]

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.

[Adam Treat]

Created attachment 28100 [details]
Adds some comments and asserts

[Adam Treat]

Landed with r41295.

[Anders Carlsson]

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] ()

[Simon Fraser (smfr)]

The assertion fires because you have to call frameCount() before m_haveFrameCount is true. Who'd a thunk?

[Jan Alonzo]

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

[Adam Treat]

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.

[Adam Roben (:aroben)]

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.

[Adam Treat]

Landed again with r41358.