668068 - ICO width/height of 0 should mean 256 width/height

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

Apparently (according to Wikipedia and Chrome's source code), icon files that have a width or height set to 0 in the header should have that direction set to 256. Bug 619048 contains a couple of examples of this.

> 127-sized ICOs are very spottily supported, though; for example, the Windows shell supports displaying them just fine, but both IE and Windows Photo Viewer can't view them.

Comment 1

[Rain [:rain1] (inactive)]

Yes, [1] is as authoritative as it gets about this.

[1] http://blogs.msdn.com/b/oldnewthing/archive/2010/10/18/10077133.aspx

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests

The reftests are a little bigger beause of 256x256, let me know if you want me to kick out a few of the tests on the BPP tests.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 546611 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests

> 
>+  // Obtains the width of the icon directory entry
>+  PRUint32 GetRealWidth() const
>+  {
>+    return mDirEntry.mWidth == 0? 256 : mDirEntry.mWidth; 
>+  }
>+
>+  // Obtains the height of the icon directory entry
>+  PRUint32 GetRealHeight() const
>+  {
>+    return mDirEntry.mHeight == 0? 256 : mDirEntry.mHeight; 
>+  }
>+

It would be better to just store the real height and do this translation at decode time.

Comment 4

[Brian R. Bondy [:bbondy]]

It is an PRUint8 field in the BMP header so cannot store 256.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #3)
> Comment on attachment 546611 [details] [diff] [review] [review]
> Patch for 256x256 ICO decoder, encoder, and reftests
> 

Also, it seems like these png files could be much smaller:

convert -strip ico-size-256x256-1bpp.png a.png

-rw-r--r--  1 jrmuizel  staff  17197 18 Jul 16:20 ico-size-256x256-1bpp.png
-rw-r--r--  1 jrmuizel  staff   7156 18 Jul 16:21 a.png

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #4)
> It is an PRUint8 field in the BMP header so cannot store 256.

Ah, I didn't realize we were trying to preserve the structure layout.

Comment 7

[Brian R. Bondy [:bbondy]]

Oh nice about the file sizes.  I'll apply a strip to those PNGs once Joe reviews and submit a new patch with any of his comments + those.

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests

Same as last patch but with stripped (convert -strip) and optimized (optipng) PNG images.

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 546769 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests

Review of attachment 546769 [details] [diff] [review]:
-----------------------------------------------------------------

Other than formatting, looks good!

::: modules/libpr0n/decoders/nsICODecoder.cpp
@@ +256,5 @@
>                  (mCurrIcon*sizeof(mDirEntryArray))) {
>        mCurrIcon++;
>        ProcessDirEntry(e);
> +      if (((e.mWidth == 0? 256 : e.mWidth) == PREFICONSIZE 
> +          && (e.mHeight == 0? 256 : e.mHeight) == PREFICONSIZE && 

GetRealWidth/GetRealHeight, right?

(Also && on prev line, as long as you're in here)

Comment 10

[Brian R. Bondy [:bbondy]]

GetReadWidth/GetRealHeight operates on the dir entry that we end up using and assigning to mDirEntry.

This part of code is in a loop going through each of the dir entry items, so it needs to be like so.

I will fix the formatting though.

Comment 11

[Joe Drew (not getting mail)]

Also add a comment then :)

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes

Same as last patch but rebased to work with the new series of patches.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Rebased patch for 256x256 ICO decoder, encoder, and reftests

Needed another rebasing due to review comments on BMP decoder patch.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Rebased patch for 256x256 ICO decoder, encoder, and reftests

Rebased patch to work with Bug 670466's and Bug 549468's latest patch.

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 550724 [details] [diff] [review]
Rebased patch for 256x256 ICO decoder, encoder, and reftests

Review of attachment 550724 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/encoders/ico/nsICOEncoder.h
@@ +104,5 @@
>    // or if no encoding options specified will use the default (PNG)
>    nsCOMPtr<imgIEncoder> mContainedEncoder;
>  
>    // These headers will always contain endian independent stuff 
> +  // Don't trust the width and height of mICODirEntry directly, use accessors

maybe specify the names of these accessors

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests

Same as last patch which was r+ just a comment change, so just marking as r+

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch for 256x256 ICO decoder, encoder, and reftests. v2

Rebased, will push to try when tree opens again.

Comment 19

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fcb3a88198ee

Comment 20

[Brian R. Bondy [:bbondy]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4da52874a049

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4da52874a049