Bug 26460 – Cross-platform ICO decoder should return all icons in file as separate frames, sorted by decreasing quality

• Component: Images
• Status: RESOLVED FIXED

[Peter Kasting]

In order to unfork ImageSourceSkia, ICOImageDecoder.* needs to return all icons from a file instead of just one, and get rid of the "preferred size" concept (i.e. push this decision to the caller).  I think this will also require minor changes to RGBA32Buffer since the subsequent frames will differ in size from the original.

[Peter Kasting]

Doing this is going to include changing BMPImageReader to be included in *ImageDecoder via composition rather than inheritance, and fixing it to lazily decode instead of decoding at setData() (which was never the right behavior).

[Peter Kasting]

Created attachment 31509 [details]
part one (Landed r44825)

This is just some miscellaneous cleanup I did while writing the main behavioral changes here.

[Eric Seidel (no email)]

Comment on attachment 31509 [details]
part one (Landed r44825)

Will your change to:
 116 bool GIFImageDecoder::isSizeAvailable()

ever cause it to return true when m_failed?

Otherwise LGTM.

[Peter Kasting]

(In reply to comment #3)
> (From update of attachment 31509 [details] [review])
> Will your change to:
>  116 bool GIFImageDecoder::isSizeAvailable()
> 
> ever cause it to return true when m_failed?

No, because ImageDecoder::isSizeAvailable() checks this.

[Peter Kasting]

Created attachment 31519 [details]
part two

The next patch is going to change BMPImageReader to not inherit from ImageDecoder, and instead change *ImageDecoder to inherit directly, and include a BMPImageReader as a member.

Since that's a large, hard-to-review change, this patch attempts to make it more readable by first landing as much non-functional stuff as possible to minimize the diff.  Specific items are mentioned in the ChangeLog entry, but none should have any functional effect.

[Eric Seidel (no email)]

Comment on attachment 31519 [details]
part two

I don't think the line wraping is really worth it.  But OK.  

You were going to clean up the:
 299         m_infoHeader.biClrUsed = readUint32(data, m_decodedOffset + 32);

stuff, and I'll look in more detail in a second pass. This is a big big to page into my brain all at once. :)

[Peter Kasting]

Created attachment 31523 [details]
part two, v2 (Modified and landed r44833)

Updated as promised.

[Eric Seidel (no email)]

Comment on attachment 31523 [details]
part two, v2 (Modified and landed r44833)

Style:
         inline uint32_t readUint32(SharedBuffer* data, int offset) const {
 50             return readUint32Helper(data, m_decodedOffset + offset);
 51         }
Fight your google instincts!  fight, fight fight! ;)

Why this change?
 115     RGBA32Buffer* buffer = &m_frameBufferCache.first();

I don't think the wrapping to 80c is really helpful in cases liek this:

