600556 - Support Vista-style ICO files

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Rimas Kudelis [use needinfo to ping]]

Attachment: The "classic" ico file

Windows Vista introduced a new possibility in its ico files: it allows using PNG-encoded image data instead of classic ICO data, which results in dramatic file size decrease. Can we perhaps support those new icons in nsICODecoder?

You could say that I should simply use a PNG file instead, but I have a counter-argument: PNG files can't contain multiple image resources, while ICO files can and usually do. This means that I can create a favicon image containing optimized pre-rendered icons of different resolutions, from stupid 1x1 pixel up to 256x256.

For an illustration, I'm attaching three icons, all having the same four image resources. favicon-classic.ico is the "classic" format we already support, favicon-png.ico has all four images encoded in PNG, and favicon-optimal.ico has three bigger images in PNG, while the 16x16px image is in classic format, for backwards compatibility with older browsers.

Comment 1

[Rimas Kudelis [use needinfo to ping]]

Attachment: Vista-style PNG-compressed ico file

Comment 2

[Rimas Kudelis [use needinfo to ping]]

Attachment: Mixed ico file: contains one classic resource and three PNG-compressed resources

Comment 3

[Bobby Holley (:bholley)]

This isn't something I'm going to get to anytime in the next couple of years, but I'd take a patch.

The code shouldn't be too complicated. It'd need to detect the new filetype at the beginning of nsICODecoder, and instantiate a special nsPNGDecoder under the hood.

It would also need the following:
1) To be written against some sort of spec, presumably one that MS published.
2) To have exhaustive test coverage, both of the png-style ICOs as well as more test coverage for the regular ICOs that we risk breaking.
3) To receive a security review from the security team

Comment 4

[Rimas Kudelis [use needinfo to ping]]

Weird, I couldn't find any up to date official documentation (spec) about the icon format. The article about icons on MSDN doesn't seem to have been updated for Vista. I found two sources with some info however:
http://en.wikipedia.org/wiki/ICO_%28file_format%29
http://msdn.microsoft.com/en-us/magazine/cc546571.aspx

Since the format is pretty simple, it's also easy to reverse engineer: it only took me to read the Wikipedia article to actually construct Attachment #479405 [details] from the first two attachments manually using Hex editor. Basically, a PNG resource contains the whole PNG image. if you strip the ICO header and all other resources, it becomes a perfectly valid PNG file.

The second source suggests that only the 256x256 icon should be compressed, however, both compressed attachments work fine for me with the default image viewer that comes with Windows 7, so I think it's safe to assume that those icons are valid. I suppose the suggestion itself was made with pre-Vista software (and Windows versions) in mind, which is irrelevant here.

As a web developer, I think that the approach I took in attachment #479405 [details] would currently be the best use for favicons (if it was fully supported by browsers), because it:
* is backwards compatible with old versions of IE running on old versions of Windows (16x16px icon is still uncompressed)
* can provide resources of optimal quality in different resolutions, which is not possible using a single PNG image
* provides higher quality image resources for uses outside address bar/tab header/window title etc (e.g. IE9 allows to pin a site to the taskbar, and uses a much bigger icon there)
* uses PNG compression for anything above 16x16, noticeably decreasing file size compared to legacy ICO file (the file is almost five times smaller).

With this comment I don't intend to push you to implement this. I just want to provide some more info and possible use-cases.

Comment 5

[Bobby Holley (:bholley)]

For clarification, the spec doesn't need to be from MS. We can write our own if they didn't document the feature. I'm just saying that it needs to be written against something more concrete than a hodgepodge of reference images.

If anyone wants to actively work on this, shoot me an email before you start hacking and we'll have a more thorough discussion of what needs to be done.

Comment 6

[Oskar Ivanić (:icecold)]

