360000 - transparency in favicon not shown

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061108 Minefield/3.0a1 ID:2006110810 [cairo]

repro:
1.open url

result:
no transparency, it should show only a red "O" not a white background

The favicon is send by the server (no link in the html source) so I don't know what's so special about it that is fails.
It works as expected in a non-cairo build

Comment 1

[Brian Polidoro]

The URL of the favicon is:
http://teletekst.nos.nl/favicon.ico

When I save the icon to my windows desktop it is transparent except for the circle.  But opening it in IE7 has the same effect as minefield.  It shows the white when opening the icon in the content area and changing the default background color to something other than white.

But two programs I looked at the icon with (XnView and Visual Studio 2002) don't show the icon as being transparent.  But Eye of Gnome in Ubuntu does show the transparency but I don't see how to view the other versions of the icon.  

Comment 2

[Stuart Parmenter]

yeah there is a problem with transparency on some images -- i'll fix it after some of the 1.9alpha bugs

Comment 3

[Steve England [:stevee]]

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

Comment 4

[Alfred Kayser]

I have a working patch to fix the transparancy decoding for ICO and BMP images in Cairo.

Comment 5

[Alfred Kayser]

Attachment: V1: Fix transparancy, remove buffers, and many other optimizations

Detailed explanation to follow soon

Comment 6

[Alfred Kayser]

Attachment: V2: Correctly last-minute typo and removed not-used return value from WriteRLE

Comment 7

[Alfred Kayser]

Overview of the patch:
Cairo: instead of doing SetImageData, use GetImageData to get the pointer to the image data and set the image data directly.
Cairo: Get rid of image buffers and alpha buffers
Cairo: Set transparency directly into the right place in the image
Both: Get rid of row buffers
Both: Optimize CalcBit functions
Both: Use GetUInt32 inline function instead of the LITTLE_TO macro's
Both: Only call OnDataAvailable per batch of rows instead of for every row.
Both: Header decoding: only decode the values really used.
Both: nsICODecoder: do alpha decoding in one step, instead of two steps.

This saves rowBuffer allocation for both codepaths, and other buffers for Cairo. Also the alpha decoding for ICO and BMP is faster and really works.

Comment 8

[Alfred Kayser]

Testcases:
http://home.unet.nl/alfredkayser/ico.zip (use xxx.html to show all icons)
http://home.unet.nl/alfredkayser/bmp.zip

Comment 9

[Stuart Parmenter]

Comment on attachment 249618 [details] [diff] [review]
V2: Correctly last-minute typo and removed not-used return value from WriteRLE

looks great. good job!

Comment 10

[tor]

Can you separate out your changes into three bugs/patches (more if you feel it make for a more natural split):

   1) whatever the fix for this bug was
   2) removing extra buffers
   3) miscellaneous code improvements

Comment 11

[Alfred Kayser]

Attachment: Patch to just fix the transparancy issue

Tor,

This patch only addresses the transparency thing for favicon. 
The issue only happened for 32bit icons so with real alpha transparency.
The solution is thus premultiplication!

Comment 12

[Alfred Kayser]

Attachment: Patch to just fix the transparancy issue

Tor,

This patch only addresses the transparency thing for favicon. 
The issue only happened for 32bit icons so with real alpha transparency.
The solution is thus premultiplication!

This patch also makes sure the mAlphaBuffer is not allocaed for Cairo (as it is not used anyway), and that SetPixel really gets inlined in the right way (compensating for the PREMULTIPLY overhead)

Comment 13

[Alfred Kayser]

Attachment: Testcase: 32bit ico with alpha transparency

Comment 14

[tor]

Comment on attachment 251104 [details] [diff] [review]
Patch to just fix the transparancy issue

Thanks for splitting out the pertinent chunks.

Comment 15

[Nickolay_Ponomarev]

mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.h    1.23
mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp  1.40
mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.h    1.14

Comment 16

[Alfred Kayser]

Verified in build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070117 Minefield/3.0a2pre

Verified with http://teletekst.nos.nl/ and with testcase from above (save as exmatrikulationsamt.de.ico)

Comment 17

[aja+bugzilla]

Seems this may have cause regression to file folder icon's color in download manager (bug 367275 filed).