Bug 59041 – Skia port should implement BitmapImage::checkForSolidColor()

• Component: New Bugs
• Status: RESOLVED FIXED

[Pratik Solanki]

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.

[Stephen White]

See http://code.google.com/p/chromium/issues/detail?id=80011 -- same issue, i think. If so, pls coordinate w/twiz.

[Mike Reed]

possible duplicate of http://code.google.com/p/chromium/issues/detail?id=80011

[John Knottenbelt]

See also related bug https://bugs.webkit.org/show_bug.cgi?id=58926

[Jeff Timanus]

Created attachment 91563 [details]
Patch

[Jeff Timanus]

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?

[James Robinson]

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?

[Jeff Timanus]

Created attachment 91590 [details]
Patch

[Jeff Timanus]

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.

[Mike Reed]

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
}

[Jeff Timanus]

Created attachment 91678 [details]
Patch

[Jeff Timanus]

Created attachment 91686 [details]
Patch

[Jeff Timanus]

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.

[Eric Seidel (no email)]

Comment on attachment 91686 [details]
Patch

YES!  This is a huge win for some pages.

[WebKit Commit Bot]

Comment on attachment 91686 [details]
Patch

Clearing flags on attachment: 91686

Committed r85422: <http://trac.webkit.org/changeset/85422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.