The mac icon decoder is broken under cocoa-cairo. No icons draw (see unknown content dialog). It works fine under cocoa-quickdraw.

In nsIconDecoder.cpp, "mFrame->GetAlphaBytesPerRow(&abpr);", abpr is 64 for a 16x16 image with 1 bit alpha. That is almost certainly incorrect. It should be 16, and it is when using gfx/src/mac instead of gfx/src/thebes. PRInt32 nsThebesImage::GetLineStride() { return mStride; } PRInt32 nsThebesImage::GetAlphaLineStride() { return (mAlphaDepth > 0) ? mStride : 0; } Why would GetAlphaLineStride() return the same thing as GetLineStride() if the image has alpha and then 0 vs. mStride when the image does not have alpha?

Hmm, so this could be the cause of https://bugzilla.mozilla.org/attachment.cgi?id=246323&action=view (bug 361568)?

Actually doing mFrame->GetAlphaBytesPerRow(&abpr); to get the size of the 'alphabits' row for the image data fed through the pipe is wrong. In the old world this happened to be the same, but the size of the alphabytes in the frame is not the same as the format of the bits sent through the pipe. The format of the data through the pipe (from the OS specific decoders) is: PRUint8 width; PRUint8 height; PRUint8 alphaBits; (1 or 8) PRUint8 imageBytes; (width * height * 3) (each pixel is (PRUint8 R, PRUint8 G, PRUint8 B)) PRUint8 alphaBytes; (when alphaBits is 1: packed bits, otherwise 1 byte per pixel). So the size of an alphaBytes row in this format is depending on alphaBits. In the old gfx world, the mFrame would have the same format for alphaBits (set by the format), but in Cairo RGB_A1 and RGB_A8 are both mapped to ARGB (4 bytes) format. So, actually the simple fix is to fix this alphaBytesRow calculation within nsIconDecoder (as it is only for this internal pipe format). The better fix, if really everything will be Cairo, is to update this internal format to the Cairo format of passing PRUint32 per pixel (ARGB), so that there is no need for transcoding from the internal pipe format to the format of Cairo.

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

Actually: For Cairo, in nsIconDecoder we can directly read the data from the inputStream into the Image databuffer, as the os-dependent decoders (such as for gtk) all produce the imagedata already in Cairo format. See attachment. This saves a buffer allocation, and image data copying, and also makes the code simpler. Patch coming up

Note, it is not immediately obvious from the patch, but this patch removes any calls to GetAlphaBytesPerRow and its underlying GetAlphaLineStride which is plain wrong in Cairo. (actually thebes should define GetAlphaLineStride as not implemented/valid).