8         if (((m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS) <
 479                 (m_headerOffset + m_infoHeader.biSize))
 480             || (m_imgDataOffset && (m_imgDataOffset <
 481                 (m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS)))) {
 482             setFailed();

or this:
             m_bitMasks[i] &=
 511                 ((static_cast<uint32_t>(1) << m_infoHeader.biBitCount) - 1);

are we trying to merge these with something?  If not, then there is no need to try and keep 80c wrapping.

Style:
137         inline uint16_t readUint16(SharedBuffer* data, int offset) const {
 138             return readUint16Helper(data, m_decodedOffset + offset);
 139         }
 140 
 141         inline uint32_t readUint32(SharedBuffer* data, int offset) const {
 142             return readUint32Helper(data, m_decodedOffset + offset);
 143         }

In general this looks fine.  Please address the above issues when landing.

[Peter Kasting]

(In reply to comment #8)
> (From update of attachment 31523 [details] [review])
> Style:
>          inline uint32_t readUint32(SharedBuffer* data, int offset) const {
>  50             return readUint32Helper(data, m_decodedOffset + offset);
>  51         }
> Fight your google instincts!  fight, fight fight! ;)

I always have to look at these for several minutes to realize what style rule I'm breaking!  Fixed.

> Why this change?
>  115     RGBA32Buffer* buffer = &m_frameBufferCache.first();

Because in the next patch |buffer| is going to become a pointer member that gets set by the owner classes.  This goes halfway by making it a pointer now (but not member).

> I don't think the wrapping to 80c is really helpful in cases liek this:
> 
> 8         if (((m_headerOffset + m_infoHeader.biSize + SIZEOF_BITMASKS) <
>  479                 (m_headerOffset + m_infoHeader.biSize))
>  480             || (m_imgDataOffset && (m_imgDataOffset <
>  481                 (m_headerOffset + m_infoHeader.biSize +
> SIZEOF_BITMASKS)))) {
>  482             setFailed();
> 
> or this:
>              m_bitMasks[i] &=
>  511                 ((static_cast<uint32_t>(1) << m_infoHeader.biBitCount) -
> 1);
> 
> are we trying to merge these with something?  If not, then there is no need to
> try and keep 80c wrapping.

I know WK doesn't have an 80c rule, but it doesn't really have a prohibition against it.  I do think they're more readable when I don't have to scroll my screen around :).

> Style:
> 137         inline uint16_t readUint16(SharedBuffer* data, int offset) const {
>  138             return readUint16Helper(data, m_decodedOffset + offset);
>  139         }
>  140 
>  141         inline uint32_t readUint32(SharedBuffer* data, int offset) const {
>  142             return readUint32Helper(data, m_decodedOffset + offset);
>  143         }

Yep, fixed.

[Peter Kasting]

Created attachment 31526 [details]
part three (Landed r44874)

This is the bulk of the work -- switch from inheritance to composition (so that in the future ICOImageDecoder can decode several different BMPs within one file) and move to lazy decoding like the decoders are supposed to do.

Even though the nitpicky details about how to decode (e.g. structs to look through, offsets within them, how to decode individual pixels, etc.) have not changed, this patch is still pretty hard to review.  Sorry, I don't know how to make it simpler easily :/.  It _might_ be possible to just do composition first and then lazy decoding as a second pass, but they're so intertwined that it'd take a lot of work, I'd immediately throw all that work away, and I'm not sure the separated patches would be more comprehensible.

Note that a lot of the diff here is still "mechanical" despite the last patch, like switching to the new, simpler-to-use form of the readUintX() wrappers.

[Eric Seidel (no email)]

Comment on attachment 31526 [details]
part three (Landed r44874)

I assume BMPs are always single-frame?  We should have a comment to that effect here:
 RGBA32Buffer* BMPImageDecoder::frameBufferAtIndex(size_t index)
 70 {
 71     if (index)
 72         return 0;
 73 

Why do we have to pre-allocate the space:
74     if (m_frameBufferCache.isEmpty())
 75         m_frameBufferCache.resize(1);

Oh, I guess because we're grabbing it on the next line:
RGBA32Buffer* buffer = &m_frameBufferCache.first();

Still seems awkward somehow.

We generally try to think of more descriptive names than "Helper".

It seems strange that we have to do all these manual calls to set up the BMPImageReader:
1     if (!m_reader) {
 102         m_reader.set(new BMPImageReader(this, m_decodedOffset, imgDataOffset,
 103                                         false));
 104         m_reader->setData(m_data.get());
 105     }
 106 
 107     if (!m_frameBufferCache.isEmpty())
 108         m_reader->setBuffer(&m_frameBufferCache.first());
 109 
 110     return m_reader->decodeBMP(onlySize);

I would think we could implement a virtual interface and pass "this" to the BMPReader and it could grab all this appropriate data itself?

processFileHeader should take a reference instead of a pointer.  There seems to be little value in using a pointer, especially since at the callsite you do:
&& !processFileHeader(&imgDataOffset))

The arg name "data" doesn't really add much here:
 46         virtual void setData(SharedBuffer* data, bool allDataReceived);

getInfoHeaderSize is poorly named.  It does not actually return the size.  It should be something like updateInfoHeaderSize or calculateInfoHeaderSize or cacheInfoHeaderSize (You don't have to change it here, but next time you're cleaning up this code...)

m_parent seems like kinda a funny name for the owning decoder.  I guess I would have called it m_imageDecoder or m_owningDecoder maybe?  m_parent is fine... just initially confusing.

When you ask m_parent->size() aren't you getting the size of the first frame?  I guess all frames are the same size?  Is this true of ICO files too?

I think it's neat that setFailed() returns false, but it is a litlte confusing that in other places of the code we don't use:
return setFailed();

Is it possible/expected that the malloc of any of these framebuffers would/should fail?  Are there really large BMPs out there we need to be aware of?  I believe fastMalloc will just abort() if it fails, which is fine.

confused why your'e not setting m_headerOffset anymore?
      m_decodedOffset = m_headerOffset = m_dirEntry.dwImageOffset;
 214     m_decodedOffset = m_dirEntry.dwImageOffset;

It seems you should either remove or rename BMPImageReader::size() to avoid confusion with m_parent->size()?

The 80c thing still seems silly to me.  Since we don't wrap to 80c in other parts of WebCore.


This change looks great!  I think that the management of buffers between ICODecoder and BMPImageReader is a little too manual/error-prone.  And the "Helper" name is silly, but I can't seem to come up with a better one.  I like that you split out the "Helper" code into another function, although you might not have needed to if the buffer management was simpler.

I don't really need to see this another time.  Please read through and deal with the nits above, but you're good to go.

I look forward to seeing these classes use things like IntPoint and IntSize :)

