198293 - support alpha channel for .ico images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

http://storm.campus.luth.se/kevel/favicon.ico is an image that's supposed to
have an alpha channel. the ico decoder does not support that yet.

e.g. microangelo can be used to create such an image -
ftp://daxim.ath.cx/muangelo.png


I am not aware of a spec for this.
http://msdn.microsoft.com/library/en-us/dnwui/html/msdn_icons.asp?frame=true
says: "Also remember that the AND mask is a monochrome DIB, with a color depth
of 1 bpp." which kinda contradicts the validness of above ico...

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like one is supposed to just throw away the "AND-Mask" of the .ico and use
the 4th color bit of each pixel (currently ignored by the ico decoder) as the
alpha value, where 0=transparent 255=opaque.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 3

[[e-mail]]

Comment on attachment 119623 [details] [diff] [review]
patch

>-                  if (mBIH.bpp == 32)
>-                    p++; // Padding byte
>+                  if (mBIH.bpp == 32) {
>+                    *alphaPos++ = *p++; // Alpha value
>+                  }
Nit: keep existing brace style :-P

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 119623 [details] [diff] [review]
patch

clearing review request, I found a problem with this patch

neil: thanks, fixed it locally

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

this should be better

Comment 6

[ArronM (:paper)]

I've looked at the patch, I just need to find time to test it.

Comment 7

[[e-mail]]

Any reason why http://wp.netscape.com/favicon.ico displays correctly on my Linux
build but not in my Win32 nightly? AFAIK they're both 24-bit displays.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

neil: because of a bug in the ico decoder :) please file a new bug for that...
the decoder should zero out the r/g/b data when the pixel is transparent. please
don't assign that bug to me, though :)

Comment 9

[[e-mail]]

Sprinkling some keywords because Netscape uses 32-bit favicons.

Comment 10

[Samir Gehani]

adt: nsbeta1-

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 204340 has been marked as a duplicate of this bug. ***

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 206801 has been marked as a duplicate of this bug. ***

Comment 13

[Jo Hermans]

*** Bug 210397 has been marked as a duplicate of this bug. ***

Comment 14

[Scott MacGregor]

Christian, do you need an sr for this bug? I can if you want. I'd like to see
this get fixed for thunderbird.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 119842 [details] [diff] [review]
patch v2

scott: that would be cool. I'll ask tor for review, then.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in - thanks for the reviews

Comment 17

[Stephen Walker]

*** Bug 214027 has been marked as a duplicate of this bug. ***

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: testcase

attaching favicon from url here so it won't get lost

Comment 19

[Kevin Newman]

Isn't this fixed already?

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #19)
> Isn't this fixed already?

yes it is. I added the testcase to allow testing further patches to the ico
decoder against previously reported bugs, to make sure not to regress them.