Bug 53455 – Update ImageDecoder as suggested on webkit-dev

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Update ImageDecoder as suggested on webkit-dev

[Adam Barth]

Created attachment 80685 [details]
Patch

[Adam Barth]

Created attachment 80686 [details]
Patch

[Peter Kasting]

Comment on attachment 80686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80686&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:56
> +inline bool matchesGIFSignature(char* contents)

I'd remove "inline" on all these as it's not important whether they're inlined.

I also suggest an anonymous namespace around all of them.  (Or "static" on each, but that's deprecated in C++.)

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:72
> +inline bool matchesWebPSignature(char* contents const SharedBuffer& data)

Missing comma, won't compile.

Instead of passing extra args, I'd just change the caller and simplify all this, see below.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:-59
> -    // with.

Might be nice to instead note that this value needs to be as large as the largest sequence above.

In that sense, I think we should change this to 8 and thus avoid all the additional copying stuff for WebP, unless it's possible to have a valid image of one of the other types that's less than 8 bytes total (I don't think it is).

[Adam Barth]

Created attachment 80691 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 80691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:76
> +    return !memcmp(contents, "RIFF", 4) && !memcmp(header + 8, "WEBPVP", 6);

“header” doesn’t appear to be defined here.

[Peter Kasting]

Comment on attachment 80691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

LGTM, whatever that's worth :).  Thanks for taking the initiative on this one.

P.S. You can remove the "Zero the image" comment too if you want, that's another one in this file that's pretty worthless.

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:76
>> +    return !memcmp(contents, "RIFF", 4) && !memcmp(header + 8, "WEBPVP", 6);
> 
> “header” doesn’t appear to be defined here.

Yeah... I think it should be |contents|.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:95
> +}

I think this is supposed to get a "// namespace" comment?

[WebKit Review Bot]

Attachment 80691 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7681586

[Peter Kasting]

Would be nice to update the header to clean up some of its comments/function names too.  Maciej and I may not agree on everything but there's certainly some stuff in there that we both concur is valueless.

I'll try to take a quick pass at that tomorrow.

[WebKit Review Bot]

Attachment 80691 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7685429

[David Levin]

Comment on attachment 80691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

Compile error (where header needs to be contents) plus a few other things to consider.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:43
> +unsigned copyFromSharedBuffer(char* buffer, unsigned bufferLength, const SharedBuffer& sharedBuffer, unsigned offset)

Why did this go from being static to being in an anonymous namespace? (I've seen this a lot in Chromium code but WebKit seems to go with static functions, so I was curious what caused you to choose one over the other.)

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:60
> +    return !strncmp(contents, "GIF8", 4);

How odd that some places use memcmp and others strncmp but you are simply translating what was there already.

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:95
>> +}
> 
> I think this is supposed to get a "// namespace" comment?

fwiw, that isn't a WebKit style (and several folks don't want it).

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:97
>  ImageDecoder* ImageDecoder::create(const SharedBuffer& data, ImageSource::AlphaOption alphaOption, ImageSource::GammaAndColorProfileOption gammaAndColorProfileOption)

Additional future clean up would make this a PassOwnPtr.

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:99
> +    static const unsigned lengthOfLongestSignature = 14; // To wit: "RIFF????WEBPVP"

This is different from before in that it potentially allowed 4 byte items to go thru previously but perhaps no image can be encoded in less than 14 bytes?

What about doing something like
  memset(contents, sizeof(contents) - length, 0xee);
instead of returning?

[Adam Barth]