[Peter Kasting]

(In reply to comment #11)
> (From update of attachment 31526 [details] [review])
> I assume BMPs are always single-frame?  We should have a comment to that effect
> here:
>  RGBA32Buffer* BMPImageDecoder::frameBufferAtIndex(size_t index)
>  70 {
>  71     if (index)
>  72         return 0;

I think this is too obvious to comment.  The identical code appears in the JPEG, PNG, and XBM decoders (and currently in the ICO decoder but that will eventually change).

> Why do we have to pre-allocate the space:
> 74     if (m_frameBufferCache.isEmpty())
>  75         m_frameBufferCache.resize(1);
> 
> Oh, I guess because we're grabbing it on the next line:
> RGBA32Buffer* buffer = &m_frameBufferCache.first();
> 
> Still seems awkward somehow.

I don't understand how this is awkward.  For one, it duplicates all the other decoders, but for another, you have to return a non-NULL frame pointer for a valid frame index request, and the actual decoding core has to have somewhere to write to.  These constraints make sense, too, since there's such a thing as a frame with status "empty" so this doesn't actually allocate a bunch of memory for pixel data but just says "ok, here's the struct that says what our results are if any".

> It seems strange that we have to do all these manual calls to set up the
> BMPImageReader:
> 1     if (!m_reader) {
>  102         m_reader.set(new BMPImageReader(this, m_decodedOffset,
> imgDataOffset,
>  103                                         false));
>  104         m_reader->setData(m_data.get());
>  105     }
>  106 
>  107     if (!m_frameBufferCache.isEmpty())
>  108         m_reader->setBuffer(&m_frameBufferCache.first());
>  109 
>  110     return m_reader->decodeBMP(onlySize);
> 
> I would think we could implement a virtual interface and pass "this" to the
> BMPReader and it could grab all this appropriate data itself?

I am unwilling to touch this until after I have implemented multi-image ICO decoding; at that point I'll know better what the true interface constraints are.

At that point something like what you suggest could certainly be possible, though I don't think a separate virtual interface to expose all this, plus all the manual calls (on the callee side) to get the data, is actually simpler; it's just as complex as the above plus an extra interface.  So I'm kind of skeptical.

> processFileHeader should take a reference instead of a pointer.  There seems to
> be little value in using a pointer, especially since at the callsite you do:
> && !processFileHeader(&imgDataOffset))

I don't understand.  processFileHeader() is _setting_ imgDataOffset, so it's a pure outparam.  Are you saying that non-const refs are _preferable_ to pointers for pure outparams?  This would certainly be illegal Google style and it raises my personal hackles too.

> The arg name "data" doesn't really add much here:
>  46         virtual void setData(SharedBuffer* data, bool allDataReceived);

Will remove.  (I hate this style rule but I don't make the rules.)

> getInfoHeaderSize is poorly named.  It does not actually return the size.  It
> should be something like updateInfoHeaderSize or calculateInfoHeaderSize or
> cacheInfoHeaderSize (You don't have to change it here, but next time you're
> cleaning up this code...)

Will change to readInfoHeaderSize().

> m_parent seems like kinda a funny name for the owning decoder.  I guess I would
> have called it m_imageDecoder or m_owningDecoder maybe?  m_parent is fine...
> just initially confusing.

|m_owner| would be fine with me if you think that would be clearer.  (Putting "decoder" in the name seems redundant with the type.)

> When you ask m_parent->size() aren't you getting the size of the first frame? 
> I guess all frames are the same size?  Is this true of ICO files too?

There aren't multiple frames.  Just one.  (I will see whether once I start decoding ICOs as multi-frame images this needs to change but for now it is accurate.)

> I think it's neat that setFailed() returns false, but it is a litlte confusing
> that in other places of the code we don't use:
> return setFailed();

I assume you refer to the subclasses of ImageDecoder.  I think it would make sense to change ImageDecoder::setFailed() from void to bool for this same purpose.  The change seemed extraneous to this one.  I'll do it as a followup that will also peer through other decoders' uses of m_failed.

> Is it possible/expected that the malloc of any of these framebuffers
> would/should fail?  Are there really large BMPs out there we need to be aware
> of?  I believe fastMalloc will just abort() if it fails, which is fine.

The code explicitly handles this.  RGBA32Buffer::setSize() is the call that allocates, and callers check its return code and fail the decode if the allocation didn't succeed.

> confused why your'e not setting m_headerOffset anymore?
>       m_decodedOffset = m_headerOffset = m_dirEntry.dwImageOffset;
>  214     m_decodedOffset = m_dirEntry.dwImageOffset;

Because it doesn't exist (in the class where this code is).  An artifact of constructing the BMPImageReader where I do is that m_headerOffset is guaranteed to equal m_decodedOffset at that moment (if we've succeeded in decoding so far) and that is what the BMPImageReader constructor sets up.

> It seems you should either remove or rename BMPImageReader::size() to avoid
> confusion with m_parent->size()?

I don't understand.  There is no BMPImageReader::size().

> This change looks great!  I think that the management of buffers between
> ICODecoder and BMPImageReader is a little too manual/error-prone.

This was designed with an eye towards what I'll need when I do multi-frame decoding in ICOs.  It doesn't seem too bad right now, honestly; there's only the image data and the framebuffer, right?  One change I could do to make things easier would be to make BMPImageReader a friend and let it slurp its owner's data directly instead of doing an extra setData() call on the owner side, but I don't know how much benefit that really counts as...

> And the
> "Helper" name is silly, but I can't seem to come up with a better one.

I could use decodeWrapper() and decode(), or decodeWithCheckForDataEnded() and decode(), or something... I dunno

> I like
> that you split out the "Helper" code into another function, although you might
> not have needed to if the buffer management was simpler.

I actually could do it all inline, but I'd have to use about twice as many conditionals.  I don't think buffer management complexity has anything to do with this needing two pieces... I just needed a convenient place to "catch" all the decode failures a la an exception handler.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> > processFileHeader should take a reference instead of a pointer.  There seems to
> > be little value in using a pointer, especially since at the callsite you do:
> > && !processFileHeader(&imgDataOffset))
> 
> I don't understand.  processFileHeader() is _setting_ imgDataOffset, so it's a
> pure outparam.  Are you saying that non-const refs are _preferable_ to pointers
> for pure outparams?  This would certainly be illegal Google style and it raises
> my personal hackles too.

