Closed Bug 391583 Opened 17 years ago Closed 17 years ago

gfxPlatform::DoesARGBImageDataHaveAlpha is slow

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

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.
Summary: gfxPlatform::DoesARGBImageDataHaveAlpha → gfxPlatform::DoesARGBImageDataHaveAlpha is slow
Assignee: nobody → vladimir
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 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.
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.
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.

Attached patch updated patch (obsolete) — Splinter Review
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)
Attachment #276223 - Flags: review?(pavlov) → review+
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-
Updated
Attachment #276223 - Attachment is obsolete: true
Attachment #276532 - Flags: review?(pavlov)
Attachment #276532 - Flags: review?(pavlov) → review+
Note that XBM images are always transparent, being a 1bit image format, with black and transparent pixels. See also bug 376471.
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"?
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. 
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.
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.