108318 - Make ICO decoder non-incremental

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Hyatt]

This bug is tracking adjusting the ICO decoder to be non-incremental (since
being incremental on a 16x16 icon is rather pointless anyway) and is also
tracking ensuring that the alpha info is set first.

Comment 1

[David Hyatt]

Attachment: Make ICO decoder more robust.

Comment 2

[David Hyatt]

This patch stops using the bitmap info header width/height fields in favor of 
the icon dir entry width/height fields.  What I'm seeing with some favicons on 
the net is that the width/height fields are sometimes garbage in the bitmap 
info header.  It appears that they're relying on the decoder only using the 
icon dir entry width/height.  While I'm sure the ico files are technically 
malformed, this makes the ICO decoder more robust in that it can successfully 
view those files.

Comment 3

[David Hyatt]

Looking for r/sr on this patch.

Comment 4

[David Hyatt]

Attachment: Stop sending OnDataAvailable twice for a given row.

Comment 5

[David Hyatt]

Attachment: Avoid re-decoding the alpha mask.

Comment 6

[David Hyatt]

Attachment: Cleanup.

Comment 7

[Stuart Parmenter]

Comment on attachment 56454 [details] [diff] [review]
Cleanup.

r=pavlov

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 56454 [details] [diff] [review]
Cleanup.

sr=jst

Comment 9

[David Hyatt]

Attachment: Final patch to make decoder non-incremental and to set alpha data before setting image data.

Comment 10

[David Hyatt]

Attachment: Non-incremental decoder, sets alpha before image data.

Comment 11

[Stuart Parmenter]

Comment on attachment 56699 [details] [diff] [review]
Non-incremental decoder, sets alpha before image data.

r=pavlov

Comment 12

[Darin Fisher]

Comment on attachment 56699 [details] [diff] [review]
Non-incremental decoder, sets alpha before image data.

>Index: decoders/bmp/nsICODecoder.cpp

>+  mFrame->GetImageBytesPerRow(&bpr);
>+  for (PRUint32 i = 0; i < mDirEntry.mHeight; i++) {
>+    PRUint32 offset = bpr*i;
>+    mFrame->SetImageData(mDecodedBuffer+offset, bpr, offset);
>+  }

how about this instead:

for (PRUint32 offset = 0, i = 0; i < mDirEntry.mHeight; ++i, offset += bpr)
  mFrame->SetImageData(mDecodedBuffer + offset, bpr, offset);


>+  for (PRUint32 i = 0; i < mDirEntry.mHeight; i++) {
>+    PRUint32 offset = bpr*i;
>+    mFrame->SetAlphaData(mAlphaBuffer+offset, bpr, offset);
>+  }

likewise, here.

Comment 13

[Chris Waterson]

strength reduce as darin suggested, and sr=waterson.

Comment 14

[David Hyatt]

Fixed.

Comment 15

[Brendan Eich [:brendan]]

If reduction in strength is appropriate, why not also eliminate the induction
variable (i)?

for (PRUint32 offset = 0, limit = mDirEntry.mHeight * bpr;
     offset < limit;
     offset += bpr)
  mFrame->SetImageData(mDecodedBuffer + offset, bpr, offset);

/be