With patch, all image decoders uses the same pattern: use the image buffer of Cairo to decode directly to.

Comment on attachment 260576 [details] [diff] [review] Write directly to Cairo image buffer please use tabs in the makefile like everything else

Comment on attachment 260576 [details] [diff] [review] Write directly to Cairo image buffer + mFrame->GetImageData((PRUint8**)&mImageData, &imageLen); I'd use C++ casts here + *ar++ = NS_CAIRO_PIXEL((pixel & 1)?255:0, 0, 0, 0); I can't find a definition for NS_CAIRO_PIXEL in lxr, where is it? please add spaces around the ? and the : why are you counting down from numPixels rather than up? So this patch breaks incremental rendering of XBM files?

1. C++ casts or C casts, it is just what one's preference is. 2. NS_CAIRO_PIXEL is now GFX_PACKED_PIXEL. 3. Only the loop counter goes down (which is faster) but image decoding is still incremental, in the order that pixels come in. 4. Adding spaces for the ? / 0; but possibly I will use a different way to do this more nicely. New patch coming up...

(In reply to comment #3) > 3. Only the loop counter goes down (which is faster) Why's that faster? > but image decoding is still incremental, in the order that pixels come in. Yes, but you don't call OnDataAvailable anymore escape at the end of decoding, right?

that "escape" should've been "except"

This patch re-enables OnDataAvailable per row. It also fixes the NS_CAIRO_PIXEL name. (Note, a loop counting down to zero is faster because it doesn't have to compare the loopcounter to the endvalue)

Comment on attachment 276006 [details] [diff] [review] V3: Make OnDataAvailable row-based again >--- modules/libpr0n/decoders/xbm/Makefile.in 9 Dec 2004 19:28:23 -0000 1.7 >+++ modules/libpr0n/decoders/xbm/Makefile.in 9 Aug 2007 18:51:16 -0000 >@@ -45,15 +45,17 @@ include $(DEPTH)/config/autoconf.mk > > MODULE = imgxbm > LIBRARY_NAME = imgxbm_s > FORCE_STATIC_LIB = 1 > MODULE_NAME = nsXBMModule > LIBXUL_LIBRARY = 1 > > REQUIRES = xpcom \ >+ string \ > gfx \ >+ thebes \ > imglib2 \ > $(NULL) Nit: fix spacing.

This patch is needed to fix bug 367281, to get rid of the ugly SetImageData/SetAlphaData from gfxImageFrame. SetImageData/SetAlphaData don't work as expected (and could even crash) in the Cairo based Gecko 1.9, and it would be wrong to keep these methods exposed in the official Gecko 1.9 release.

this won't block the release but I will approve the patch once it gets r/sr=

This patch has already r/sr (see v1/v2). The last version addressed the comment from the SR.

Stuart, as this patch has r/sr, can you give your approval1.9?

Comment on attachment 278216 [details] [diff] [review] V4: Fix alignment in Makefile.in This patch is quite bitrot and refuses to apply on current trunk. Can you attach an unbitrot patch for check-in?

Checking in modules/libpr0n/decoders/xbm/Makefile.in; /cvsroot/mozilla/modules/libpr0n/decoders/xbm/Makefile.in,v <-- Makefile.in new revision: 1.9; previous revision: 1.8 done Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp,v <-- nsXBMDecoder.cpp new revision: 1.24; previous revision: 1.23 done Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.h; /cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.h,v <-- nsXBMDecoder.h new revision: 1.9; previous revision: 1.8 done

A bit of the patch was dropped in the fixing of the bitrot in the patch. XBM images don't display at all now.

Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp,v <-- nsXBMDecoder.cpp new revision: 1.25; previous revision: 1.24 done

Verifed in build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100405 Minefield/3.0a9pre. Thanks for the quick checkin!