Summary: | Skia port should implement BitmapImage::checkForSolidColor() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||||||
Component: | New Bugs | Assignee: | 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
Pratik Solanki
2011-04-20 17:01:21 PDT
See http://code.google.com/p/chromium/issues/detail?id=80011 -- same issue, i think. If so, pls coordinate w/twiz. possible duplicate of http://code.google.com/p/chromium/issues/detail?id=80011 See also related bug https://bugs.webkit.org/show_bug.cgi?id=58926 Created attachment 91563 [details]
Patch
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 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? Created attachment 91590 [details]
Patch
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. 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 } Created attachment 91678 [details]
Patch
Created attachment 91686 [details]
Patch
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 on attachment 91686 [details]
Patch
YES! This is a huge win for some pages.
Comment on attachment 91686 [details] Patch Clearing flags on attachment: 91686 Committed r85422: <http://trac.webkit.org/changeset/85422> All reviewed patches have been landed. Closing bug. |