Last Comment Bug 731824 - App icons on windows 7/XP for native apps appear to be showing a black background
: App icons on windows 7/XP for native apps appear to be showing a black backgr...
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: unspecified
: x86_64 Windows 7
: -- minor
: Firefox 14
Assigned To: Nobody; OK to take it and work on it
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-29 15:20 PST by Jason Smith [:jsmith]
Modified: 2016-02-04 15:00 PST (History)
6 users (show)
jsmith: in‑moztrap+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Black Background Icons Win 7 (21.66 KB, image/png)
2012-02-29 15:20 PST, Jason Smith [:jsmith]
no flags Details

Description Jason Smith [:jsmith] 2012-02-29 15:20:51 PST
Created attachment 601789 [details]
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] 2012-02-29 15:50:06 PST
Also: I've seen this issue on Windows XP.
Comment 2 Mark Hammond [:markh] 2012-04-04 22:21:11 PDT
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] 2012-04-04 22:22:36 PDT
sorry - to clarify, I meant "It appears bug 687062 is adding support *for transparency* when decoding icons,
Comment 4 Mark Hammond [:markh] 2012-04-04 23:50:38 PDT
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] 2012-04-05 04:59:57 PDT
> * 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] 2012-04-05 20:35:01 PDT
(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 me!) 2012-04-05 22:18:28 PDT
Thanks a lot Mark for the thorough investigation. I'll incorporate this change in the patch over bug 731541.
Comment 8 :Felipe Gomes (needinfo me!) 2012-04-19 22:46:35 PDT
Fixed in bug 731541
Comment 9 Jason Smith [:jsmith] 2012-06-21 18:18:59 PDT
MozTrap +. Already covered by windows shortcut MozTrap test case.

Note You need to log in before you can comment on or make changes to this bug.