I use non-const references... but I've never seen them as dangerous or confusing. :)

The WebKit Style guide seems mute on the issue.

> > m_parent seems like kinda a funny name for the owning decoder.  I guess I would
> > have called it m_imageDecoder or m_owningDecoder maybe?  m_parent is fine...
> > just initially confusing.
> 
> |m_owner| would be fine with me if you think that would be clearer.  (Putting
> "decoder" in the name seems redundant with the type.)

Interesting.  I intentionally name my variables to make their type clear at all callsites.  I don't see it as redundant at all. :)

m_parent is also fine.

> > I think it's neat that setFailed() returns false, but it is a litlte confusing
> > that in other places of the code we don't use:
> > return setFailed();
> 
> I assume you refer to the subclasses of ImageDecoder.  I think it would make
> sense to change ImageDecoder::setFailed() from void to bool for this same
> purpose.  The change seemed extraneous to this one.  I'll do it as a followup
> that will also peer through other decoders' uses of m_failed.

Sounds fine.

> > It seems you should either remove or rename BMPImageReader::size() to avoid
> > confusion with m_parent->size()?
> 
> I don't understand.  There is no BMPImageReader::size().

You were changing callsites from "size()" to "m_parent->size()" I wanted to make sure that size() was gone.

> > This change looks great!  I think that the management of buffers between
> > ICODecoder and BMPImageReader is a little too manual/error-prone.
> 
> This was designed with an eye towards what I'll need when I do multi-frame
> decoding in ICOs.  It doesn't seem too bad right now, honestly; there's only
> the image data and the framebuffer, right?  One change I could do to make
> things easier would be to make BMPImageReader a friend and let it slurp its
> owner's data directly instead of doing an extra setData() call on the owner
> side, but I don't know how much benefit that really counts as...
> 
> > And the
> > "Helper" name is silly, but I can't seem to come up with a better one.
> 
> I could use decodeWrapper() and decode(), or decodeWithCheckForDataEnded() and
> decode(), or something... I dunno