(In reply to comment #3)
> This isn't something I'm going to get to anytime in the next couple of years

Sorry for spamming here, but why do you say this? 

Why this can't be done in short time?

Comment 7

[Bobby Holley (:bholley)]

(In reply to comment #6)
> (In reply to comment #3)
> > This isn't something I'm going to get to anytime in the next couple of years
> 
> Sorry for spamming here, but why do you say this? 
> 
> Why this can't be done in short time?

It certainly could be. However, most active Mozilla developers (including myself) have a pretty long TODO list, and so working on a feature means saying "this is more important than everything else I could be working on." So far, I'm not convinced that this bug qualifies. Also, I'm not really active at the moment, and won't be until August.

At the same time, if somebody wanted to take the project on themselves, I'd certainly help guide them and answer any questions.

Comment 8

[Rimas Kudelis [use needinfo to ping]]

Attachment: Lame ICO specification

I'm not very good at this, but here's an attempt at a specification. Anybody with enough time and knowledge, feel free to correct or expand it.

Comment 9

[Rain [:rain1] (inactive)]

FTR, for bug 659788 I wrote a small Python script to wrap a PNG into an ICO.

Comment 10

[Brian R. Bondy [:bbondy]]

I'm starting on this task today.

Bobby Holley mentioned that there is a lot of duplicated code between nsBMPDecoder and nsICODecoder.

We should:
1. Merge the DIB decoding code from the two files into nsBMPDecoder
2. Make nsICODecoder a thin wrapper that only reads the ICO header, and then instantiates the appropriate nsBMPDecoder or nsPNGDecoder to which it pipes all further data.

Comment 11

[Brian R. Bondy [:bbondy]]

Regarding the refactoring mentioned in #1 of Comment 10...
The BMP Reference can be found here: 
http://www.daubnet.com/en/file-format-bmp

The common part of duplicated code inside nsICODecode is everything except for BITMAPFILEHEADER.  So everything starting from and including the BITMAPINFOHEADER.

Since BITMAPFILEHEADER is not included in ICO, I cannot use BITMAPFILEHEADER's Signature field to determine if it's a bitmap. 
I will therefore assume it is a bitmap unless a PNG signature is found. 

The PNG signature is specified in libpng here and is defined to be  { 137, 80, 78, 71, 13, 10, 26, 10 }:
http://www.libpng.org/pub/png/book/chapter08.html#png.ch08.div.2

Comment 12

[Jim Mathies [:jimm]]

Make sure and include tests for any changes/additions you make.

Comment 13

[Brian R. Bondy [:bbondy]]

Ya will do. 
I discussed that with Bobby Holley as well.

He mentioned there were no BMP tests and very few ICO tests, but that I should create a bunch of reftests (similar to the ones for png and jpeg) when I'm done.

Comment 14

[Rain [:rain1] (inactive)]

(In reply to comment #11)
> Regarding the refactoring mentioned in #1 of Comment 10...
> The BMP Reference can be found here: 
> http://www.daubnet.com/en/file-format-bmp
> 
> The common part of duplicated code inside nsICODecode is everything except
> for BITMAPFILEHEADER.  So everything starting from and including the
> BITMAPINFOHEADER.
> 
> Since BITMAPFILEHEADER is not included in ICO, I cannot use
> BITMAPFILEHEADER's Signature field to determine if it's a bitmap. 
> I will therefore assume it is a bitmap unless a PNG signature is found. 

Note that in the ICONDIRENTRY, PNGs have:
more than 8 bits per pixel, so bColorCount set to 0
1 colour "plane", so wPlanes set to 1
32 bpp, so wBitCount set to 32

Windows only supports 32-bpp ARGB [1], so we should do the same too.

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

Comment 15

[Brian R. Bondy [:bbondy]]

- Finished detection of if each contained icon resource is a BMP or PNG
- Finished basic untested refactoring of code so that ICO calls into BMP decoder, can display my test DIB icon as before
- Added in code so that PNG will be called if PNG signature is detected. I have successful rendering of a PNG icon via an <img> html tag

Currently fixing problems with transparency/alpha not working in fixed up code.
Old code for ICO is 518 lines, new code is 340 lines and supports BMP ICOs, PNG ICOs, and mixed ICOs. 

Tomorrow will be fixing the transparency issues and testing / refactoring more.
Also pending is to implement a bunch of ref tests for BMP and ICOs which contain BMP&PNG.

Comment 16

[Joe Drew (not getting mail)]

Brian, if you feel comfortable with it, uploading your work-in-progress patches can help - it lets us give you feedback on your changes, and makes sure that progress isn't lost if something happens to your computer.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch in progress supporting PNG icons and refactored ICO code

I attached the work in progress patch, please keep in mind it is mid development :)
Feedback is more than welcome.

- Was going to make the BMP code changes conditional depending on if a flag is set to ICON mode, to eliminate chance of regressions in BMP code
- The old ICO code only adds one frame of optimal size into the image container, I'd like to eventually make all frames added, but probably not in the context of this bug. 
- Will fix line lengths and other coding standard changes, will remove unneeded code

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch in progress supporting PNG icons and refactored ICO code, and fixes

Intermediate patch, but fixed a couple crashes and other things since my last intermediate patch:
- Fixed if loading .ico directly would crash
- Fixed if loading continuously (hold down F5) would eventually crash
- Fixed problem with re-init multiple times depending on how data was fed to WriteInternal
- Some extra code fixups, line lengths, and other coding style issues
- Changed ICO's contained decoders to pointers so we don't use up memory if not needed (we either use BMP or PNG but not both)
  May change to use a single nsDecoder pointer instead of having 2 and determining which to use.
Transparency/alpha issue not yet addressed

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Intermediate patch, with yet again more fixes

Let me know what you think, this patch is closer to final. 
Planned to do are the ref tests and any bug fixes found when implementing those reftests.

Intermediate patch, with yet again more fixes:
- Fixed transparency issues when 4th byte is used for transparency and also when there is an image mask, now also correctly specify the correct surface
- Changed BMP code so it won't be affected as much when run with normal bitmaps and not through icons
- Fixed bug with transparency not working due to incorrect dataoffset calculated field when using images with color tables (<= 8bpp)
- Fixed additional single black pixel problem on transparent 32bit icons

Comment 20

[Rain [:rain1] (inactive)]

FWIW I think you should also use the parameters I mentioned in comment 14 to verify that it's a PNG...

Comment 21

[Brian R. Bondy [:bbondy]]

Hi sid0,

Thanks for the Raymond Chen link.
Do you envision an extra check something like this?

if (png sig is a match 
    && (mDirEntry.mBitCount != 32 
    || mDirEntry.mPlanes != 1 || mDirEntry.mColorCount != 0)) {
  PostDataError();
  return;
}

Comment 22

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #20)
> FWIW I think you should also use the parameters I mentioned in comment 14 to
> verify that it's a PNG...

I can't find the source right now, but I've read somewhere that the information in the ICO header is merely a hint. The image itself should always be checked to determine the true info.

I thus don't think that the information stored in header should be trusted more than the actual bits of the image.

Comment 23

[Rain [:rain1] (inactive)]

Yes, I agree, but the header info check can certainly be done in addition. Brian, is there any possible way the first few bytes of a BMP's header could match the PNG signature?

Comment 24

[Brian R. Bondy [:bbondy]]

Rimas: 
>  but I've read somewhere that the information in the ICO header is merely a hint

I've read this too, I'll check the values in the PNG image itself and throw an error in that case instead of the dir entry info.

sid0:
> is there any possible way the first few bytes of a BMP's header could match the PNG signature?

I checked into this and it is not possible because the first byte of data is either:
1) The bih header size.  Valid value is only 40.
2) The first byte of the PNG signature. 

The PNG signature at the same spot would contain 137 for that byte.  So we can unambiguously determine we have a PNG if the PNG signature is found.  The old ICO decoder simply ignored that field by the way.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: 544524: Intermediate patch, with more fixes and 32bpp PNG check

Added in the 32bpp PNG check and some other minor coding style issues.

Remaining for this task is the reftests and to implement any other feedback given. I'll also add some info to the great ICO doc Rimas Kudelis provided.

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch for supporting Vista-style ICO files, and BMP/ICO refactoring

Additional fixes after running reftests.  I'll submit the reftests as a separate patch for ease of reading since there are a lot of them.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: reftests for bug600556.patch

Added 163 reftests for a pretty good coverage of BMPs, ICO BMPs, and ICO PNGs.
It may seem like a lot of ref tests added, but they all test different parts of the code.
Found and fixed some problems which are included in the base patch from the reftests

BMP: (64 tests)
  - various edge case sizes tests for 1bpp, 4bpp, 8bpp, 24bpp (15 per color depth)
  - Tests for corrupted BMP (4)
 
ICO-BMPs: (78 tests)
  - various edge case size tests for 1bpp, 4bpp, 8bpp, 24bpp, 32bpp (15 per color depth)
  - Tests for corrupted BMP inside of an ICO (3)

ICO-PNG: (19 tests)
  - various edge case tests for for PNG (15)
  - Tests for corrupted PNG inside of an ICO (3)
  - Tests for transparent PNG (1)

ICO-BMP+PNG: (1 test)
  - Tests for mixed PNG+BMP (1)

CUR: (1 test)
  - Tests to make sure we cna display a .cur file in an image tag

To run all ICO reftests:
TEST_PATH=modules/libpr0n/test/reftest/ico/reftest.list make -C $(OBJDIR) reftest


To run all BMP reftests:
TEST_PATH=modules/libpr0n/test/reftest/bmp/reftest.list make -C $(OBJDIR) reftest

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 544892 [details] [diff] [review]
reftests for bug600556.patch

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

::: modules/libpr0n/test/reftest/ico/ico-png/wrapper.html
@@ +2,5 @@
> +<html>
> +<head>
> +<title>Image reftest wrapper</title>
> +<style type="text/css">
> +    #image1 { background-color: rgb(10, 100, 250); }

This is nice. Can you put this through all the wrappers?

::: modules/libpr0n/test/reftest/ico/ico-png/xlfn0g04.png
@@ +1,3 @@
> +PNG
> +
> +

Be sure that this image is added as a binary file.

Comment 29

[Brian R. Bondy [:bbondy]]

Thanks for the review Joe, will do for both suggestions.

Comment 30

[Joe Drew (not getting mail)]

Comment on attachment 544891 [details] [diff] [review]
Patch for supporting Vista-style ICO files, and BMP/ICO refactoring

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

A good start! There are some minor formatting details, and I don't understand all of what nsBMPDecoder is doing, but some cleanup there should help.

The thing I'm most concerned about is driving the PNG and BMP decoder manually from within nsICODecoder. I'm sorry I didn't bring this up earlier; I was unfortunately slammed last week, and off on Monday.

I don't think we should do this manually, but instead create a way of driving a decoder by just passing it data, and let the regular decoder mechanics take hold. That would be less error-prone and more self-contained.

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +249,5 @@
>                  PostDecoderError(NS_ERROR_OUT_OF_MEMORY);
>                  return;
>              }
> +
> +

