transparency in favicon not shown

VERIFIED FIXED

Status

()

VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Peter6, Assigned: alfredkayser)

Tracking

Trunk
x86
Windows 2000
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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

12 years ago
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. ***
(Assignee)

Comment 4

12 years ago
I have a working patch to fix the transparancy decoding for ICO and BMP images in Cairo.
(Assignee)

Updated

12 years ago
Blocks: 331298
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Blocks: 318699
(Assignee)

Comment 5

12 years ago
Created attachment 249505 [details] [diff] [review]
V1: Fix transparancy, remove buffers, and many other optimizations

Detailed explanation to follow soon
Assignee: nobody → alfredkayser
(Assignee)

Comment 6

12 years ago
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
(Assignee)

Comment 7

12 years ago
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.
(Assignee)

Comment 8

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

Comment 9

12 years ago
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+
(Assignee)

Updated

12 years ago
Attachment #249618 - Flags: superreview?(tor)

Comment 10

12 years ago
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
(Assignee)

Comment 11

12 years ago
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!
Attachment #249618 - Attachment is obsolete: true
Attachment #251103 - Flags: superreview?(tor)
Attachment #249618 - Flags: superreview?(tor)
(Assignee)

Comment 12

12 years ago
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)
(Assignee)

Updated

12 years ago
Attachment #251103 - Attachment is obsolete: true
Attachment #251103 - Flags: superreview?(tor)
(Assignee)

Comment 13

12 years ago
Created attachment 251108 [details] [diff] [review]
Testcase: 32bit ico with alpha transparency

Comment 14

12 years ago
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+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
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]
(Assignee)

Comment 16

12 years ago
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

Updated

12 years ago
Depends on: 367275

Comment 17

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

Updated

12 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.