Bug 28751 – Misc. small image system cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Peter Kasting]

There are a couple patches I want to write to clean up things about the image system:
* Switch ICO decoder to use Vector<OwnPtr<> > now that that works
* Use WebCore types in the decoders where possible (e.g. IntSize instead of two ints)
* Change ImageDecoder::setFailed() to return a bool so the callers can simplified
* Move RGBA32Buffer to graphics/ImageBuffer.*

None of these is drastic so I'll just file this umbrella bug to hold them all.

[Peter Kasting]

Created attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder

[Eric Seidel (no email)]

Comment on attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder

LGTM.

[Peter Kasting]

Comment on attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder

Landed in r47800, clearing flags.

[Peter Kasting]

Created attachment 38684 [details]
Shorten #ifdef section of ImageSource.h

[Peter Kasting]

Created attachment 38688 [details]
Clean up ImageSource.cpp a little

[Peter Kasting]

Created attachment 38689 [details]
Clean up ImageDecoder.cpp a little

[Peter Kasting]

Created attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

Added in the other ImageDecoder*.cpp files since I'm looking at them to figure out how to merge them together.

[Peter Kasting]

Created attachment 38694 [details]
Clean up ImageSource.* in preparation for more fixes on bug 27965

Collapsed the two ImageSource.* cleanup patches together for ease of landing.

[Eric Seidel (no email)]

Comment on attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

Why no "static" on:
 41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate, int length)
I guess inline also prevents it from ever becoming a symbol?

And here?
 55 template <MatchType type> int getScaledValue(const Vector<int>& scaledValues, int valueToMatch, int searchStart)
Does static not work on templates?

Otherwise this looks fine.

[Eric Seidel (no email)]

Comment on attachment 38694 [details]
Clean up ImageSource.* in preparation for more fixes on bug 27965

+    return m_decoder ? m_decoder->isSizeAvailable() : false;
return m_decoder && m_decoder->isSizeAvailable();
is how I would write that.

Looks OK.

[Peter Kasting]

(In reply to comment #9)
> (From update of attachment 38693 [details])
> Why no "static" on:
>  41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate,
> int length)
> 
> And here?
>  55 template <MatchType type> int getScaledValue(const Vector<int>&
> scaledValues, int valueToMatch, int searchStart)

Because they have been moved into the pre-existing anonymous namespace, so they don't also need a "static" declaration.

Anonymous namespaces and "static" are the two ways to locally scope functions; "static" is deprecated in C++ (Stroustrup secs. 9.2, B.2.3).

[Eric Seidel (no email)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 38693 [details] [details])
> > Why no "static" on:
> >  41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate,
> > int length)
> > 
> > And here?
> >  55 template <MatchType type> int getScaledValue(const Vector<int>&
> > scaledValues, int valueToMatch, int searchStart)
> 
> Because they have been moved into the pre-existing anonymous namespace, so they
> don't also need a "static" declaration.
> 
> Anonymous namespaces and "static" are the two ways to locally scope functions;
> "static" is deprecated in C++ (Stroustrup secs. 9.2, B.2.3).

Random.  I didn't realize we used anonymous namespaces in WebCore, or that static is "deprecated".

[Eric Seidel (no email)]

Comment on attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

OK.

[Peter Kasting]

Comment on attachment 38694 [details]
Clean up ImageSource.* in preparation for more fixes on bug 27965

Landed in r47840, clearing flags.

[Peter Kasting]

Comment on attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

Landed in r47841, clearing flags.

[Peter Kasting]

Created attachment 46606 [details]
Condense downsampling logic

This greatly reduces the amount of code in the decoders that support image downsampling.  It does this by making the downsampling functions available for all builds, and having them act "as you'd expect" (e.g. scaledSize() returns the same as size() when the image has not been scaled).  This allowed me to condense a lot of logic.

This also contains the fixes on bug 33624 (which I discovered while writing this patch).

There should be no functional change in this patch.

[WebKit Review Bot]

Attachment 46606 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:360:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1

[Peter Kasting]

Created attachment 46609 [details]
Condense downsampling logic, v2

Fixes style nit and condenses another few lines I didn't previously realize I could shorten.

[Yong Li]

Comment on attachment 46609 [details]
Condense downsampling logic, v2

