Image content rewriting

This code is the basis for integrating image rewriting into the Shindig proxy pipeline. This code will try to make reasonable assumptions about what target image format is most suitable and then serialize to that format. If the content is smaller than the original then it is used. A fairly high degree of configurability is added and protections to ensure that rogue images cannot create exceesively large memory burdens on the VM

For sample traffic seen on Orkut this kind of rewriting can yield pretty substantial reductions in image size. Ill post more thorough results later.

Details
GIF - We dont rewrite animated GIFs though we could it is an expensive process doing frame to frame differencing. We also dont rewrite transparent GIF currently though we could later with a reasonable AlphaImageLoader solution for IE6

PNG - Most PNG writers dont do a good job stripping metadata or minimizing palette and bit depth, or they use palettes when the image is small. This will fix most of this. We also attempt to rewrite to JPEG if the image is opaque and only if it yields a significant improvement ( > 20%)  over an efficient PNG. JPEG should only provide significant improvement if the image is photographic in nature.

JPEG - We strip metadata and allow for configurable compression level.

[awiner, 2008-12-12 23:47:03]

A quick glance at the non-test classes.

http://codereview.appspot.com/10841/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/21#newcode35
Line 35: ImageReader reader;
these three can be final, as can a few others

http://codereview.appspot.com/10841/diff/1/21#newcode55
Line 55: this.baos = new ByteArrayOutputStream(original.getContentLength());
this will allocate a byte[] of size getContentLength(), which is wasteful for
large images that we're not going to resize anyway;  should lazily instantiate

http://codereview.appspot.com/10841/diff/1/21#newcode73
Line 73: if (image == null) return;
Is: 

if (image == null) {
  return;
}

preferred code-style 'round these parts?

http://codereview.appspot.com/10841/diff/1/16
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):

http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
log, don't printStackTrace()

http://codereview.appspot.com/10841/diff/1/16#newcode114
Line 114: return new HttpResponseBuilder(original)
<< 2 chars

[etnu00, 2008-12-13 01:26:22]

http://codereview.appspot.com/10841/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/21#newcode63
Line 63: * @throws IOException
No point in the javadoc tags if there's no explanation.

http://codereview.appspot.com/10841/diff/1/21#newcode92
Line 92: public HttpResponse rewrite() throws IOException {
It might be a good idea to call this method 'optimize' to avoid confusion with
ContentRewriter.

http://codereview.appspot.com/10841/diff/1/18
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/18#newcode60
Line 60: write(ImageUtils.palettize(image, config.getMaxPaletteSize()));
What happens if getMaxPaletteSize returns something larger than 16 bits?
Shouldn't we check this and not bother calling palettize?

http://codereview.appspot.com/10841/diff/1/16
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):

http://codereview.appspot.com/10841/diff/1/16#newcode59
Line 59: * @param response - Rewritten response
I think this should be 'response to rewrite'.

http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
ImageRewriteException ?

http://codereview.appspot.com/10841/diff/1/16#newcode110
Line 110: String path = uri.getPath().toLowerCase();
Is this really worth passing the uri around for?

http://codereview.appspot.com/10841/diff/1/19
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/19#newcode28
Line 28: * Optimize JPEG images by either converting them to PNGs to storing
them with a more
s/to/or/ ?

http://codereview.appspot.com/10841/diff/1/19#newcode47
Line 47: config.getIgnoreThresholdBytes()), originalResponse);
This is hard to read.

http://codereview.appspot.com/10841/diff/1/7
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizerTest.java
(right):

http://codereview.appspot.com/10841/diff/1/7#newcode30
Line 30: */
Your code is under attack by stray asterisks.

[louiscryan, 2009-01-22 23:56:19]

Rewrite to use Sanselan library to handler attacks from malicious images and
vulnerabilities in the ImageIO libraries.

ImageRewriter now an interface with a default impl.

