Bug 59041

Summary: Skia port should implement BitmapImage::checkForSolidColor()
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: New BugsAssignee: Mike Reed <reed>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jamesr, jknotten, psolanki, reed, senorblanco, twiz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Pratik Solanki 2011-04-20 17:01:21 PDT
The implementation of the above functions is

void BitmapImage::checkForSolidColor()
{
    m_checkedForSolidColor = true;
}

This should be correctly implemented so it sets m_isSolidColor to true when you have 1x1 image with 1 frame so it can make use os optimization in Image.cpp that does a color fill when you're drawing a 1x1 image.
Comment 1 Stephen White 2011-04-21 06:18:12 PDT
See http://code.google.com/p/chromium/issues/detail?id=80011 -- same issue, i think. If so, pls coordinate w/twiz.
Comment 2 Mike Reed 2011-04-21 08:20:10 PDT
possible duplicate of http://code.google.com/p/chromium/issues/detail?id=80011
Comment 3 John Knottenbelt 2011-04-28 01:52:38 PDT
See also related bug https://bugs.webkit.org/show_bug.cgi?id=58926
Comment 4 Jeff Timanus 2011-04-28 15:02:51 PDT
Created attachment 91563 [details]
Patch
Comment 5 Jeff Timanus 2011-04-28 15:04:36 PDT
Comment on attachment 91563 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:471
> +        WebCore::NativeImageSkia* frame = frameAtIndex(currentFrame());

Is there a possibility that I need to check for a decode complete status here as well?
Comment 6 James Robinson 2011-04-28 15:20:46 PDT
Comment on attachment 91563 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:466
> +    if (frameCount() != 1)

the other ports seem to implement this check as frameCount() > 1. is there any significance to this being different?

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:471
>> +        WebCore::NativeImageSkia* frame = frameAtIndex(currentFrame());
> 
> Is there a possibility that I need to check for a decode complete status here as well?

it seems like this codepath wouldn't work if the bitmap was not decoded yet or if insufficient data was available to decode the image, right?  the other ports null check the image here, at least.

can you see in what cases the null check fails in the other ports and see what happens here?

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:472
> +        frame->lockPixels();

don't we have a pixel autolocker class to avoid the manual lock/unlock?
Comment 7 Jeff Timanus 2011-04-28 16:54:28 PDT
Created attachment 91590 [details]
Patch
Comment 8 Jeff Timanus 2011-04-28 17:02:15 PDT
Comment on attachment 91563 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:466
>> +    if (frameCount() != 1)
> 
> the other ports seem to implement this check as frameCount() > 1. is there any significance to this being different?

I was trying to be more defensive - in the case that frameCount could be 0.  Uploaded a new patch, following the style of the other ports.

>>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:471
>>> +        WebCore::NativeImageSkia* frame = frameAtIndex(currentFrame());
>> 
>> Is there a possibility that I need to check for a decode complete status here as well?
> 
> it seems like this codepath wouldn't work if the bitmap was not decoded yet or if insufficient data was available to decode the image, right?  the other ports null check the image here, at least.
> 
> can you see in what cases the null check fails in the other ports and see what happens here?

Added a check for null in a new patch.  The other ports do not check for completion of decoding of the image.  I'm not familiar with this code - is it even possible to receive an incomplete bitmap at this point?

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:472
>> +        frame->lockPixels();
> 
> don't we have a pixel autolocker class to avoid the manual lock/unlock?

Uploaded new patch using the autolocker.
Comment 9 Mike Reed 2011-04-29 04:48:31 PDT
Fringe case, you'll want to check that there are valid pixels after the lock call.

if (bitmap.getPixels() == NULL) {
    // there are no pixels allocated, or the lock failed
}
Comment 10 Jeff Timanus 2011-04-29 07:47:49 PDT
Created attachment 91678 [details]
Patch
Comment 11 Jeff Timanus 2011-04-29 08:25:31 PDT
Created attachment 91686 [details]
Patch
Comment 12 Jeff Timanus 2011-04-29 08:26:51 PDT
Comment on attachment 91686 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:477
> +        m_solidColor = Color(frame->getColor(0, 0));

Removed the clunky construction of Color from individual components, as per recommendation from Stephen White.
Comment 13 Eric Seidel (no email) 2011-05-01 10:47:06 PDT
Comment on attachment 91686 [details]
Patch

YES!  This is a huge win for some pages.
Comment 14 WebKit Commit Bot 2011-05-01 11:35:18 PDT
Comment on attachment 91686 [details]
Patch

Clearing flags on attachment: 91686

Committed r85422: <http://trac.webkit.org/changeset/85422>
Comment 15 WebKit Commit Bot 2011-05-01 11:35:24 PDT
All reviewed patches have been landed.  Closing bug.