decodeWithCheckForDataEnded() sounds more desciptive than "helper" :)  But I haven't looked at the source to verify if it's an accurate name. :)

> > I like
> > that you split out the "Helper" code into another function, although you might
> > not have needed to if the buffer management was simpler.
> 
> I actually could do it all inline, but I'd have to use about twice as many
> conditionals.  I don't think buffer management complexity has anything to do
> with this needing two pieces... I just needed a convenient place to "catch" all
> the decode failures a la an exception handler.

OK.

Thanks for the replies.

[Peter Kasting]

(In reply to comment #13)
> > > It seems you should either remove or rename BMPImageReader::size() to avoid
> > > confusion with m_parent->size()?
> > 
> > I don't understand.  There is no BMPImageReader::size().
> 
> You were changing callsites from "size()" to "m_parent->size()" I wanted to
> make sure that size() was gone.

Yes, size() was ImageDecoder::size() since BMPImageReader used to inherit from it.  Now it doesn't, so there is no size().

> decodeWithCheckForDataEnded() sounds more desciptive than "helper" :)  But I
> haven't looked at the source to verify if it's an accurate name. :)

Will change, and add comment to decode() saying not to call it directly.

[Peter Kasting]

Created attachment 33468 [details]
part four

Some cleanup and plumbing I split out of the next patch (which, finally, implements the desired behavior), in order to make it more readable.

Mainly, this adds a new function ImageDecoder::frameSizeAtIndex(), and hooks the ImageSource::frameSizeAtIndex() implementations to it; no decoder actually implements it yet (the ICO decoder will after the next patch).

