376471 - Make XBM decoding also write directly to Cairo image buffer

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

Attachment: Write directly to Cairo image buffer

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

Comment 1

[Stuart Parmenter]

Comment on attachment 260576 [details] [diff] [review]
Write directly to Cairo image buffer

please use tabs in the makefile like everything else

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 3

[Alfred Kayser]

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...

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

(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?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

that "escape" should've been "except"

Comment 6

[Alfred Kayser]

Attachment: V3: Make OnDataAvailable row-based again

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 7

[Alfred Kayser]

Attachment: Test images for XBM decoding

Comment 8

[tor]

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.

Comment 9

[Alfred Kayser]

Attachment: V4: Fix alignment in Makefile.in

Comment 10

[Alfred Kayser]

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.

Comment 11

[Stuart Parmenter]

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

Comment 12

[Alfred Kayser]

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

Comment 13

[Alfred Kayser]

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

Comment 14

[Reed Loden [:reed]]

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?

Comment 15

[Alfred Kayser]

Attachment: V5: unbitrotted version

Comment 16

[Reed Loden [:reed]]

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

Comment 17

[Alfred Kayser]

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

Comment 18

[Alfred Kayser]

Attachment: Patch to add back the missing OnStartFrame call

Comment 19

[Reed Loden [:reed]]

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

Comment 20

[Alfred Kayser]

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!