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
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.
yeah there is a problem with transparency on some images -- i'll fix it after some of the 1.9alpha bugs
*** Bug 361741 has been marked as a duplicate of this bug. ***
I have a working patch to fix the transparancy decoding for ICO and BMP images in Cairo.
Created attachment 249505 [details] [diff] [review] V1: Fix transparancy, remove buffers, and many other optimizations Detailed explanation to follow soon
Assignee: nobody → alfredkayser
Created attachment 249618 [details] [diff] [review] V2: Correctly last-minute typo and removed not-used return value from WriteRLE
Attachment #249505 - Attachment is obsolete: true
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.
Testcases: http://home.unet.nl/alfredkayser/ico.zip (use xxx.html to show all icons) http://home.unet.nl/alfredkayser/bmp.zip
Comment on attachment 249618 [details] [diff] [review] V2: Correctly last-minute typo and removed not-used return value from WriteRLE looks great. good job!
Attachment #249618 - Flags: review+
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
Created attachment 251103 [details] [diff] [review] 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!
Created attachment 251104 [details] [diff] [review] 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)
Attachment #251104 - Flags: superreview?(tor)
Comment on attachment 251104 [details] [diff] [review] Patch to just fix the transparancy issue Thanks for splitting out the pertinent chunks.
Attachment #251104 - Flags: superreview?(tor) → superreview+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
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)
Status: RESOLVED → VERIFIED
Seems this may have cause regression to file folder icon's color in download manager (bug 367275 filed).
You need to log in before you can comment on or make changes to this bug.