225083 - moz-icon urls show wrongly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

build from a few days ago, but with neil's patch for Bug 222694, shows
moz-icon://.htm?size=16 urls rather wrongly. I'll attach a screenshot

Comment 1

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

Attachment: screenshot

Comment 2

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

hm, I should've used a less lossy format than jpeg. anyway, the screenshot
doesn't make it much uglyer.

Comment 3

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

this was with 16bpp (1024x768, nvidia geforce2 ti)

Comment 4

[[e-mail]]

What's going on here is that the icon channel is ignoring the bitfields on
16-bit bitmaps although the icon decoder can't handle them anyway...

Comment 5

[[e-mail]]

Attachment: Rough patch

I know this patch does no error-checking, but it should be good for testing.

Comment 6

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

hm, have you tried compiling this patch? I get
c:/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp(269) : error
C2039: 'biHeight' : Is no element of 'tagBITMAPINFO'

Comment 7

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

simply removing the line with that error and recompiling isn't exactly an
improvement

Comment 8

[[e-mail]]

Attachment: Alternative approach

OK, so this way should ensure that I don't query for strange icon types.

Comment 9

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

this new patch does fix the bug

Comment 10

[Ere Maijala (slow)]

Comment on attachment 135145 [details] [diff] [review]
Alternative approach

Change the compression to BI_RGB and add a comment about the required double
height. With these changes, r=ere

Comment 11

[[e-mail]]

Attachment: Better patch

OK, so this fixes 16-bit and 32-bit displays by forcing them to 24-bit.

Comment 12

[[e-mail]]

Sigh... somehow I left out the double height code and comment...
 if (GetDIBits(hDC, iconInfo.hbmColor, 0, colorInfo.bmiHeader.biHeight, buffer +
sizeof(ICONFILEHEADER) + sizeof(ICONENTRY) + colorInfoSize, lpBitmapInfo,
DIB_RGB_COLORS)) {
+   // doubling the height because icons contain two bitmaps
+   lpBitmapInfo->bmiHeader.biHeight <<= 1;

Comment 13

[[e-mail]]

Attachment: diff -w

Comment 14

[[e-mail]]

Fix checked in.