Bug 28751 - Misc. small image system cleanups
Summary: Misc. small image system cleanups
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks: 26467
  Show dependency treegraph
 
Reported: 2009-08-26 14:40 PDT by Peter Kasting
Modified: 2010-02-18 11:57 PST (History)
7 users (show)

See Also:


Attachments
Use Vector<OwnPtr<> > in ICO decoder (3.91 KB, patch)
2009-08-26 15:34 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Shorten #ifdef section of ImageSource.h (2.04 KB, patch)
2009-08-27 14:12 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Clean up ImageSource.cpp a little (2.78 KB, patch)
2009-08-27 14:36 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Clean up ImageDecoder.cpp a little (3.52 KB, patch)
2009-08-27 14:52 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785 (8.86 KB, patch)
2009-08-27 15:30 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Clean up ImageSource.* in preparation for more fixes on bug 27965 (4.40 KB, patch)
2009-08-27 15:38 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Condense downsampling logic (27.96 KB, patch)
2010-01-14 14:37 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Condense downsampling logic, v2 (27.98 KB, patch)
2010-01-14 14:45 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Condense downsampling logic, v3 (28.49 KB, patch)
2010-01-14 15:35 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Use OwnPtrs and remove GIFImageDecoderPrivate (21.13 KB, patch)
2010-01-15 13:40 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Use OwnPtrs and remove GIFImageDecoderPrivate, v2 (21.14 KB, patch)
2010-01-15 15:54 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Style/code cleanup (112.67 KB, patch)
2010-02-12 18:55 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Style/code cleanup, v2 (115.06 KB, patch)
2010-02-12 19:07 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Saner PNG-in-ICO decoding (10.84 KB, patch)
2010-02-17 19:21 PST, Peter Kasting
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-08-26 14:40:47 PDT
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.
Comment 1 Peter Kasting 2009-08-26 15:34:15 PDT
Created attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder
Comment 2 Eric Seidel (no email) 2009-08-26 16:35:34 PDT
Comment on attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder

LGTM.
Comment 3 Peter Kasting 2009-08-26 16:40:42 PDT
Comment on attachment 38641 [details]
Use Vector<OwnPtr<> > in ICO decoder

Landed in r47800, clearing flags.
Comment 4 Peter Kasting 2009-08-27 14:12:30 PDT
Created attachment 38684 [details]
Shorten #ifdef section of ImageSource.h
Comment 5 Peter Kasting 2009-08-27 14:36:07 PDT
Created attachment 38688 [details]
Clean up ImageSource.cpp a little
Comment 6 Peter Kasting 2009-08-27 14:52:54 PDT
Created attachment 38689 [details]
Clean up ImageDecoder.cpp a little
Comment 7 Peter Kasting 2009-08-27 15:30:37 PDT
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.
Comment 8 Peter Kasting 2009-08-27 15:38:57 PDT
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.
Comment 9 Eric Seidel (no email) 2009-08-27 16:04:00 PDT
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.
Comment 10 Eric Seidel (no email) 2009-08-27 16:05:30 PDT
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.
Comment 11 Peter Kasting 2009-08-27 16:09:18 PDT
(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).
Comment 12 Eric Seidel (no email) 2009-08-27 16:10:55 PDT
(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".
Comment 13 Eric Seidel (no email) 2009-08-27 16:11:11 PDT
Comment on attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

OK.
Comment 14 Peter Kasting 2009-08-27 16:15:27 PDT
Comment on attachment 38694 [details]
Clean up ImageSource.* in preparation for more fixes on bug 27965

Landed in r47840, clearing flags.
Comment 15 Peter Kasting 2009-08-27 16:16:55 PDT
Comment on attachment 38693 [details]
Clean up ImageDecoder*.cpp in preparation for fixing bug 28785

Landed in r47841, clearing flags.
Comment 16 Peter Kasting 2010-01-14 14:37:16 PST
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.
Comment 17 WebKit Review Bot 2010-01-14 14:38:32 PST
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
Comment 18 Peter Kasting 2010-01-14 14:45:13 PST
Created attachment 46609 [details]
Condense downsampling logic, v2

Fixes style nit and condenses another few lines I didn't previously realize I could shorten.
Comment 19 Yong Li 2010-01-14 14:53:19 PST
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 :)
Comment 20 Peter Kasting 2010-01-14 14:56:38 PST
(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).
Comment 21 WebKit Review Bot 2010-01-14 15:22:48 PST
Attachment 46609 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/186745
Comment 22 Peter Kasting 2010-01-14 15:35:31 PST
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.
Comment 23 Adam Barth 2010-01-14 17:38:24 PST
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.
Comment 24 Peter Kasting 2010-01-14 17:46:30 PST
Comment on attachment 46614 [details]
Condense downsampling logic, v3

Landed in r53309, clearing flags.
Comment 25 Eric Seidel (no email) 2010-01-14 17:55:34 PST
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...
Comment 26 Peter Kasting 2010-01-14 18:05:12 PST
(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.
Comment 27 Peter Kasting 2010-01-15 13:40:42 PST
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.
Comment 28 Peter Kasting 2010-01-15 14:01:12 PST
Holger, since the latest patch touches Qt code (though trivially so), thought you might want to take a look and/or r+.
Comment 29 WebKit Review Bot 2010-01-15 15:49:02 PST
Attachment 46709 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/190557
Comment 30 Peter Kasting 2010-01-15 15:54:24 PST
Created attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

Try to fix Qt build error.
Comment 31 Adam Barth 2010-01-15 23:07:17 PST
Comment on attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

This patch is fantastic.
Comment 32 Peter Kasting 2010-01-18 09:47:25 PST
Comment on attachment 46714 [details]
Use OwnPtrs and remove GIFImageDecoderPrivate, v2

Landed in r53411, clearing flags.
Comment 33 Peter Kasting 2010-02-12 18:55:35 PST
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 :)
Comment 34 WebKit Review Bot 2010-02-12 18:57:28 PST
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.
Comment 35 Peter Kasting 2010-02-12 19:07:15 PST
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.
Comment 36 WebKit Review Bot 2010-02-12 19:08:09 PST
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.
Comment 37 Adam Barth 2010-02-12 22:05:24 PST
> 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?
Comment 38 Adam Barth 2010-02-12 22:08:35 PST
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!
Comment 39 Peter Kasting 2010-02-16 12:50:28 PST
Comment on attachment 48690 [details]
Style/code cleanup, v2

Landed in r54823, clearing flags.
Comment 40 Peter Kasting 2010-02-17 19:21:50 PST
Created attachment 48960 [details]
Saner PNG-in-ICO decoding

This makes PNG-in-ICO decoding a little less convoluted.
Comment 41 Adam Barth 2010-02-17 23:47:47 PST
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?
Comment 42 Peter Kasting 2010-02-18 11:35:54 PST
Landed in r54978.  Closing bug per comment 41.
Comment 43 Adam Barth 2010-02-18 11:57:10 PST
Thanks Peter.