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]
:
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 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 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 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 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 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 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 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 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 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 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 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 Joe Drew (not getting mail) 2011-07-25 19:23:33 PDT
Also add a comment then :)
Comment 12 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 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 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 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 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 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 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 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 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 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.