(In reply to comment #8) > Note, it is not immediately obvious from the patch, but this patch removes any > calls to GetAlphaBytesPerRow and its underlying GetAlphaLineStride which is > plain wrong in Cairo. (actually thebes should define GetAlphaLineStride as not > implemented/valid). Is there a bug filed on doing that?

nsIImage and gfxIImageFrame are both on the to-kill list so their behavior shouldn't be too important here.

checked in

Problems with this patch that unfortunately nobody (including me) caught before checkin: /Users/josh/src/mozilla/ff_cairo/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp: In member function 'virtual nsresult nsIconDecoder::WriteFrom(nsIInputStream*, PRUint32, PRUint32*)': /Users/josh/src/mozilla/ff_cairo/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp:156: error: 'width' was not declared in this scope /Users/josh/src/mozilla/ff_cairo/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp:156: error: 'height' was not declared in this scope /Users/josh/src/mozilla/ff_cairo/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp:161: error: no matching function for call to 'nsDerivedSafe<gfxIImageFrame>::GetImageData(PRUint8**, PRUint8*)' ../../../../dist/include/gfx/gfxIImageFrame.h:124: note: candidates are: virtual nsresult gfxIImageFrame::GetImageData(PRUint8**, PRUint32*)

bustage fixed (hopefully)

Another fix was required to fix (hopefully :) sunbird linux/mac bustage due to all the #ifdefs.

For what it's worth, I tested that the Mac icon decoder did work under cairo when I tested and landed bug 333253, so I'm not sure why this was needed to get the icon decoder working unless something else broke since then. (And I'm pretty sure I was really using the code, since testing on Mac actually caught bugs in the initial revision of the patch.) Could you please get bugs filed, and blocking1.9? for either fixing the broken APIs or removing them?

Oh, and the argo tinderbox on http://tinderbox.mozilla.org/Firefox/ is still orange following this checkin.

Backed out due to tinderbox orange. Judging from the bustage fixes that had to land, this clearly wasn't even compiled, never mind tested.

And, for confirmation, this was the cause of the orange.

Also, note that at the time the bug was filed, the icon decoder was broken in general under cairo -- that was fixed in bug 333253.

Also removed the non-cairo stuff, as it is no longer needed (and also removed from other image decoders), and to clear the confusion around the ifdef's.

I just tried the latest nightly on 10.3.9 (06-Feb-2007 05:23). The patch that was checked in does not seem to fix bug 361568 (that was duped to this one).

(In reply to comment #17) > Backed out due to tinderbox orange. Oops...

OK, some new info: I updated the relevant file (dbarons back-out), applied attachment #254150 [details] [diff] [review] and did a "make clean && make" in libpr0n. The patch doesn't seem to change the broken icons reported in bug 361568. Note: I'm building with the 3.0 sdk on 10.3.9.

Actually, this patch only fixes the nsIconDecoder.cpp part. But the mac/nsIconChannel.cpp part also needs fixing, as it generates the icon data fed into nsIconDecoder.cpp in the old (gfx) format. Fixing mac/nsIconChannel.cpp should not be to difficult. For the record, the new format is: PRUint8 width PRUint8 height PRUint32 pixels[width*height] Whereby each pixel is a Cairo packed/premultiplied pixel (ARGB)(see Cairo specs).

mac/nsIconChannel.cpp shouldn't be used anymore for cairo builds. We use nsIconChannelCocoa.mm.

Also, bug 361568 doesn't have anything to do with the icon decoder. I reopened it and commented further there.

could someone add a screen grab or something to demonstrate what the issue is? "The mac icon decoder is broken under cocoa-cairo. No icons draw (see unknown content dialog)." what icons aren't being drawn?

As dbaron said in comment #15, the original issue for this bug has been fixed. Alfred's patch is followup to "read image directly from inputstream to image".

Comment on attachment 256700 [details] [diff] [review] Updated to the recent Cairo cleanup + if (mObserver) + mObserver->OnStartDecode(nsnull); why the nullcheck? urg, I see, the rest of this file nullchecks too. sigh.

Who can do the checkin?

I was going to check this in but it doesn't compile on Mac OS X trunk. In file included from nsIconDecoder.cpp:50: nsIImage.h:44:21: error: gfxRect.h: No such file or directory nsIImage.h:204: error: expected ',' or '...' before '&' token nsIImage.h:205: error: ISO C++ forbids declaration of 'gfxRect' with no type

Changing bug summary and dependencies to reflect what this bug has morphed into (despite my protests).

Note, do we still need to fix mac/nsIconChannel.cpp ? If so, it needs a separate bug.

Does this need a re-review? And the new patches should indeed probably have gone into a separate bug...

Comment on attachment 262994 [details] [diff] [review] V6: Include 'thebes' in the Makefile for dependencies... Can you check this out and in? Only the Makefile.in is changed since previous patch version

Since this waits for a review, getting it off the [checkin needed] radar.

Comment on attachment 262994 [details] [diff] [review] V6: Include 'thebes' in the Makefile for dependencies... Tor, can you approve this one? cbiesinger did already do the SR, but the Makefile.in change was lost from the patch, so it failed to compile. This is corrected in the latest patch

Who can do the checkin?

Checking in modules/libpr0n/decoders/icon/Makefile.in; /cvsroot/mozilla/modules/libpr0n/decoders/icon/Makefile.in,v <-- Makefile.in new revision: 1.24; previous revision: 1.23 done Checking in modules/libpr0n/decoders/icon/nsIconDecoder.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconDecoder.cpp,v <-- nsIconDecoder.cpp new revision: 1.28; previous revision: 1.27 done

"Note, do we still need to fix mac/nsIconChannel.cpp" Please file another bug about investigating this. I don't want to forget to look. I don't know exactly what you mean by "fix" otherwise I'd file it myself.

If the non-Cocoa based Mac icon decoder is still needed, we need to fix the mac/nsIconChannel.cpp, as described in comment #24. Marking this bug as fixed.