Comment on attachment 80691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:43
>> -static unsigned copyFromSharedBuffer(char* buffer, unsigned bufferLength, const SharedBuffer& sharedBuffer, unsigned offset)
>> +namespace {
>> +
>> +unsigned copyFromSharedBuffer(char* buffer, unsigned bufferLength, const SharedBuffer& sharedBuffer, unsigned offset)
> 
> Why did this go from being static to being in an anonymous namespace? (I've seen this a lot in Chromium code but WebKit seems to go with static functions, so I was curious what caused you to choose one over the other.)

I can never remember which one I'm supposed to use.  Whichever one I pick, someone always tells me to use the other one.  I'm happy to do whichever.

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:60
>> +bool matchesGIFSignature(char* contents)
>> +{
>> +    return !strncmp(contents, "GIF8", 4);
> 
> How odd that some places use memcmp and others strncmp but you are simply translating what was there already.

Yes.  The strncmp are incorrect according to http://tools.ietf.org/html/draft-ietf-websec-mime-sniff

I'll fix them in a follow-up patch to keep the functional changes separate.

>>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:76
>>> +    return !memcmp(contents, "RIFF", 4) && !memcmp(header + 8, "WEBPVP", 6);
>> 
>> “header” doesn’t appear to be defined here.
> 
> Yeah... I think it should be |contents|.

Yeah.  Turns out its hard to write code without a compiler.  :)

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:97
>>  ImageDecoder* ImageDecoder::create(const SharedBuffer& data, ImageSource::AlphaOption alphaOption, ImageSource::GammaAndColorProfileOption gammaAndColorProfileOption)
> 
> Additional future clean up would make this a PassOwnPtr.

Good idea.  :)

>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:99
>> +    static const unsigned lengthOfLongestSignature = 14; // To wit: "RIFF????WEBPVP"
> 
> This is different from before in that it potentially allowed 4 byte items to go thru previously but perhaps no image can be encoded in less than 14 bytes?
> 
> What about doing something like
>   memset(contents, sizeof(contents) - length, 0xee);
> instead of returning?

That seems kind of hacky.  14 bytes is really small for an image.  As an example, a 1x1 transparent GIF is 43 bytes.

[Peter Kasting]

(In reply to comment #11)
> > Why did this go from being static to being in an anonymous namespace? (I've seen this a lot in Chromium code but WebKit seems to go with static functions, so I was curious what caused you to choose one over the other.)
> 
> I can never remember which one I'm supposed to use.  Whichever one I pick, someone always tells me to use the other one.  I'm happy to do whichever.

There are two reasons to use an anonymous namespace here:

(1) Since we're now going to have a whole bunch of helper functions, it's easier (for me at least) to see they're all a block of helpers when wrapped in a namespace than when each is individually marked static.
(2) "static" for file-scope linkage is deprecated in C++ (see Stroustrup 3rd ed., p. 200, 818).

> Yes.  The strncmp are incorrect according to http://tools.ietf.org/html/draft-ietf-websec-mime-sniff
> 
> I'll fix them in a follow-up patch to keep the functional changes separate.

I'd be OK with fixing them here if you want, it's pretty clear what the changes mean.

> > This is different from before in that it potentially allowed 4 byte items to go thru previously but perhaps no image can be encoded in less than 14 bytes?
> > 
> > What about doing something like
> >   memset(contents, sizeof(contents) - length, 0xee);
> > instead of returning?
> 
> That seems kind of hacky.  14 bytes is really small for an image.  As an example, a 1x1 transparent GIF is 43 bytes.

This is what I was getting at when I asked if anything could be valid in 8 bytes.  I don't think anything can, and I don't think anything can in 14 bytes either, though I'm not totally sure.

[Peter Kasting]

Comment on attachment 80691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review

I can probably take over this patch if Adam wants as I can test-compile it and also roll in my promised cleanup of the header.

>>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:95
>>> +}
>> 
>> I think this is supposed to get a "// namespace" comment?
> 
> fwiw, that isn't a WebKit style (and several folks don't want it).

Oh.  I was going off the namespace examples in the WK style guide, which seemed to use it.  Someone should maybe remove them from that (not sure how myself).

[Adam Barth]

> I can probably take over this patch if Adam wants as I can test-compile it and also roll in my promised cleanup of the header.

Go for it.

[Peter Kasting]

Created attachment 80833 [details]
Larger patch

This is Adam's patch plus:
* Convert two strncmp() calls to memcmp()
* Clean up comments in header some
* Rename/eliminate a couple functions in header (required touching various callers/impls)

[WebKit Review Bot]

Attachment 80833 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7683750

[Peter Kasting]

Created attachment 80841 [details]
Larger patch v1.1

[David Levin]

Comment on attachment 80841 [details]
Larger patch v1.1

View in context: https://bugs.webkit.org/attachment.cgi?id=80841&action=review

Sorry to r- over misc issues in comments but since that is what this patch is about it seems appropriate.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:52
> +    // ImageFrame represents the decoded image data.  This buffer is what all

Since you're fixing up the comments consider adjusting the ones that you fix have only one space after periods which is typical in WebKit (although not in the style guide yet because I haven't had a chance to update it).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:115
> +        // (This pointer will be owned by BitmapImage and freed in