[Eric Seidel (no email)]

Comment on attachment 33468 [details]
part four

Can we test this using pixel tests?
Can we share:
+IntSize ImageSource::frameSizeAtIndex(size_t index) const
via some baseclass?

[Peter Kasting]

(In reply to comment #16)
> (From update of attachment 33468 [details])
> Can we test this using pixel tests?

I know Chromium has some BMP and ICO unittests (although they do a lot more than just load the images and display them, they hook directly to the decoders so they can test things like breaking the input data into random chunks), it would probably be possible to create a few tests that are just "load this BMP/ICO and ensure it displays".

One difficulty is that these open-source BMP and ICO decoders support more variants of the formats than the CG decoders do.  I'd probably need to make some tests only run for non-CG ports...

The main difficulty here is that I haven't written a pixel test.  It's probably time to rectify that failure :)

> Can we share:
> +IntSize ImageSource::frameSizeAtIndex(size_t index) const
> via some baseclass?

Yes, the whole point of this bug is to enable me to remove some hacks from the Skia ImageSource so we can then collapse most of the ImageSource implementations together.  So, I wouldn't worry about this right this second; it is planned :)

[Peter Kasting]

Fixed in r46423.

[Peter Kasting]

Oops, I commented on the wrong bug.  Ignore that...

[Eric Seidel (no email)]

Comment on attachment 33468 [details]
part four

Sorry, forgot about this one.  Looks good!

[Peter Kasting]

