Closed
Bug 391583
Opened 17 years ago
Closed 17 years ago
gfxPlatform::DoesARGBImageDataHaveAlpha is slow
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: vlad, Assigned: vlad)
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
pavlov
:
review+
vlad
:
approval1.9+
|
Details | Diff | Splinter Review |
DoesARGBImageDataHaveAlpha is somewhat slow, and it's a Tp hit on any page with a lot of images that don't have alpha -- even though that's specifically the case that it wants to optimize rendering for later. We should instead pass this information up from the decoders; they're already dealing with the image data anyway, so they should be able to figure out if the image really has any pixels with non-opaque alpha or not.
Assignee | ||
Updated•17 years ago
|
Summary: gfxPlatform::DoesARGBImageDataHaveAlpha → gfxPlatform::DoesARGBImageDataHaveAlpha is slow
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → vladimir
Assignee | ||
Comment 1•17 years ago
|
||
Here's a patch. I think that the XBM decoder could also get the 1-bit alpha handling, but who uses XBM anyway?
Attachment #276045 -
Flags: review?(pavlov)
Comment 2•17 years ago
|
||
Comment on attachment 276045 [details] [diff] [review]
get rid of DoesARGBImageDataHaveAlpha
modules/libpr0n/decoders/png/nsPNGDecoder.h
>--- a/modules/libpr0n/decoders/png/nsPNGDecoder.h Mon Aug 06 16:57:02 2007 -0700
>+++ b/modules/libpr0n/decoders/png/nsPNGDecoder.h Thu Aug 09 16:44:56 2007 -0700
>@@ -93,6 +93,7 @@ public:
> PRUint8 apngFlags;
> PRUint8 mChannels;
> PRPackedBool mError;
>+ PRBool mFrameHasNoAlpha;
> };
>
> #endif // nsPNGDecoder_h__
PRPackedBool would save save a tiny amount of memory.
Assignee | ||
Comment 3•17 years ago
|
||
I thought about that, but wouldn't PRPackedBool be a tiny bit slower to write? I'm not so sure about that, so if someone knows, please let me know :) It's being set often as part of a tight loop.
Comment 4•17 years ago
|
||
It would be slower on some architectures, particularly anything RISC based.
You mean this kind of tight loop?
for (PRUint32 x=width; x>0; --x) {
*cptr32++ = GFX_PACKED_PIXEL(line[3], line[0], line[1], line[2]);
+ if (line[3] != 0xff)
+ decoder->mFrameHasNoAlpha = PR_FALSE;
line += 4;
}
You could use a local PRBool in the method if you are worried and assign to the member PRPackedBool outside the loop. You could create an (inner?) class that had a PRBool and assigned it from the PRPackedBool in its constructor and wrote it back to the PRPackedBool in the destructor.
Assignee | ||
Comment 5•17 years ago
|
||
Updated patch based on feedback -- uses a local variable to keep track while iteration, assigning to the PRPackedBool as appropriate at the end. Also handles XBM.
Attachment #276045 -
Attachment is obsolete: true
Attachment #276223 -
Flags: review?(pavlov)
Attachment #276045 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #276223 -
Flags: review?(pavlov) → review+
Comment 6•17 years ago
|
||
Comment on attachment 276223 [details] [diff] [review]
updated patch
er, sorry, can you just get rid of the setHasNoAlpha on the gfxIImageFrame and just call through to the nsIImage using the interface requestor stuff we already use?
Attachment #276223 -
Flags: review+ → review-
Assignee | ||
Comment 7•17 years ago
|
||
Updated
Attachment #276223 -
Attachment is obsolete: true
Attachment #276532 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #276532 -
Flags: review?(pavlov) → review+
Comment 8•17 years ago
|
||
Note that XBM images are always transparent, being a 1bit image format, with black and transparent pixels. See also bug 376471.
Comment 9•17 years ago
|
||
One of the comments in nsThebesImage.cpp is a little off grammatically:
>+// A hint from the image decoders that this image has no alpha, even
>+// though we created is ARGB32.
"is" => "it as"?
Assignee | ||
Updated•17 years ago
|
Attachment #276532 -
Flags: approval1.9+
Comment 10•17 years ago
|
||
XBM's really don't need to check for transparent pixels, as they are by definition transparent, as they are 1bit images, with 1's representing black and 0's transparent. So they are always transparent.
GIF's also don't need this check, as they format provides a bit telling that the iamge is transparent, and image editors only set that bit (and the 'tpixel' value) when the image really has a transparent pixel in the colormap. Note furthermore, that the image format for GIF's is correctly set to be RGB24 resp ARGB32 for non-trans resp trans images.
Checking for non-transparency for all images, is a costly thing to for just detecting those rare cases where the GIF is lying about its transparency.
Note, that the bug 343655 that introduced DoesARGBImageDataHaveAlpha also reports that it didn't bring the expected performance improvement.
Assignee | ||
Comment 11•17 years ago
|
||
No, it wasn't a Tp win on the tinderboxes, but was a perf win on some machines (e.g. like mine). But sure, we can remove the check from GIF/XBM.
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•