Is this last sentence needed? It could be owned by and freed by any code and this code would not be affected. 

Also you added an open paren but no close paren.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:191
> +        IntSize m_size; // The size of the buffer.  This should be the same as

Would m_bufferSize be a better name? (and then the first sentence seems redundant).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:196
> +        IntRect m_rect; // The rect of the original specified frame within the

m_originalFrameRect? (I'm not sure if this rename helps or not. Just an idea.)

> Source/WebCore/platform/image-decoders/ImageDecoder.h:226
> +        // Factory function to create an appropriate ImageDecoder.  Returns 0 if

Does this first sentence add anything?

It feels like the thing missing from this comment is that the caller need to take ownership of the returned value (but changing it to PassOwnPtr in another change will fix that w/o a comment).

> Source/WebCore/platform/image-decoders/ImageDecoder.h:243
> +        // Lazily-decodes enough of the image to get the size (if possible).

I don't understand this comment as this function doesn't lazily decode anything.  Perhaps it should be removed.

It just does "return !m_failed && m_sizeAvailable;9"

> Source/WebCore/platform/image-decoders/ImageDecoder.h:262
> +        // frame.

I'm trying to shorten this comment. How about

"Returns the size of a particular frame which may vary for formats like ICO, where each frame represents a different icon.  Notably, for GIF, the size is constant because decoding makes all frames the same size."

Still a bit long but slightly shorter.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:268
> +        // Sets the image size.  Returns whether the size is legal (i.e. not

It looks like the first sentence may go away.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:280
> +        // Lazily-decodes enough of the image to get the frame count (if

This base implementation doesn't appear to lazily decode anything.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:287
> +        // ImageDecoder-owned pointer.

Why is this "Decodes as much of the requested frame as possible" notable?

I would guess that the function would decode the frame buffer at the given index due to its name. Is it notable because it may not decode the whole frame?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
> +        // frames.  In practice this is used on large animated GIFs to save

It feels like you could remove everything before "In practice" (and I'm not sure if that is needed either as long as I can grep the code for it because usage may change).

[David Levin]

(In reply to comment #13)
> (From update of attachment 80691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80691&action=review
> 
> I can probably take over this patch if Adam wants as I can test-compile it and also roll in my promised cleanup of the header.
> 
> >>> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:95
> >>> +}
> >> 
> >> I think this is supposed to get a "// namespace" comment?
> > 
> > fwiw, that isn't a WebKit style (and several folks don't want it).
> 
> Oh.  I was going off the namespace examples in the WK style guide, which seemed to use it.  Someone should maybe remove them from that (not sure how myself).

fwiw,
http://trac.webkit.org/browser/trunk/Websites/webkit.org/coding/coding-style.html


Discussion:
  http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563,
  https://lists.webkit.org/pipermail/webkit-dev/2010-August/013759.html
You'll also find a few supports in the second thread. The short answer is that there doesn't seem to be a style rule here either way, so the guide is correct. (You can do it or not, so I usually only ask for consistency within the patch.)

[Peter Kasting]

Comment on attachment 80841 [details]
Larger patch v1.1

View in context: https://bugs.webkit.org/attachment.cgi?id=80841&action=review

New patch to be attached momentarily.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:52
>> +    // ImageFrame represents the decoded image data.  This buffer is what all
> 
> Since you're fixing up the comments consider adjusting the ones that you fix have only one space after periods which is typical in WebKit (although not in the style guide yet because I haven't had a chance to update it).

I'd prefer not to do this -- most of the Image subsystem is consistent about two-spaces-after-period.  (I'd also rather not specify this in the style guide, though I'm OK with "one space before //".)

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:115
>> +        // (This pointer will be owned by BitmapImage and freed in
> 
> Is this last sentence needed? It could be owned by and freed by any code and this code would not be affected. 
> 
> Also you added an open paren but no close paren.

Fixed the parens.

I admit that the last sentence doesn't affect the code here, but I'd like to keep it because it took a long time to understand that ownership scheme and ensure it was correct.

I added the words "Actual use:" to it in hopes of more clarity.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:191
>> +        IntSize m_size; // The size of the buffer.  This should be the same as
> 
> Would m_bufferSize be a better name? (and then the first sentence seems redundant).

Actually, the comment is flat wrong.  Deleted it entirely.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:196
>> +        IntRect m_rect; // The rect of the original specified frame within the
> 
> m_originalFrameRect? (I'm not sure if this rename helps or not. Just an idea.)

Sure, deleted the first part of the comment too.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:226
>> +        // Factory function to create an appropriate ImageDecoder.  Returns 0 if
> 
> Does this first sentence add anything?
> 
> It feels like the thing missing from this comment is that the caller need to take ownership of the returned value (but changing it to PassOwnPtr in another change will fix that w/o a comment).

Changed first sentence to instead say that the pointer is caller-owned.  I went to change this to PassOwnPtr and discovered there's some interesting typedef stuff in ImageSource.h that's going to make this trickier (because ImageSource also works with ports that use other image decoders than these), so I punted that for now.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:243
>> +        // Lazily-decodes enough of the image to get the size (if possible).
> 
> I don't understand this comment as this function doesn't lazily decode anything.  Perhaps it should be removed.
> 
> It just does "return !m_failed && m_sizeAvailable;9"

I wasn't sure how to write this comment.  The function is overridden by every decoder to do the actual lazy-decoding -- what's here is just a check that all of them call.  Eventually I'd like to factor out some more of that functionality so that this base implementation actually calls a function called decode(), but until then I still wanted to document that this function always does (and indeed must do) lazy-decode.

I tried adding a FIXME here.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:262

> 
> I'm trying to shorten this comment. How about
> 
> "Returns the size of a particular frame which may vary for formats like ICO, where each frame represents a different icon.  Notably, for GIF, the size is constant because decoding makes all frames the same size."
> 
> Still a bit long but slightly shorter.

I took a stab at shortening this.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:268
>> +        // Sets the image size.  Returns whether the size is legal (i.e. not
> 
> It looks like the first sentence may go away.

I removed it.  I'm worried, though, that this makes it sound as if the function doesn't actually set size, just checks it.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:280
>> +        // Lazily-decodes enough of the image to get the frame count (if
> 
> This base implementation doesn't appear to lazily decode anything.

Added another FIXME.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:287
>> +        // ImageDecoder-owned pointer.
> 
> Why is this "Decodes as much of the requested frame as possible" notable?
> 
> I would guess that the function would decode the frame buffer at the given index due to its name. Is it notable because it may not decode the whole frame?

Notable for two reasons:
1) As you guessed, the function may decode only part (or even none) of the frame, but it's still legal to call.
2) Conversely, the function _will_ decode what it can, even if that hasn't been decoded previously (in other words, it's not a cheap accessor).