http://codereview.appspot.com/10841/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/21#newcode35
Line 35: ImageReader reader;
On 2008/12/12 23:47:03, awiner wrote:
> these three can be final, as can a few others

Done.

http://codereview.appspot.com/10841/diff/1/21#newcode55
Line 55: this.baos = new ByteArrayOutputStream(original.getContentLength());
On 2008/12/12 23:47:03, awiner wrote:
> this will allocate a byte[] of size getContentLength(), which is wasteful for
> large images that we're not going to resize anyway;  should lazily instantiate

Done.

http://codereview.appspot.com/10841/diff/1/21#newcode63
Line 63: * @throws IOException
On 2008/12/13 01:26:22, etnu00 wrote:
> No point in the javadoc tags if there's no explanation.

Done.

http://codereview.appspot.com/10841/diff/1/21#newcode73
Line 73: if (image == null) return;
On 2008/12/12 23:47:03, awiner wrote:
> Is: 
> 
> if (image == null) {
>   return;
> }
> 
> preferred code-style 'round these parts?

Done.

http://codereview.appspot.com/10841/diff/1/21#newcode92
Line 92: public HttpResponse rewrite() throws IOException {
On 2008/12/13 01:26:22, etnu00 wrote:
> It might be a good idea to call this method 'optimize' to avoid confusion with
> ContentRewriter.

I think re-write is OK for the moment. ImageRewriter may become a
ContentRewriter later

http://codereview.appspot.com/10841/diff/1/18
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/18#newcode60
Line 60: write(ImageUtils.palettize(image, config.getMaxPaletteSize()));
On 2008/12/13 01:26:22, etnu00 wrote:
> What happens if getMaxPaletteSize returns something larger than 16 bits?
> Shouldn't we check this and not bother calling palettize?

It shouldnt have any negative impact other than unnecessary CPU cost. We still
choose an optimal bit-depth based on the no. of found colors in the image.

http://codereview.appspot.com/10841/diff/1/16
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):

http://codereview.appspot.com/10841/diff/1/16#newcode59
Line 59: * @param response - Rewritten response
On 2008/12/13 01:26:22, etnu00 wrote:
> I think this should be 'response to rewrite'.

Done.

http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
On 2008/12/12 23:47:03, awiner wrote:
> log, don't printStackTrace()

Done.

http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
On 2008/12/13 01:26:22, etnu00 wrote:
> ImageRewriteException ?

I dont think we care too much about propagating other than log messages

http://codereview.appspot.com/10841/diff/1/16#newcode110
Line 110: String path = uri.getPath().toLowerCase();
On 2008/12/13 01:26:22, etnu00 wrote:
> Is this really worth passing the uri around for?

Its not much of an overhead to pass it in currently. Only used in the fetchers.

http://codereview.appspot.com/10841/diff/1/16#newcode114
Line 114: return new HttpResponseBuilder(original)
On 2008/12/12 23:47:03, awiner wrote:
> << 2 chars

Done.

http://codereview.appspot.com/10841/diff/1/19
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1/19#newcode28
Line 28: * Optimize JPEG images by either converting them to PNGs to storing
them with a more
On 2008/12/13 01:26:22, etnu00 wrote:
> s/to/or/ ?

Done.

http://codereview.appspot.com/10841/diff/1/19#newcode47
Line 47: config.getIgnoreThresholdBytes()), originalResponse);
On 2008/12/13 01:26:22, etnu00 wrote:
> This is hard to read.

Cleaned up somewhat

http://codereview.appspot.com/10841/diff/1/7
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizerTest.java
(right):

http://codereview.appspot.com/10841/diff/1/7#newcode30
Line 30: */
On 2008/12/13 01:26:22, etnu00 wrote:
> Your code is under attack by stray asterisks.

Done.

[louiscryan, 2009-01-22 23:59:44]

Updated with security fixes

[awiner, 2009-01-27 00:46:04]

http://codereview.appspot.com/10841/diff/1001/818
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
(right):

