The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: bbondy)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=joe@drew.ca])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=joe@drew.ca]
Yes, [1] is as authoritative as it gets about this.

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

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Updated

6 years ago
Depends on: 549468, 600556
(Assignee)

Comment 2

6 years ago
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.
Attachment #546611 - Flags: review?(joe)
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.
(Assignee)

Comment 4

6 years ago
It is an PRUint8 field in the BMP header so cannot store 256.
(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
(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.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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.
Attachment #546611 - Attachment is obsolete: true
Attachment #546769 - Flags: review?(joe)
Attachment #546611 - Flags: review?(joe)
(Reporter)

Comment 9

6 years ago
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)
Attachment #546769 - Flags: review?(joe) → review+
(Assignee)

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
Also add a comment then :)
(Assignee)

Comment 12

6 years ago
Created attachment 548358 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests with formatting fixes
Attachment #546769 - Attachment is obsolete: true
Attachment #548358 - Flags: review?(joe)
(Assignee)

Comment 13

6 years ago
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.
Attachment #548358 - Attachment is obsolete: true
Attachment #548682 - Flags: review?(joe)
Attachment #548358 - Flags: review?(joe)
(Assignee)

Comment 14

6 years ago
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.
Attachment #548682 - Attachment is obsolete: true
Attachment #549600 - Flags: review?(joe)
Attachment #548682 - Flags: review?(joe)
(Assignee)

Comment 15

6 years ago
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.
Attachment #549600 - Attachment is obsolete: true
Attachment #550724 - Flags: review?(joe)
Attachment #549600 - Flags: review?(joe)
(Reporter)

Comment 16

6 years ago
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
Attachment #550724 - Flags: review?(joe) → review+
(Assignee)

Comment 17

6 years ago
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+
Attachment #550724 - Attachment is obsolete: true
Attachment #553580 - Flags: review+
(Assignee)

Comment 18

6 years ago
Created attachment 561466 [details] [diff] [review]
Patch for 256x256 ICO decoder, encoder, and reftests. v2

Rebased, will push to try when tree opens again.
Attachment #553580 - Attachment is obsolete: true
Attachment #561466 - Flags: review+
(Assignee)

Comment 19

6 years ago
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fcb3a88198ee
(Assignee)

Comment 20

6 years ago
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4da52874a049
https://hg.mozilla.org/mozilla-central/rev/4da52874a049
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.