>  #include "ImageDecoder.h"
>  #endif
>  
> -#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> -#ifndef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS
> -#define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS (1024 * 1024)
> -#endif
> -#endif
> -
>  namespace WebCore {
>  
>  ImageSource::ImageSource()
> @@ -81,6 +75,9 @@ void ImageSource::setData(SharedBuffer* 
>      if (!m_decoder) {
>          m_decoder = static_cast<NativeImageSourcePtr>(ImageDecoder::create(*data));
>  #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
> +#ifndef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS
> +#define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS (1024 * 1024)
> +#endif
>          if (m_decoder)
>              m_decoder->setMaxNumPixels(IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS);
>  #endif

Why do you move it into the function? I think I was asked to move it out before :)

[Peter Kasting]

(In reply to comment #19)
> Why do you move it into the function? I think I was asked to move it out before
> :)

For readability (to keep the constant close to where it's used) and simplicity (one fewer preprocessor block, two fewer lines of code).

[WebKit Review Bot]

Attachment 46609 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/186745

[Peter Kasting]

Created attachment 46614 [details]
Condense downsampling logic, v3

Try to fix both build warnings and errors on gtk.  I intended to reorder the ImageDecoder.h member variables in a later patch but I guess I have to do a little bit to get things in the right order now.

[Adam Barth]

Comment on attachment 46614 [details]
Condense downsampling logic, v3

I'm not an expert here, but the minus lines outnumber the plus lines and the plus lines are prettier.

More seriously, I looked through this and the parts I understood looked good.  There were some calculations that mystified me, but looks plausible.  If there's a better reviewer for this code, please let me or Peter know.

[Peter Kasting]

Comment on attachment 46614 [details]
Condense downsampling logic, v3

Landed in r53309, clearing flags.

[Eric Seidel (no email)]

Broke Chromium Linux:
http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/932/steps/compile-webkit/logs/stdio

I'm not sure why the EWS bot didn't catch that...

[Peter Kasting]

(In reply to comment #25)
> Broke Chromium Linux:
> http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/932/steps/compile-webkit/logs/stdio
> 
> I'm not sure why the EWS bot didn't catch that...

Attempted fix landed as r53311.

[Peter Kasting]

Created attachment 46709 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate

This moves the private reader objects in several decoders from being raw pointers to OwnPtrs, to simplify the code a little.  Since I'm touching this part of the GIF decoder anyway, it also removes GIFImageDecoderPrivate, since that wrapper class doesn't seem to add much value beyond simplifying the "decode" API call, and it takes up a big chunk of space.

[Peter Kasting]

Holger, since the latest patch touches Qt code (though trivially so), thought you might want to take a look and/or r+.

[WebKit Review Bot]

Attachment 46709 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/190557

[Peter Kasting]

Created attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

Try to fix Qt build error.

[Adam Barth]

Comment on attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

This patch is fantastic.

[Peter Kasting]

Comment on attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

Landed in r53411, clearing flags.

[Peter Kasting]

Created attachment 48689 [details]
Style/code cleanup

This somewhat stretches the definition of a "small" cleanup, but it's all mechanical, with no functional change (if you find one, flag it!).  I'm just trying to make the Image Decoders more compact, readable, and style-guide-compliant.  In particular, this gets rid of the 80-column wrapping in code originally written by me, which Eric Seidel has frequently complained to me about :)

[WebKit Review Bot]

Attachment 48689 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
itespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:72:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:178:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:240:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:250:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:276:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:288:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:293:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:309:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:368:  term_source is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:112:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:121:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:61:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:184:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:187:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:318:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 27


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 48690 [details]
Style/code cleanup, v2

This fixes the fixable style comments.  There are two cases the style bot will still complain about that aren't fixed:

int var;   // Indenting these comments the same
bool var2; // causes the style bot to complain

void jpeg_decompress_struct(); // Funcs/structs used by libjpeg are named/styled to match its conventions, which makes the style bot angry

I don't think fixing either of these is advantageous.

[WebKit Review Bot]

Attachment 48690 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:63:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:63:  setjmp_buffer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:67:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:70:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:72:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:87:  Declaration has space between type name and * in JPEGImageReader *decoder  [whitespace/declaration] [3]
WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:369:  term_source is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/image-decoders/ImageDecoder.h:58:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:59:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:60:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:184:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/image-decoders/ImageDecoder.h:187:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 12


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> void jpeg_decompress_struct(); // Funcs/structs used by libjpeg are
> named/styled to match its conventions, which makes the style bot angry

We can whitelist these files not to check for underscores in names.  Do you know the full set of affected image decoder files?

[Adam Barth]

Comment on attachment 48690 [details]
Style/code cleanup, v2

I didn't verify everything in detail, but this patch looks great.  Thanks for doing this cleanup!

[Peter Kasting]

Comment on attachment 48690 [details]
Style/code cleanup, v2

Landed in r54823, clearing flags.

[Peter Kasting]

Created attachment 48960 [details]
Saner PNG-in-ICO decoding

This makes PNG-in-ICO decoding a little less convoluted.

[Adam Barth]

Comment on attachment 48960 [details]
Saner PNG-in-ICO decoding

I didn't 100% understand the changes to ICOImageDecoder::decodeAtIndex, but the patch looks good.

Can we retire this bug and use a new one for future patches?

[Peter Kasting]

Landed in r54978.  Closing bug per comment 41.

[Adam Barth]

Thanks Peter.