Extra blank line here.

@@ +251,5 @@
>              }
> +
> +
> +            rv = mImage->AppendFrame(0, 0, mBIH.width, real_height, 
> +                                     mFromICO? gfxASurface::ImageFormatARGB32

Should this not be mHaveAlphaData instead of mFromICO?

@@ +403,5 @@
> +                        else {
> +                          // We assume that 32bit doesn't have alpha data until
> +                          // we find a non-zero alpha byte. If we find such a
> +                          // byte, it means that all previous pixels are really
> +                          // clear (alphabyte=0). This working assumption

Is that true? Wouldn't it just mean *that* pixel is clear?

@@ +412,5 @@
> +                              // Clear previous pixels from current row to end
> +                              PRInt32 lineToClear = mCurLine;
> +                              if(lineToClear > 0) {
> +                                --lineToClear;
> +                              }

I don't understand why you're decrementing lineToClear here.

Also, space between if and ( please.

@@ +415,5 @@
> +                                --lineToClear;
> +                              }
> +                              memset(mImageData + lineToClear * mBIH.width, 0, 
> +                                (mBIH.height - lineToClear) * mBIH.width 
> +                                * sizeof(PRUint32));

Please line up the arguments directly under mImageData here.

@@ +431,5 @@
>                      }
>                      mCurLine --;
>                      if (mCurLine == 0) { // Finished last line
> +                        if (mFromICO) {
> +                          mDecodingAndMask = PR_TRUE;//break;

mDecodingAndMask - it's not clear to me what this is supposed to mean. Maybe a new name?

Also, //break seems like a left-over.

@@ +612,5 @@
>              }
>          }
>      }
> +
> +    if (mFromICO && mDecodingAndMask && !mHaveAlphaData) {

Can you put a comment at the top of this block describing what effect it makes? It's not clear to me, especially not in the inner loop of the if (rowSize == mRowBytes) block.

@@ +614,5 @@
>      }
> +
> +    if (mFromICO && mDecodingAndMask && !mHaveAlphaData) {
> +      // Calculate rowsize in DWORD's and then return in # of bytes
> +      PRUint32 rowSize = ((mBIH.width + 31) / 32)*4; // +31 to round up

Space between arguments and infix operator (*) here and throughout.

@@ +615,5 @@
> +
> +    if (mFromICO && mDecodingAndMask && !mHaveAlphaData) {
> +      // Calculate rowsize in DWORD's and then return in # of bytes
> +      PRUint32 rowSize = ((mBIH.width + 31) / 32)*4; // +31 to round up
> +      //if (mPos == (1 + mImageOffset + BITMAPINFOSIZE + mNumColors*4)) {

This seems like a leftover.

@@ +620,5 @@
> +      if (mPos == 1+mBFH.dataoffset) {
> +        mPos++;
> +        mRowBytes = 0;
> +        mCurLine = mBIH.height;
> +        mRow = (PRUint8*)realloc(mRow, rowSize);

Change realloc() to moz_realloc() so we're guaranteed fallibility.

@@ +639,5 @@
> +      while (mCurLine > 0 && aCount > 0) {
> +        PRUint32 toCopy = NS_MIN(rowSize - mRowBytes, aCount);
> +        if (toCopy) {
> +          memcpy(mRow + mRowBytes, aBuffer, toCopy);
> +          aCount -= toCopy;

Usually we try to not modify arguments; maybe make a copy here?

@@ +654,5 @@
> +          while (p < p_end) {
> +            PRUint8 idx = *p++;
> +            for (PRUint8 bit = 0x80; bit && decoded<decoded_end; bit >>= 1) {
> +              // Clear pixel completely for transparency.
> +              if (idx & bit) *decoded = 0;

Please no single-line ifs; makes it impossible to debug.

::: modules/libpr0n/decoders/nsBMPDecoder.h
@@ +108,5 @@
>                                 (((PRUint32) x) >> 24))
> +#define NATIVE32_TO_LITTLE(x) (((((PRUint32) x) & 0xFF) << 24) | \
> +  (((((PRUint32) x) >> 8) & 0xFF) << 16) | \
> +  (((((PRUint32) x) >> 16) & 0xFF) << 8) | \
> +  (((PRUint32) x) >> 24))

Please indent this similarly to the other two macros here; alternately, just define it as LITTLE_TO_NATIVE32.

@@ +183,5 @@
>      PRInt32 mCurPos;    ///< Index in the current line of the image
>  
> +    PRPackedBool mFromICO;
> +    PRPackedBool mHaveAlphaData;
> +    PRPackedBool mDecodingAndMask;

Can you document these booleans, and make sure they're packing efficiently (potentially by putting them at the end of the class)?

@@ +195,5 @@
>      /** Set mBIH from the raw data in mRawBuf, converting from little-endian
>       * data to native data as necessary */
>      void ProcessInfoHeader();
> +
> +  friend class nsICODecoder;

This doesn't look correctly indented.

::: modules/libpr0n/decoders/nsICODecoder.cpp
@@ +66,5 @@
>  // Actual Data Processing
>  // ----------------------------------------
>  
> +PRUint32
> +nsICODecoder::CalcAlphaRowSize() {

Can you put the { on its own line for functions, please?

@@ +212,5 @@
> +  // If we are within the first PNGSIGNATURESIZE bytes of the image data,
> +  // then we have either a BMP or a PNG.  We use the first PNGSIGNATURESIZE
> +  // bytes to determine which one we have.
> +  if (mCurrIcon == mNumIcons && mPos >= mImageOffset 
> +                  && mPos < mImageOffset + PNGSIGNATURESIZE)

end a line with && and then vertically line up mPos with mCurrIcon; do this globally please :)

::: modules/libpr0n/decoders/nsICODecoder.h
@@ +97,5 @@
>    PRUint16 mNumIcons;
>    PRUint16 mCurrIcon;
>    PRUint32 mImageOffset;
> +  nsAutoPtr<nsBMPDecoder> mBMPImage;
> +  nsAutoPtr<nsPNGDecoder> mPNGImage;

Do we need to have both BMP and PNG images at the same time?

::: modules/libpr0n/src/Decoder.h
@@ +192,5 @@
>    bool mSizeDecode;
>    bool mInFrame;
>    bool mDecodeDone;
>    bool mDataError;
> +  friend class nsICODecoder;

We definitely shouldn't special-case decoders here. I suspect you need this because of calling FinishInternal/etc; I'd rather us just create a way to write data to a given Decoder instance, and have it do its thing for us.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: reftests for bug600556.patch + comments

This is based on the reftest patch you r+. 

Changes:
- There was a minor syntax error in the parent reflist.list
before:
reftest ico/reftest.list
now:
include ico/reftest.list
- Removed the non binary png reftest as it was redundant.  We already test corrupted png inside a ICO in a different reftest that was properly detected as binary.  hg doesn't do anything special with binary files, so this was just a limitation of the patch file itself. 
- I added the background color on all of the wrappers except for the corrupted bmp ones because of the corrupted image icon when inside img tag.

Comment 32

[Brian R. Bondy [:bbondy]]

Attachment: 545579: reftests for bug600556.patch + comments

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 545580 [details] [diff] [review]
545579: reftests for bug600556.patch + comments

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

I prefer we put the same wrapper html file in all the different places so there's no accidents copying it in the future. Other than that, looks fine.

Comment 34

[Brian R. Bondy [:bbondy]]

By same did you mean a single wrapper HTML or the same content but multiple wrapper files in each dir?

The problem with transparency in the 2 "corrupted image" tests is that the broken image icon is itself partially transparent.  Perhaps for that one I could rename the wrapper html to noalphatest_wrapper.html or something like that?

Comment 35

[Joe Drew (not getting mail)]

I was thinking the same content but multiple wrapper files, one in each dir.

I'm actually not clear on how the reftests pass in that case, considering you're comparing them to about:blank.

Comment 36

[Brian R. Bondy [:bbondy]]

I'm a confused about that as well.  When I load it manually it shows the broken image icon with the background in the transparent parts.   When I load the broken image directly, it shows nothing.  So I would think it should fail whether or not there is a background color. 

With background color, the ref test fails.
Without background color the reftest passes. 

To make matters more confusing the PNG corrupted reftests do a background color and they pass. 

Maybe it needs a reftest-wait to fail always?  Maybe the PNG reftests are passing but shouldn't be?

Comment 37

[Joe Drew (not getting mail)]

Comment on attachment 545580 [details] [diff] [review]
545579: reftests for bug600556.patch + comments

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

::: modules/libpr0n/test/reftest/bmp/bmp-corrupted/wrapper.html
@@ +6,5 @@
> +<body>
> +<img id="image1">
> +<script>
> +  var imgURL = document.location.search.substr(1);
> +  document.images[0].src = imgURL;

Sorry, this is my failure to correctly review. You're setting the src on an image, which is inherently asynchronous, but the reftest framework is taking its snapshot once the page is done "loading," which in this case happens right after the script is finished executing, but before the image is loaded; instead of capturing the loaded or error-state image, it just captures the page's background.

What you need to do is add the reftest-wait class to the root element as described in http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt , then in the image's onload handler, remove that class from the element.

Comment 38

[Brian R. Bondy [:bbondy]]

OK understood, will fix.

Comment 39

[Brian R. Bondy [:bbondy]]

Regarding the reftest patch, after looking into it I found out more. 

onload doesn't fire but onerror does for the corrupted images. 
I don't think the reftest-wait class is needed on the html element in this case.
If it is needed wouldn't that make all current wrapper tests in imagelib wrong (even PNG ones)?  Since none are using onload on the image tag.  Comparing the data urls the tests look right though.  

After looking at the reftest error log data URIs, I found that the reason why my tests were failing for bmp wasn't related to the broken image icon but related to the BMP allowing images for invalid RLE compression values.  So this is why the PNG were passing but my BMP were failing with the background color. 

New patches coming.

Comment 40

[Brian R. Bondy [:bbondy]]

What I'll do is add the reftest-wait to all wrappers and clear the flag onerror and onload.  I don't think it hurts to add it even know the data URIs look right without it.

Comment 41

[Brian R. Bondy [:bbondy]]

Attachment: reftests for bug600556.patch

Here are the changes since the last reftest patch:

- Fixed wrapper html for all wrapped image reftests w/ using reftest-wait, all wrapper.html are the same now.
- Added a new reftest which tests invalid BMP compression values both in BMP and in ICO, before we had only tests for valid compression values but invalid BPP for those compression values. (old code failed these tests by the way)
- Completely transparent images for 32x32 was broken in my last patch, fixed
- Added transparency and partially transparent tests for each of the BPPs as this was broken in the last patch for some BPPs.
- Added non square test for each BMP and ICO in case bugs with confusing width and height

Comment 42

[Brian R. Bondy [:bbondy]]

Attachment: Patch for supporting PNG ICO files, and BMP/ICO code refactoring

Here are the changes since last patch, I implemented all of your comments as well.


- Refactored the code as you mentioned so we let the BMP decoder and PNG decoder drive all of that decoding completely without messing with its internals.
  - It is important for us though to read the BMP headers before passing them into the BMP decoder because some logic in the ICO decoder depends on values of the contained BMP. Also because the bitmap information header (BIH) must have a corrected height of /2 because the BMP decoder shouldn't know about the icon's AND mask.
  - I no longer do any manual modifications to the contained decoder's internals. I just drive data to them through Write.
  - Not using 'friend' keyword anywhere any more
- Fixed issue with images being rendered with invalid compression values (This was a bug in the original decoder too)
- Fixed issues with images being rendered with valid compression values but invalid BPPs (This was a bug in the original decoder too)
- Added support in the BMP decoder for BI_ALPHABITFIELDS compression type
- Moved out BMP AND mask stuff into the ICO decoder because it's part of the ICO spec and not the BMP spec
- Cleaned up, and commented a lot of the previously code.

Comment 43

[Brian R. Bondy [:bbondy]]

Attachment: Patch for supporting PNG ICO files, and BMP/ICO code refactoring

Rebased patch from head of mozilla-central (EnsureFrame instead of AppendFrame)

Comment 45

[Joe Drew (not getting mail)]

Comment on attachment 546387 [details] [diff] [review]
reftests for bug600556.patch

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

Comment 46

[Joe Drew (not getting mail)]

Comment on attachment 546748 [details] [diff] [review]
Patch for supporting PNG ICO files, and BMP/ICO code refactoring

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

Note for the future: this would have been easier to review if it was split into 3 or more patches (add Decoder infrastructure, refactor nsBMPDecoder, refactor nsPNGDecoder, use from nsICODecoder).

General formatting comment: always put spaces between binary operators: a + b, a * b, etc.

r- because I want to look at this again, but it's looking good!

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +110,5 @@
> +}
> +
> +// Obtains the internal output image buffer
> +PRUint32* 
> +nsBMPDecoder::GetImageData() {

{ on new line

@@ +127,5 @@
> +  // mBIH.image_size isn't always filled for BI_RGB so calculate it manually
> +  // The pixel array size is calculated based on extra 4 byte boundary padding
> +  PRUint32 rowSize = (mBIH.bpp * mBIH.width + 7) / 8; // +7 to round up
> +  if (rowSize % 4)
> +    rowSize += (4 - (rowSize % 4)); // Pad to DWORD Boundary

Put this comment above the if (). Also braces around the if body please.

@@ +129,5 @@
> +  PRUint32 rowSize = (mBIH.bpp * mBIH.width + 7) / 8; // +7 to round up
> +  if (rowSize % 4)
> +    rowSize += (4 - (rowSize % 4)); // Pad to DWORD Boundary
> +  PRInt32 pixelArraySize = rowSize * 
> +                           (mBIH.height > 0? mBIH.height : -1 * mBIH.height);

Why is height sometimes negative?

@@ +299,5 @@
> +        if (((mBIH.compression == BI_RLE8) && (mBIH.bpp != 8)) 
> +          || ((mBIH.compression == BI_RLE4) && 
> +              (mBIH.bpp != 4) && (mBIH.bpp != 1))
> +          || ((mBIH.compression == BI_ALPHABITFIELDS) && 
> +              (mBIH.bpp != 16) && (mBIH.bpp != 32))) {

Can you factor this out into a couple of different checks to simplify it?

Also, please put || and && at the end of the previous line rather than the beginning of the next line.

@@ +324,5 @@
>                  return;
>              }
> +            rv = mImage->EnsureFrame(0, 0, 0, mBIH.width, real_height, 
> +                                     mUseAlphaData? gfxASurface::ImageFormatARGB32
> +                                             : gfxASurface::ImageFormatRGB24,

For the ternary operator, please always put a space before the ?. Also, can you vertically line up the ? and :?

(Alternately, put it in an if block - I always prefer those to ternary operators.)

::: modules/libpr0n/decoders/nsBMPDecoder.h
@@ +227,5 @@
> +    // Reference: 
> +    // http://en.wikipedia.org/wiki/ICO_(file_format)#cite_note-9
> +    PRPackedBool mUseAlphaData;
> +    // Keeps track to make sure we don't repost the BMP frame twice
> +    PRPackedBool mFramePosted;

Why did we run into this issue?

::: modules/libpr0n/decoders/nsICODecoder.cpp
@@ +129,5 @@
> +// Signature 2 bytes 'BM'
> +// FileSize	 4 bytes File size in bytes
> +// reserved	 4 bytes unused (=0)
> +// DataOffset	 4 bytes File offset to Raster Data
> +void nsICODecoder::FillBitmapFileHeaderBuffer(PRInt8 *bfh) {

{ on new line

@@ +139,4 @@
>  
> +  // We subtract 4 because our BFH_LENGTH internally
> +  // is larger than the spec's to include the BIH size
> +  // Our BITMAPINFOSIZE matches the spec's 40 bytes

Why does this have to be the case? And if it absolutely must be the case, is it documented at the definition of BFH_LENGTH?

@@ +144,4 @@
>  
> +  // The color table is present only if BPP is <= 8
> +  if (mDirEntry.mBitCount <= 8)
> +  {

While I personally love this code format, our style dictates { on the same line as the if.

@@ +256,4 @@
>        mCurrIcon++;
>        ProcessDirEntry(e);
> +      if ((e.mWidth == PREFICONSIZE 
> +          && e.mHeight == PREFICONSIZE && e.mBitCount >= colorDepth)

&& on previous line

@@ +320,5 @@
> +      return;
> +    }
> +    mDataError = mContainedImage->HasDataError();
> +    if (mContainedImage->HasDataError()) {
> +      return;

Looks like copy and paste error here.

@@ +343,5 @@
> +    && mPos < mImageOffset + BITMAPINFOSIZE) {
> +
> +    // We didn't actually have a signature, so backfill what should 
> +    // have been in the mBIHraw
> +    memcpy(mBIHraw, mSignature, PNGSIGNATURESIZE);

What are we overwriting here?

@@ +361,5 @@
> +  if (!mIsPNG && mPos == mImageOffset + BITMAPINFOSIZE) {
> +    // We don't have a PNG We've copied the whole bitmap info header 
> +    // into the mBHIraw buffer
> +    // We are getting the BPP from the BIH header as it should be trusted 
> +    // over the one we have from the icon header

These comments need reformatting into sentences, I think.

@@ +376,5 @@
>  
> +    // The ICO format when containing a BMP does not include the 14 byte
> +    // bitmap file header. To use the code of the BMP decoder we need to 
> +    // generate this header ourselves and feed it to the BMP decoder.
> +    PRInt8 bfhBuffer[14];

We don't have a #define for the size of this header?

@@ +419,5 @@
> +    PRInt32 bmpDataEnd = mDirEntry.mImageOffset + BITMAPINFOSIZE + 
> +                         ((nsBMPDecoder*)mContainedImage.get())->GetCompressedImageSize() +
> +                         4 * GetNumColors();
> +
> +    // If we are feeding in the core image data (but not the ICO AND mask)

In my initial reading of this comment, it sounded like we would *never* send the AND mask in; I think that's incorrect, right?

@@ +461,4 @@
>  
> +        // Ensure memory has been allocated before decoding.
> +        NS_ASSERTION(mRow, "mRow is null");
> +        NS_ASSERTION(mImage, "mImage is null");

Switch these to NS_ABORT_IF_FALSE after doing a bit of an audit; we'll notice then :)

::: modules/libpr0n/decoders/nsPNGDecoder.h
@@ +57,5 @@
> +
> +// This is defined in the PNG spec as an invariant. We use it to
> +// do manual validation without libpng.
> +const PRUint8 pngSignatureBytes[] =
> +  { 137, 80, 78, 71, 13, 10, 26, 10 };

In general, I don't think this isn't going to work in a .h file - the same symbol is going to be defined in multiple places (wherever nsPNGDecoder.h is included). You'll probably need to make it a public static on nsPNGDecoder.

::: modules/libpr0n/src/Decoder.cpp
@@ +87,5 @@
>    InitInternal();
>    mInitialized = true;
>  }
>  
> +// Initializes a decoder who's aImage and aObserver is already being used by a

whose

::: modules/libpr0n/src/Decoder.h
@@ +64,5 @@
>    void Init(RasterImage* aImage, imgIDecoderObserver* aObserver);
>  
> +
> +  /**
> +   * Initializes a decoder who's aImage and aObserver is already being used by a

whose

Comment 47

[Brian R. Bondy [:bbondy]]

> Why is height sometimes negative?

Bitmaps are stored bottom up, but if you specify a negative height, then the decoder should do top-bottom.  The height is the absolute value of what is specified in the data.

I'll add a comment.

Comment 48

[Brian R. Bondy [:bbondy]]

> +    // Keeps track to make sure we don't repost the BMP frame twice
> +    PRPackedBool mFramePosted;
> Why did we run into this issue?

There was a bug in the original decoder if you wrote data to the decoder on the exact boundary of the bitmap headers.  The check for the next section to decode is the same as the previous check for EnsureFrame with mPos >= mLOH. 

I.e. before we had if (mPos == mLOH) for the EnsureFrame and the next section was if (mColors && (mPos >= mLOH && .  If you happened to call the function exactly on the boundary mPos == mLOH, mPos is not incremented at all and you get a second EnsureFrames on your next Write.  

So the boolean mFramePosted prevents that bug.

Comment 49

[Brian R. Bondy [:bbondy]]

> We subtract 4 because our BFH_LENGTH internally
> +  // is larger than the spec's to include the BIH size
> +  // Our BITMAPINFOSIZE matches the spec's 40 bytes
>
> Why does this have to be the case? And if it absolutely must be the case, is it > documented at the definition of BFH_LENGTH?

I think the original decoder did this since it's easier to put the size in that first file header since the next header/struct used depends on that field.

I'll make another define though so I don't have to put comments about subtracting 4 and just document the defines more.

Comment 50

[Brian R. Bondy [:bbondy]]

Attachment: Patch for supporting PNG ICO files, and BMP/ICO code (with review formatting changes)

Pretty much the same patch as before but with fixes mentioned in the review (formatting).

Comment 51

[Brian R. Bondy [:bbondy]]

I have been splitting up my later patches by the way and I will try to always split them up even more when possible.  Sorry about this big one.

Comment 52

[Joe Drew (not getting mail)]

(In reply to comment #48)
> > +    // Keeps track to make sure we don't repost the BMP frame twice
> > +    PRPackedBool mFramePosted;
> > Why did we run into this issue?
> 
> There was a bug in the original decoder if you wrote data to the decoder on
> the exact boundary of the bitmap headers.  The check for the next section to
> decode is the same as the previous check for EnsureFrame with mPos >= mLOH. 
> 
> I.e. before we had if (mPos == mLOH) for the EnsureFrame and the next
> section was if (mColors && (mPos >= mLOH && .  If you happened to call the
> function exactly on the boundary mPos == mLOH, mPos is not incremented at
> all and you get a second EnsureFrames on your next Write.  
> 
> So the boolean mFramePosted prevents that bug.

I don't really understand what you mean by "the original decoder." Can we fix that problem as a pre-patch rather than adding mFramePosted?

Comment 53

[Joe Drew (not getting mail)]

Comment on attachment 548443 [details] [diff] [review]
Patch for supporting PNG ICO files, and BMP/ICO code (with review formatting changes)

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

r- only because of mFramePosted.

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +137,5 @@
> +  // The height should be the absolute value of what the height is in the BIH.
> +  // If positive the bitmap is stored bottom to top, if negative the bitmap is
> +  // stored top to bottom.
> +  PRInt32 pixelArraySize = rowSize * 
> +                           (mBIH.height > 0 ? mBIH.height : -1 * mBIH.height);

You could always just say abs(mBIH.height).

@@ +301,5 @@
> +          PostDataError();
> +          return;
> +        }
> +
> +        // If we have RLE4 or RLE8  or BI_ALPHABITFIELDS, then ensure we

extra space before "or"

@@ +318,5 @@
> +        }
> +        if (mBIH.compression == BI_ALPHABITFIELDS && 
> +            mBIH.bpp != 16 && mBIH.bpp != 32) {
> +          PR_LOG(gBMPLog, PR_LOG_DEBUG, 
> +                 ("BMP ALPHABITFIELDS only supports 24 AND 32 bits per pixel\n"));

Something is wrong here - either the log message or the if condition.

::: modules/libpr0n/decoders/nsICODecoder.cpp
@@ +90,5 @@
> +    case 8:
> +      numColors = 256;
> +      break;
> +    default:
> +      numColors = (PRUint16)-1;

I'm a teeny bit concerned that you don't test for this case when calling GetNumColors. As far as I can tell it won't cause issues - you'll just end up with a larger allocation - but I'd like a carefully-crafted BMP/ICO as a crashtest to be sure.

@@ +146,5 @@
> +    dataOffset += 4 * numColors;
> +    fileSize = dataOffset + mDirEntry.mWidth * mDirEntry.mHeight;
> +  } else {
> +    fileSize = dataOffset + (mDirEntry.mBitCount * mDirEntry.mWidth
> +      * mDirEntry.mHeight) / 8;

Indentation here - * should be on previous line, mDirEntry.mHeight aligned with mDirEntry.mBitCount

@@ +317,5 @@
> +      return;
> +    }
> +    mPos += aCount;
> +    aCount = 0;
> +    aBuffer += aCount;

I know that it doesn't matter here, because we've dealt with all the data by writing it to our contained decoder, but I'd prefer the aCount zeroing out be _after_ aBuffer += aCount.

@@ +409,5 @@
> +
> +    // Feed the actual image data (not including headers) into the BMP decoder
> +    PRInt32 bmpDataOffset = mDirEntry.mImageOffset + BITMAPINFOSIZE;
> +    PRInt32 bmpDataEnd = mDirEntry.mImageOffset + BITMAPINFOSIZE + 
> +                         ((nsBMPDecoder*)mContainedImage.get())->GetCompressedImageSize() +

Can you use a static_cast where you cast to derived types? Keeps us from accidentally casting to unrelated types.

::: modules/libpr0n/decoders/nsICODecoder.h
@@ +109,5 @@
> +  PRUint8 *mRow;      // Holds one raw line of the image
> +  PRInt32 mCurLine;   // Line index of the image that's currently being decoded
> +  PRUint32 mRowBytes; // How many bytes of the row were already received
> +  PRInt32 mOldLine;   // Previous index of the line 
> +  nsAutoPtr<Decoder> mContainedImage; // Contains either a BMP or PNG resource

This should probably be called mContainedDecoder.

Comment 54

[Brian R. Bondy [:bbondy]]

> I don't really understand what you mean by "the original decoder." Can we fix that problem as a pre-patch rather than adding mFramePosted?

I just meant the code as is now released, before this patch.  I can try to fix without adding a variable.

Comment 55

[Brian R. Bondy [:bbondy]]

Attachment: Patch for supporting PNG ICO files, and BMP/ICO code (with review changes)

- Got rid of frame posted variable, the info could be obtained by simply using GetFrameCount()
- Added error checking in all cases for GetNumColors(), there was error checking at the start of the decode for this so it would never reach the later calls but it's better to always check for errors anyway.  By the way there is a carefully crafted reftest already for this in mozilla-central\modules\libpr0n\test\reftest\ico\ico-bmp-corrupted\invalid-bpp.ico
- Implemented other review comments


By the way, the original author of this file did tabspace == 4, if you want once this is r+ I can fix the spacing globally.

Comment 56

[Joe Drew (not getting mail)]

(In reply to comment #54)
> > I don't really understand what you mean by "the original decoder." Can we fix that problem as a pre-patch rather than adding mFramePosted?
> 
> I just meant the code as is now released, before this patch.  I can try to
> fix without adding a variable.

Actually the variable isn't what I objected to; the need for the logic at all is the problem I had with it.

Can you describe exactly what situation caused the need for the check? Maybe there's a better way around it.

Comment 57

[Brian R. Bondy [:bbondy]]

Yup I can describe why the check is needed:

As you know, `WriteInternal` can be called multiple times, specifying only part of the data each time.

With the BMP decoder that is live today in FF5, if you happen to call WriteInternal with exactly only the first WIN_HEADER_LENGTH header bytes, and then call WriteInternal with the rest of the data, then you will have a bug of 2 frames being posted. 

Why?
    
Because if you have exaclty WIN_HEADER_LENGTH bytes in aCount it will reach this condition: 
if (mPos == mLOH /*&& GetFrameCount() == 0*/) {
... add frame / ensure frame code ...
... does not change mPos...
}

When it reaches this condition aCount is == 0.

The first call to WriteInternal will then end inside the next condition since there is no data left.
if (mColors && (mPos >= mLOH && (mPos < (mLOH + mNumColors * bpc)))) {
  .. returns since no data left and does not change mPos...
}


The next time WriteInternal is called with more data it will again repost the frame because inside mPos is still the same value. (i.e. This still holds mPos == mLOH)

Instead of GetFrameCount() == 0 we could probably check aCount != 0 if you prefer.

Comment 58

[Joe Drew (not getting mail)]

OK, I think I can buy that testing for GetFrameCount() == 0 is OK given that situation. I think it would probably be useful to have the logic change fundamentally here, but it isn't necessary to do it in this bug.

Comment 59

[Joe Drew (not getting mail)]

Comment on attachment 549597 [details] [diff] [review]
Patch for supporting PNG ICO files, and BMP/ICO code (with review changes)

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

I think this is ready. I don't need to review it again.

::: modules/libpr0n/decoders/nsBMPDecoder.cpp
@@ +320,5 @@
> +        }
> +        if (mBIH.compression == BI_ALPHABITFIELDS && 
> +            mBIH.bpp != 16 && mBIH.bpp != 32) {
> +          PR_LOG(gBMPLog, PR_LOG_DEBUG, 
> +                 ("BMP ALPHABITFIELDS only supports 16 AND 32 bits per pixel\n"));

16 _OR_ 32 bits, right?

::: modules/libpr0n/decoders/nsBMPDecoder.h
@@ +60,5 @@
> +
> +// For our purposes, we include bihsize in the BFH, this makes the decoding
> +// easier since the BIH can have varying sizes and representations
> +#define BFH_LENGTH 14 
> +#define BFH_INTERNAL_LENGTH 18

Can you modify this comment to better diferentiate these two constants?

Comment 60

[Brian R. Bondy [:bbondy]]

Attachment: Completed patch for supporting PNG ICO files, and BMP/ICO code

Comment 61

[Brian R. Bondy [:bbondy]]

I'll wait until all of the related tasks are done and run everything through the try server before marking as checkin needed.

Comment 62

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/373bd41d9e28
http://hg.mozilla.org/mozilla-central/rev/5401cb3d7350
http://hg.mozilla.org/mozilla-central/rev/1f86f6af9434

Comment 63

[bomfog]

On a mozilla-inbound nightly, Vista OS,

http://hg.mozilla.org/integration/mozilla-inbound/rev/c70c539f2e93

(Very) dirty profile, Session Manager extension in use, opening a session of a half dozen or so windows with ~ 75 tabs total, I crash at

[@ mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) ]

https://crash-stats.mozilla.com/report/index/bp-9f797b78-e5df-4402-b70c-e08052110826
https://crash-stats.mozilla.com/report/index/bp-0d3d66a2-bf2e-4b80-8f45-108882110826

Session Manager UI got borked in the process, so haven't tracked down which page(s) cause the problem (yet).

Also will try to verify the offending changeset.

This bug?

Comment 64

[Brian R. Bondy [:bbondy]]

Sounds like this bug, did you want to have it backed out? There are a few others that were pushed at the same time which are dependent on this one too.

Comment 65

[Brian R. Bondy [:bbondy]]

Attachment: Possible fix for Comment 63

Comment 66

[Brian R. Bondy [:bbondy]]

bomfog: If you have an exact page that you can find that does this, that would be great as I'd be able to check if the patch I just made fixes it.

Comment 67

[bomfog]

(In reply to Brian R. Bondy [:bbondy] from comment #64)
> Sounds like this bug, did you want to have it backed out?

Thanks for asking, but I'm just a user :)

I'll try to narrow it down to an offending page.

Comment 68

[Brian R. Bondy [:bbondy]]

I know the patch (Attachment 550884 [details] [diff]), but I a page that has an icon that reproduces it.

Comment 69

[Brian R. Bondy [:bbondy]]

Sorry I meant to say:
...but I *need* a page...

Comment 70

[bomfog]

Sorry, I seem to have exhausted my competence without result.

The crash seems to depend on some combination of open tabs, Session Manager version, restored vs recreated-via-bookmarks session, maybe cache or cookies, and probably the phase of the moon, among other possibilities.

If I manage to narrow things down, I'll let you know, but I'm not hopeful.

Comment 71

[Brian R. Bondy [:bbondy]]

ok I'll post a new ticket and also try to cook up some reference ico file with a hex editor that can cause a crash at that location to verify the fix.

Comment 72

[Brian R. Bondy [:bbondy]]

I moved your report here bomfog: 
Bug 682568 - Investigate ICO crash and create reftest, reference image, and fix 

Thanks for the report.

Comment 73

[Brian R. Bondy [:bbondy]]

How in depth are we lookeing for dev-doc?  

Do we want to write our own detailed BMP and ICO spec?  
Or just add entries to here? https://wiki.mozilla.org/Mozilla2:ImageLib?
Or?

Comment 74

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

review with 549468

Comment 75

[Eric Shepherd [:sheppy]]

Is there more to this, from a developer doc standpoint, than just saying we now support Vista-style ICO files? Or does someone want to put together docs for that format? Is there a doc for the format out there we can link to?

Comment 76

[Brian R. Bondy [:bbondy]]

The ICO and BMP file formats on Wikipedia are very good and complete.

http://en.wikipedia.org/wiki/BMP_file_format
http://en.wikipedia.org/wiki/ICO_%28file_format%29

Comment 77

[Rimas Kudelis [use needinfo to ping]]

Brian, in Comment 25 you said you would add some info to my spec. Are you still going to do that? :)

Comment 78

[Brian R. Bondy [:bbondy]]

> Is there more to this, from a developer doc standpoint, 
> than just saying we now support Vista-style ICO files? 

No I don't think there is anything else.  There is encoder options (which can be accessed from canvas) for the BMP and ICO encoders, but those are not in this task they are in the follow up tasks.

>  in Comment 25 you said you would add some info to my spec

I don't think it would particularly useful, but that's why I asked this question specifically in Comment 73.  I haven't heard any feedback for this in about a month so I think we do not need it.  If anyone feels strongly that we should have a formal spec of our own please post a new task and I can take that task eventually.  

What we do have that I think is useful though is several hundred new reftests to ensure that we don't break anything.

> Regarding your spec

Regarding your spec though it was very useful to me while developing this to get up to speed. If you want to put up a wiki page for a getting started with implementing ICO files with the contents of that document for future imagelib developers, that would be great.

Comment 79

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/En/HTML/Element/Img#Supported_image_formats

And mentioned on Firefox 9 for developers.

Comment 80

[Brian R. Bondy [:bbondy]]

Thanks sheppy.

We always supported ICOs and BMP, so I took off the min Gecko 9.0 on those.
I split the ICO into two: BMP ICO and PNG ICO and put the Gecko 9.0 only on the PNG ICO.

Comment 81

[Christoph Diehl [:posidron]]

Plugin for ICO fuzzing has been updated. Will start a new fuzzing run most likely on Monday. If someone has interesting samples to share then please let me know.