Last Comment Bug 668068 - ICO width/height of 0 should mean 256 width/height
: ICO width/height of 0 should mean 256 width/height
Status: RESOLVED FIXED
[mentor=joe@drew.ca]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 549468 600556
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-28 15:36 PDT by Joe Drew (not getting mail)
Modified: 2011-09-22 17:44 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for 256x256 ICO decoder, encoder, and reftests (277.19 KB, patch)
2011-07-18 13:01 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for 256x256 ICO decoder, encoder, and reftests (213.09 KB, patch)
2011-07-19 06:33 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review
Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes (213.56 KB, patch)
2011-07-25 20:11 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes (213.57 KB, patch)
2011-07-26 21:11 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Rebased patch for 256x256 ICO decoder, encoder, and reftests (212.54 KB, patch)
2011-07-30 14:23 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Rebased patch for 256x256 ICO decoder, encoder, and reftests (212.47 KB, patch)
2011-08-04 09:23 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review
Patch for 256x256 ICO decoder, encoder, and reftests (212.66 KB, patch)
2011-08-16 14:03 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Patch for 256x256 ICO decoder, encoder, and reftests. v2 (212.63 KB, patch)
2011-09-21 07:55 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description User image Joe Drew (not getting mail) 2011-06-28 15:36:10 PDT
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 User image Siddharth Agarwal [:sid0] (inactive) 2011-07-07 14:14:24 PDT
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 User image Brian R. Bondy [:bbondy] 2011-07-18 13:01:16 PDT
Created attachment 546611 [details] [diff] [review]
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 User image Jeff Muizelaar [:jrmuizel] 2011-07-18 13:17:38 PDT
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 User image Brian R. Bondy [:bbondy] 2011-07-18 13:19:19 PDT
It is an PRUint8 field in the BMP header so cannot store 256.
Comment 5 User image Jeff Muizelaar [:jrmuizel] 2011-07-18 13:30:38 PDT
(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 User image Jeff Muizelaar [:jrmuizel] 2011-07-18 13:31:16 PDT
(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 User image Brian R. Bondy [:bbondy] 2011-07-18 13:44:21 PDT
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 User image Brian R. Bondy [:bbondy] 2011-07-19 06:33:36 PDT
Created attachment 546769 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests

Same as last patch but with stripped (convert -strip) and optimized (optipng) PNG images.
Comment 9 User image Joe Drew (not getting mail) 2011-07-25 13:49:49 PDT
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 User image Brian R. Bondy [:bbondy] 2011-07-25 19:11:53 PDT
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 User image Joe Drew (not getting mail) 2011-07-25 19:23:33 PDT
Also add a comment then :)
Comment 12 User image Brian R. Bondy [:bbondy] 2011-07-25 20:11:35 PDT
Created attachment 548358 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes
Comment 13 User image Brian R. Bondy [:bbondy] 2011-07-26 21:11:46 PDT
Created attachment 548682 [details] [diff] [review]
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 User image Brian R. Bondy [:bbondy] 2011-07-30 14:23:32 PDT
Created attachment 549600 [details] [diff] [review]
Rebased patch for 256x256 ICO decoder, encoder, and reftests

Needed another rebasing due to review comments on BMP decoder patch.
Comment 15 User image Brian R. Bondy [:bbondy] 2011-08-04 09:23:35 PDT
Created attachment 550724 [details] [diff] [review]
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 User image Joe Drew (not getting mail) 2011-08-15 14:02:27 PDT
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 User image Brian R. Bondy [:bbondy] 2011-08-16 14:03:50 PDT
Created attachment 553580 [details] [diff] [review]
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 User image Brian R. Bondy [:bbondy] 2011-09-21 07:55:08 PDT
Created attachment 561466 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests. v2

Rebased, will push to try when tree opens again.
Comment 19 User image Brian R. Bondy [:bbondy] 2011-09-21 10:47:34 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fcb3a88198ee
Comment 20 User image Brian R. Bondy [:bbondy] 2011-09-22 06:46:12 PDT
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4da52874a049
Comment 21 User image Ed Morley [:emorley] 2011-09-22 17:44:51 PDT
https://hg.mozilla.org/mozilla-central/rev/4da52874a049

Note You need to log in before you can comment on or make changes to this bug.