My goal was to convey both those points, I don't know if I succeeded.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
>> +        // frames.  In practice this is used on large animated GIFs to save
> 
> It feels like you could remove everything before "In practice" (and I'm not sure if that is needed either as long as I can grep the code for it because usage may change).

I shortened the first part and removed the second.

[Peter Kasting]

Created attachment 80960 [details]
Larger patch v2

[Peter Kasting]

Created attachment 80964 [details]
Larger patch v2.1

Fix broken ChangeLog merge and typo in ImageDecoder.h (thanks David Levin!).

[David Levin]

Comment on attachment 80964 [details]
Larger patch v2.1

I did cq- simply because the patch doesn't apply cleanly (see the style or cr-linux bots).

If you want to do this through the commit queue, you can upload a new patch that applies cleanly and have the "Reviewed by NOBODY (OOPS!)." replaced with "Reviewed by David Levin."
Then only mark the bug with cq+ (leave the r field blank). 

Or you can do the commit by hand.

[Adam Barth]

> If you want to do this through the commit queue, you can upload a new patch that applies cleanly and have the "Reviewed by NOBODY (OOPS!)." replaced with "Reviewed by David Levin."
> Then only mark the bug with cq+ (leave the r field blank). 

"webkit-patch land-safely" will do all that for you.

[Peter Kasting]

Created attachment 80969 [details]
Larger patch v2.2

[WebKit Commit Bot]

Comment on attachment 80969 [details]
Larger patch v2.2

Clearing flags on attachment: 80969

Committed r77427: <http://trac.webkit.org/changeset/77427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77427 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Chromium Linux Release, and EFL Linux Release (Build)