731824 - App icons on windows 7/XP for native apps appear to be showing a black background

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Jason Smith [:jsmith]]

Attachment: Black Background Icons Win 7

Steps:

1. Install the latest build of the extension
2. Go to apps.mozillalabs.com/appdir
3. Install an app natively
   a. Examples with bug: Favimon, BarFight

Expected:

The shortcut icon for the native app should have a transparent background.

Actual:

A black background is shown instead of a transparent background. See attached screenshot.

Comment 1

[Jason Smith [:jsmith]]

Also: I've seen this issue on Windows XP.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

The way I read nsICOEncoder.cpp, there is no support for transparency there - the "and" image is hard-coded such that every pixel is opaque.  It appears bug 687062 is adding support for decoding icons, but that isn't what we need here.

I'm not that familiar with the imglib stuff, so I'm adding bbondy to the CC list as he seems to be in the know about such things and might be able to offer some insights.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

sorry - to clarify, I meant "It appears bug 687062 is adding support *for transparency* when decoding icons,

Comment 4

[Mark Hammond [:markh] [:mhammond]]

So, I *think* this could be addressed by:

* Initializing the BMP encoder with 32 bytes per pixel, meaning the encoder will give us the alpha channel.

* In the ICO encoder we'd drop the memcpy of the image data and instead loop over the pixels, sticking the rgb data in the "real" buffer and the alpha channel in the "and" buffer (although IIUC, the "and" buffer is a simple byte-mask whereas the alpha data is a byte value, so I guess we'd check for the alpha channel being < 128 when setting the "and" buffer)

* Adjust uses of BMPImageBufferSize to account for this (ie, BMPImageBufferSize would then be bigger due to the alpha bytes)

Does that sound sane?

Comment 5

[Brian R. Bondy [:bbondy]]

> * Initializing the BMP encoder with 32 bytes per pixel, meaning the encoder will give us the alpha channel.

Yes you should use 32 bytes per pixel, we don't support encoding of alpha masks.

> * In the ICO encoder we'd drop the memcpy of the image data and 
> instead loop over the pixels, sticking the rgb data in the "real" 
> buffer and the alpha channel in the "and" buffer (although IIUC, the 
> "and" buffer is a simple byte-mask whereas the alpha data is a byte value, 
> so I guess we'd check for the alpha channel being < 128 when setting the "and" buffer)

You shouldn't need to deal this low level, you should have an image container and then encode it as an icon using the icon mime type (image/vnd.microsoft.icon) and its encoder options (format=bmp;bpp=32).  TimA I think has already did some work here you might want to ping him as well.

> mMimeTypeOfInputData.AssignLiteral("image/vnd.microsoft.icon"); 
> rv = imgtool->EncodeImage(container, mMimeTypeOfInputData, 
>     NS_LITERAL_STRING("format=bmp;bpp=32"), getter_AddRefs(iconStream));

You can find a more complete example in this patch or in the jump list code: 
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=110894&attachment=611183

> * Adjust uses of BMPImageBufferSize to account for this (ie,
> BMPImageBufferSize would then be bigger due to the alpha bytes)

ditto above, you shouldn't need to worry about these details.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)

Awesome, thanks!  The problem is indeed solved simply by specifying bpp=32 (we previously only specified the format but not the bpp=).  I guess Windows ignores the AND mask when a true 32bit bitmap is found.

Once WebappsInstaller.jsm is checked in, we just need to change:

      iconStream = imgTools.encodeImage(imgContainer.value,
                                        "image/vnd.microsoft.icon",
                                        "format=bmp");

to:

      iconStream = imgTools.encodeImage(imgContainer.value,
                                        "image/vnd.microsoft.icon",
                                        "format=bmp;bpp=32");

and this bug goes away!

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Thanks a lot Mark for the thorough investigation. I'll incorporate this change in the patch over bug 731541.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Fixed in bug 731541

Comment 9

[Jason Smith [:jsmith]]

MozTrap +. Already covered by windows shortcut MozTrap test case.