(In reply to comment #20)
> (From update of attachment 33468 [details])
> Sorry, forgot about this one.  Looks good!

I've created an ICO decoder pixel test, but unfortunately the run-webkit-tests script doesn't seem to like my non-Cygwin-SVN setup, and I'm not sure how to fix :(

Maybe I'll work with you to get it landed.

[Peter Kasting]

Comment on attachment 33468 [details]
part four

Landed in r46738, clearing flags.

[Peter Kasting]

Created attachment 34031 [details]
Return multiple frames from ICO decoder

This is the heart of the change -- return multiple frames from the ICO decoder and eliminate the ImageSourceSkia hack.

[Eric Seidel (no email)]

Comment on attachment 34031 [details]
Return multiple frames from ICO decoder

When is "*i" going to be NULL?
     for (BMPReaders::iterator i(m_BMPReaders.begin());
 69          i != m_BMPReaders.end(); ++i) {
 70         if (*i)
 71             (*i)->setData(data);
7172     }

Seems rather strange to have readers in your list which will be null.

I believe the style is m_bmpReaders instead of m_BMPReaders.  and m_pngDecoders instead of m_PNGDecoders.

Seems this shoudl use early-return to avoid the long indented block:
 132     if (m_PNGDecoders[index]) {

 133         // The size calculated inside the PNGImageDecoder had better match the
 134         // one in the icon directory.

probably should be:
// Fail if the size the PNGImageDecoder calculated does not match the size in the directory

It seems IconDirectoryEntry should have a size() accessor which returns a IntSize() given that you have to convert it to an IntSize more than once.  Or at least that:
138             if ((pngSize.width() != dirEntry.bWidth)
 139                 || (pngSize.height() != dirEntry.bHeight)) {
would be simpler if it had such a function.

Seems another early return is in order:
 169     if (m_PNGDecoders[index]) {
 170         // Copy out PNG data to a separate vector and send to the PNG decoder.

We don't have any column wrapping rule:
199     return (m_decodedOffset >=
 200             (sizeOfDirectory + (m_dirEntries.size() * sizeOfDirEntry)))
 201         || processDirectoryEntries();

Seems this block could instead be a helper function "determineImageType":
208     if (!m_BMPReaders[index] && !m_PNGDecoders[index]) {
 209         // Image type unknown.
maybe a separate function is not clearer, not sure.

Doesn't Vector have a VectorTraits or some such to specify a default value?  Or isnt' there a .fill()?   I'm surprised you have to use this VectorInitializer stuff, but maybe that's the right way...

No wrapping needed:
3     for (IconDirectoryEntries::iterator i(m_dirEntries.begin());
 284          i != m_dirEntries.end(); ++i)


It looks like the PNGDecoders type is never used.

Seems odd to use an iterator for one here and not the other:
68     for (BMPReaders::iterator i(m_BMPReaders.begin());
 69          i != m_BMPReaders.end(); ++i) {
 70         if (*i)
 71             (*i)->setData(data);
7172     }
 73     for (size_t i = 0; i < m_PNGDecoders.size(); ++i)
 74         setDataForPNGDecoderAtIndex(i);

if the iterator is not used, then the BMPReaders typedef is never needed.

r- for the nits above.

Also, I look at the review queue every day, but I only click on bugs which Safari thinks are un-visited.  Occasionally I look through the whole queue (like I am doing tonight).  So I won't notice your patches if you keep attaching them to old bugs. :)  WebKit seems to be of many minds about whether to re-use bugs or not. :)  (I go either way depending on the patch.)

[Peter Kasting]

Some comments before I update the patch tomorrow.

(In reply to comment #24)
> (From update of attachment 34031 [details])
> When is "*i" going to be NULL?
>      for (BMPReaders::iterator i(m_BMPReaders.begin());
>  69          i != m_BMPReaders.end(); ++i) {
>  70         if (*i)
>  71             (*i)->setData(data);
> 7172     }
> 
> Seems rather strange to have readers in your list which will be null.

*i will almost always be NULL.  There is potentially one BMP reader or PNG decoder per directory entry, but they are only instantiated when a caller actually requests the frame data for a particular entry.

You cannot do this nicely by having just one BMP reader and one PNG decoder as the old code did, because if you get asked for a different frame you have to discard all the probably-still-valuable decoding you did already and reset everything, redecoding the new frame from scratch.

If you try having arrays of objects, or guaranteed-not-null pointers, instead of this method, then you burn a lot of memory, especially on setData() calls (which create new data blobs for each PNG decoder), for pretty much no benefit.

I suppose rather than an array of mostly-NULL pointers, I could have used a linked list and kept a frame number alongside each decoder in the list.  But since BMPImageReader and PNGImageDecoder do not share a base class, I'd need two lists; and since icons generally have very few entries (typically one, sometimes up to five or so), the code would be a bit more complex for no real gain.

> I believe the style is m_bmpReaders instead of m_BMPReaders.  and m_pngDecoders
> instead of m_PNGDecoders.

OK, will change.

> Seems this shoudl use early-return to avoid the long indented block:
>  132     if (m_PNGDecoders[index]) {

It's actually not "early return" either way, since you either do BMP as the indented block or PNG (there's no chance for "if !PNG return;" here).  BMP is the shorter code block, so I can swap them.  In my head it made a little more sense to me to have the BMP path be the "main path" through the function since that's the common case, but that's a pretty weak argument.

>  133         // The size calculated inside the PNGImageDecoder had better match
> the
>  134         // one in the icon directory.
> 
> probably should be:
> // Fail if the size the PNGImageDecoder calculated does not match the size in
> the directory

Sure, OK

> It seems IconDirectoryEntry should have a size() accessor which returns a
> IntSize() given that you have to convert it to an IntSize more than once.  Or
> at least that:
> 138             if ((pngSize.width() != dirEntry.bWidth)
>  139                 || (pngSize.height() != dirEntry.bHeight)) {
> would be simpler if it had such a function.

Good idea.  I was trying to just write a simple dumb container of a struct that matched the Windows ICONDIRENTRY, but adding a simple accessor (one line of code) won't hurt it, and will indeed make a few spots less verbose.

> Seems another early return is in order:
>  169     if (m_PNGDecoders[index]) {
>  170         // Copy out PNG data to a separate vector and send to the PNG
> decoder.

Yes, this one is a legitimate early return candidate.

> We don't have any column wrapping rule:
> 199     return (m_decodedOffset >=
>  200             (sizeOfDirectory + (m_dirEntries.size() * sizeOfDirEntry)))
>  201         || processDirectoryEntries();

That's right, and I'm claiming that lack of rule as a justification for wrapping at 80 like the rest of this file (and most of the rest of the image decoder code), because code that goes for 200 characters is completely unreadable to me.

> Seems this block could instead be a helper function "determineImageType":
> 208     if (!m_BMPReaders[index] && !m_PNGDecoders[index]) {
>  209         // Image type unknown.
> maybe a separate function is not clearer, not sure.

I do not think a function here buys you anything.

> Doesn't Vector have a VectorTraits or some such to specify a default value?  Or
> isnt' there a .fill()?   I'm surprised you have to use this VectorInitializer
> stuff, but maybe that's the right way...

I don't know if there is a better way of doing this.  I am open to such, as the current code is ugly.

> No wrapping needed:
> 3     for (IconDirectoryEntries::iterator i(m_dirEntries.begin());
>  284          i != m_dirEntries.end(); ++i)

I'm not changing this either :)

> It looks like the PNGDecoders type is never used.

It's only used at the declaration of the object, but I'm OK with that; any future additions of modifications that declare an iterator or a temp will have a handy type to use.  Using a typedef for things like this feels like a good habit to me.

> Seems odd to use an iterator for one here and not the other:
> 68     for (BMPReaders::iterator i(m_BMPReaders.begin());
>  69          i != m_BMPReaders.end(); ++i) {
>  70         if (*i)
>  71             (*i)->setData(data);
> 7172     }
>  73     for (size_t i = 0; i < m_PNGDecoders.size(); ++i)
>  74         setDataForPNGDecoderAtIndex(i);

I'd like to use an iterator for both, but sadly in the case of setting data for the PNG decoder I also need the index to look up the right directory entry.

Otherwise, I normally consider iterators preferable to operator[]().

> I won't notice your patches if you keep
> attaching them to old bugs. :)  WebKit seems to be of many minds about whether
> to re-use bugs or not.

All the patches on here hang together as a logical unit and aren't really compelling in isolation.  It seems better to use one bug for them.  This is the last patch on this bug anyway.

[Peter Kasting]

(In reply to comment #25)
> > Seems this shoudl use early-return to avoid the long indented block:
> >  132     if (m_PNGDecoders[index]) {
> 
> It's actually not "early return" either way,

Oops, I misread my own diff.  Yes it can be early return.  Sorry :)

[Peter Kasting]

Created attachment 34155 [details]
Return multiple frames from ICO decoder, v2

Updated patch.  I changed IconDirectoryEntry to WebKit style and replaced the width and height members with an IntSize, which simplified a few things.  Also replaced my cumbersome VectorInitializer calls with calls to fill().  Other fixes as noted in my previous comments.  If you demand I modify my line wrapping I will :)

[Eric Seidel (no email)]

Comment on attachment 34155 [details]
Return multiple frames from ICO decoder, v2

Wrong patch.

[Peter Kasting]

Created attachment 34158 [details]
Return multiple frames from ICO decoder, v2 (for real)

Oops

[Eric Seidel (no email)]

Comment on attachment 34158 [details]
Return multiple frames from ICO decoder, v2 (for real)

I doubt the explicit namespace is needed here:
     WTF::deleteAllValues(m_bmpReaders);
 57     WTF::deleteAllValues(m_pngDecoders);

0 in C++
2     m_bmpReaders.fill(NULL, idCount);
 263     m_pngDecoders.fill(NULL, idCount);

Seems odd the sides of the cast are different:
 302     int width = static_cast<uint8_t>(m_data->data()[m_decodedOffset]);

virtual, no?
 44         ~ICOImageDecoder();

Looks sane enough.  Please note the comments above.

[Peter Kasting]

Fixed in r46807.