http://codereview.appspot.com/10841/diff/1001/818#newcode57
Line 57:
bind(DefaultImageRewriter.class).annotatedWith(Names.named("org.apache.shindig.image-rewriter"))
think this should be binding ImageRewriter.class.

Not sure why we need a naming annotation here either;  especially not one with
that generic of a name.  If there's reason to have > 1 image rewriter, names
should be describe the purpose of each.

http://codereview.appspot.com/10841/diff/1001/804
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/804#newcode117
Line 117: private ByteArrayOutputStream baos;
move up with the rest of the variables

http://codereview.appspot.com/10841/diff/1001/824
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/DefaultImageRewriter.java
(right):

http://codereview.appspot.com/10841/diff/1001/824#newcode47
Line 47: * Uses the Sanselan lilbrary to parse image content and metadata to
avoid security
lilbrary -> library

http://codereview.appspot.com/10841/diff/1001/824#newcode91
Line 91: if (response.getContentLength() < config.getIgnoreThresholdBytes()) {
could be moved up to top.  And the direction seems wrong...  > than threshold
bytes, not < than...

http://codereview.appspot.com/10841/diff/1001/824#newcode104
Line 104: if (Math.abs(imageInfo.getWidth()) * Math.abs(imageInfo.getHeight()) *
Math.abs() can be negative.
Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE.

http://codereview.appspot.com/10841/diff/1001/811
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/811#newcode22
Line 22: import java.awt.*;
personal gripe against wide imports.  YMMV.

http://codereview.appspot.com/10841/diff/1001/811#newcode59
Line 59: return "gif";
should be image/gif

http://codereview.appspot.com/10841/diff/1001/811#newcode63
Line 63: return "gif";
should be image/gif

http://codereview.appspot.com/10841/diff/1001/822
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/822#newcode52
Line 52: if (pngOptimizer.minBytes != null) {
checking minBytes != null is a bit ugly.  Add BaseOptimizer.getRewrittenImage(),
avoid need to read any instance state?

http://codereview.appspot.com/10841/diff/1001/822#newcode67
Line 67: this.minBytes = pngOptimizer.minBytes;
setting minBytes shouldn't be necessary;  minBytes should only get set if JPEG
won

http://codereview.appspot.com/10841/diff/1001/815
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/10841/diff/1001/815#newcode71
Line 71: //response.setHeader("Content-Disposition",
"attachment;filename=p.txt");
Intentional?

http://codereview.appspot.com/10841/diff/1001/806
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/10841/diff/1001/806#newcode119
Line 119: //response.setHeader("Content-Disposition",
"attachment;filename=p.txt");
Intentional?

[louiscryan, 2009-01-27 20:03:56]

http://codereview.appspot.com/10841/diff/1001/818
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
(right):

http://codereview.appspot.com/10841/diff/1001/818#newcode57
Line 57:
bind(DefaultImageRewriter.class).annotatedWith(Names.named("org.apache.shindig.image-rewriter"))
On 2009/01/27 00:46:05, awiner wrote:
> think this should be binding ImageRewriter.class.
> 
> Not sure why we need a naming annotation here either;  especially not one with
> that generic of a name.  If there's reason to have > 1 image rewriter, names
> should be describe the purpose of each.

Removed completely. No need for this when  using ImplementedBy

http://codereview.appspot.com/10841/diff/1001/804
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/804#newcode117
Line 117: private ByteArrayOutputStream baos;
On 2009/01/27 00:46:05, awiner wrote:
> move up with the rest of the variables

Done.

http://codereview.appspot.com/10841/diff/1001/824
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/DefaultImageRewriter.java
(right):

http://codereview.appspot.com/10841/diff/1001/824#newcode47
Line 47: * Uses the Sanselan lilbrary to parse image content and metadata to
avoid security
On 2009/01/27 00:46:05, awiner wrote:
> lilbrary -> library

Done.

http://codereview.appspot.com/10841/diff/1001/824#newcode91
Line 91: if (response.getContentLength() < config.getIgnoreThresholdBytes()) {
On 2009/01/27 00:46:05, awiner wrote:
> could be moved up to top.  And the direction seems wrong...  > than threshold
> bytes, not < than...

Moved. Direction is correct though. We don't bother re-writing images below a
certain size. Upper bound is the max in memory size. Renamed to 

getMinThresholdBytes

http://codereview.appspot.com/10841/diff/1001/824#newcode104
Line 104: if (Math.abs(imageInfo.getWidth()) * Math.abs(imageInfo.getHeight()) *
On 2009/01/27 00:46:05, awiner wrote:
> Math.abs() can be negative.
> Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE.

Cast int to long before calling abs to workaround.

http://codereview.appspot.com/10841/diff/1001/811
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/811#newcode22
Line 22: import java.awt.*;
On 2009/01/27 00:46:05, awiner wrote:
> personal gripe against wide imports.  YMMV.

Me too. Silly IntelliJ exceptions..

http://codereview.appspot.com/10841/diff/1001/811#newcode59
Line 59: return "gif";
On 2009/01/27 00:46:05, awiner wrote:
> should be image/gif

Done.

http://codereview.appspot.com/10841/diff/1001/811#newcode63
Line 63: return "gif";
On 2009/01/27 00:46:05, awiner wrote:
> should be image/gif

Done.

http://codereview.appspot.com/10841/diff/1001/822
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java
(right):

http://codereview.appspot.com/10841/diff/1001/822#newcode52
Line 52: if (pngOptimizer.minBytes != null) {
On 2009/01/27 00:46:05, awiner wrote:
> checking minBytes != null is a bit ugly.  Add
BaseOptimizer.getRewrittenImage(),
> avoid need to read any instance state?

Done.

http://codereview.appspot.com/10841/diff/1001/822#newcode67
Line 67: this.minBytes = pngOptimizer.minBytes;
On 2009/01/27 00:46:05, awiner wrote:
> setting minBytes shouldn't be necessary;  minBytes should only get set if JPEG
> won

Done.

http://codereview.appspot.com/10841/diff/1001/815
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java
(right):

http://codereview.appspot.com/10841/diff/1001/815#newcode71
Line 71: //response.setHeader("Content-Disposition",
"attachment;filename=p.txt");
On 2009/01/27 00:46:05, awiner wrote:
> Intentional?

Testing artifact, reverted

http://codereview.appspot.com/10841/diff/1001/806
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(right):

http://codereview.appspot.com/10841/diff/1001/806#newcode119
Line 119: //response.setHeader("Content-Disposition",
"attachment;filename=p.txt");
On 2009/01/27 00:46:05, awiner wrote:
> Intentional?

test artifact, reverted

[Paul Lindner, 2009-01-27 20:27:34]

added a note about interaction with refreshInterval...

http://codereview.appspot.com/10841/diff/1001/812
File ../trunk/java/common/conf/shindig.properties (right):

http://codereview.appspot.com/10841/diff/1001/812#newcode32
Line 32: 
can we perhaps have a refreshInterval based threshhold here?  For example images
with refreshInterval=0 should definitely not be rewriten, and they are
non-cacheable..

[louiscryan, 2009-01-27 21:43:48]

http://codereview.appspot.com/10841/diff/1001/812
File ../trunk/java/common/conf/shindig.properties (right):

http://codereview.appspot.com/10841/diff/1001/812#newcode32
Line 32: 
On 2009/01/27 20:27:34, lindner wrote:
> can we perhaps have a refreshInterval based threshhold here?  For example
images
> with refreshInterval=0 should definitely not be rewriten, and they are
> non-cacheable..

Done. Will not rewrite images if no-cache or cache ttl == 0. See
BasicHttpFetcher

[louiscryan, 2009-01-27 21:50:06]

Use No-op rewriter by default. Updated as per comments