Adding Huffmann Size Optimization to Jpeg Compression Params. (gives +(5-7)% of compression)

This change adds setOptimizeHuffmanTables(true) to the Jpeg params. (gives additional 6% of compression without any addition loss in the image quality)

Since the generalized call for ImageIOOutputter.write(new IIOImage()) has a bug, that ignores this parameter while enocding, we are using more specific call for writing for jpeg images that respects this new parameter.
Refer: com.sun.imageio.plugins.jpeg.JPEGImageWriter.java:352 write(...) function.

Refactored PNGOptimizer.java to remove some redundant code.

Note: The old flow also has a hidden bug that sets wrong 'Default Quantization Tables' while doing jpeg compression with 'ImageWriteParam.MODE_EXPLICIT' that results in encode phase of jpeg effectively use different compression quality (> than config.getJpegCompression()). This bug fix effectively gives +4% more compression in bytes.
Refer: (http://www.java2s.com/Open-Source/Java-Document/6.0-JDK-Modules-com.sun/imageio/com.sun.imageio.plugins.jpeg.htm)
BaseOptimizer.java:172 in writer.getDefaultImageMetadata(...) --->
com.sun.imageio.plugins.jpeg.JPEGImageWriter.java:609 --->  com.sun.imageio.plugins.jpeg.DQTMarkerSegment.java:215 in Qtable(boolean wantLuma, float quality) function, K2Div2Chrominance is used instead of K2Chrominance.

[gagan.goku, 2010-10-08 07:50:22]

Looks good. Small nitpicks to fix:

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

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:78:
JPEGImageWriteParam jpegParam = new JPEGImageWriteParam(param.getLocale());
just add a comment saying that all the default paramters provided by:
ImageWriteParam param = writer.getDefaultWriteParam();

have been copied over to jpegParam in this line:
JPEGImageWriteParam jpegParam = new JPEGImageWriteParam(param.getLocale());

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:82:
return new ImageIOOutputter(writer, jpegParam);
afger line 81, say:
param = jpegParam

and then remove the else block.

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:174:
IIOMetadata metadata = writer.getDefaultImageMetadata(
Just add a comment here saying that:
writer.getDefaultImageMetadata(new ImageTypeSpecifier(image.getColorModel(),
image.getSampleModel()), writeParam);

does buggy preprocessing for ImageWriteParam. Hence passing null as
ImageWriteParam here to ignore this preprocessing and passing the
ImageWriteParam later in the writer.write() call.

[satya3656, 2010-10-08 12:46:39]

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

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:78:
JPEGImageWriteParam jpegParam = new JPEGImageWriteParam(param.getLocale());
On 2010/10/08 07:50:22, gagan.goku wrote:
> just add a comment saying that all the default paramters provided by:
> ImageWriteParam param = writer.getDefaultWriteParam();
> 
> have been copied over to jpegParam in this line:
> JPEGImageWriteParam jpegParam = new JPEGImageWriteParam(param.getLocale());

Done.

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:82:
return new ImageIOOutputter(writer, jpegParam);
On 2010/10/08 07:50:22, gagan.goku wrote:
> afger line 81, say:
> param = jpegParam
> 
> and then remove the else block.

Done.

http://codereview.appspot.com/2322042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:174:
IIOMetadata metadata = writer.getDefaultImageMetadata(
On 2010/10/08 07:50:22, gagan.goku wrote:
> Just add a comment here saying that:
> writer.getDefaultImageMetadata(new ImageTypeSpecifier(image.getColorModel(),
> image.getSampleModel()), writeParam);
> 
> does buggy preprocessing for ImageWriteParam. Hence passing null as
> ImageWriteParam here to ignore this preprocessing and passing the
> ImageWriteParam later in the writer.write() call.

Done.

[satya3656, 2010-10-08 12:47:02]

Addressing the comments.

[satya3656, 2010-10-08 15:06:56]

Upadated a small condition

[gagan.goku, 2010-10-08 15:53:01]

Change looks good. Please send out to dev@ after making these small changes and
ask them how to make the image where you can test the png -> jpg change when the
gain is more.
Kudos for fixing the bugs and enabling huffman correctly :)

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:84:
((JPEGImageWriteParam) param).setOptimizeHuffmanTables(true);
make this a flag through OptimizerConfig.

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizerTest.java
(right):

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizerTest.java:49:
assertEquals("image/jpeg", httpResponse.getHeader("Content-Type"));
you should add a test where the image is retained as png because of low gains.

[satya3656, 2010-10-09 07:20:14]

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:84:
((JPEGImageWriteParam) param).setOptimizeHuffmanTables(true);
On 2010/10/08 15:53:01, gagan.goku wrote:
> make this a flag through OptimizerConfig.

Made it a flag and set default value to false.

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizerTest.java
(right):

http://codereview.appspot.com/2322042/diff/11001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizerTest.java:49:
assertEquals("image/jpeg", httpResponse.getHeader("Content-Type"));
On 2010/10/08 15:53:01, gagan.goku wrote:
> you should add a test where the image is retained as png because of low gains.

Now default is false, so this testcase is not breaking.
We should add a addition test case for the other path though.

[satya3656, 2010-10-09 07:20:32]

Added huffman optimization as a configurable through shindig properties

[satya3656, 2010-10-09 14:31:52]

sync to head(r1006167)

[gagan.goku, 2010-10-10 04:33:23]

LGTM.

Also, please ask on the dev community / add TODO's in the right places to test
with images when huffman is enabled.
You can look at the revision history of image optimizations directory to find
the right person to ask:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apa...

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java
(right):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:54:
* Defaults
would be nice if you mention that these are being used only for tests.

[anupama.dutta, 2010-10-11 06:09:48]

Some minor comments below. LGTM overall.

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:29:
import com.sun.imageio.plugins.jpeg.JPEGImageWriter;
Order imports?

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:81:
// This optimization reduces the images size by addition 4-6% without
Move this comment to shindig.properties.

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizer.java
(left):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizer.java:92:
ImageWriter writer = ImageIO.getImageWritersByFormatName("jpeg").next();
A small note in the CL description indicating that this change (in
PNGOptimizer.java) is a refactoring change, will be helpful.

[satya3656, 2010-10-11 07:52:12]

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:29:
import com.sun.imageio.plugins.jpeg.JPEGImageWriter;
On 2010/10/11 06:09:49, anupama.dutta wrote:
> Order imports?

These two imports are from different places, i guess order is correct.

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:81:
// This optimization reduces the images size by addition 4-6% without
On 2010/10/11 06:09:49, anupama.dutta wrote:
> Move this comment to shindig.properties.

Done.

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java
(right):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:54:
* Defaults
On 2010/10/10 04:33:23, gagan.goku wrote:
> would be nice if you mention that these are being used only for tests.

Done.

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizer.java
(left):

http://codereview.appspot.com/2322042/diff/30001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/PNGOptimizer.java:92:
ImageWriter writer = ImageIO.getImageWritersByFormatName("jpeg").next();
On 2010/10/11 06:09:49, anupama.dutta wrote:
> A small note in the CL description indicating that this change (in
> PNGOptimizer.java) is a refactoring change, will be helpful.

Done.

[satya3656, 2010-10-11 07:55:32]

Addressing the comments.

[johnfargo, 2010-10-14 23:20:36]

great work; just one structuring comment.

http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:185:
writeParam);
Could you use this same method call signature at line 190? If so, I'd recommend
doing:
ImageWriteParam metadataWriteParam = writeParam;
if (writer instanceof JPEGImageWriter) {
  // Comment about workaround
  metadataWriteParam = null;
}
writer.write(...);

[satya3656, 2010-10-18 07:49:39]

Addressing the comments.

[satya3656, 2010-10-18 07:51:04]

http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:185:
writeParam);
On 2010/10/14 23:20:37, johnfargo wrote:
> Could you use this same method call signature at line 190? If so, I'd
recommend
> doing:
> ImageWriteParam metadataWriteParam = writeParam;
> if (writer instanceof JPEGImageWriter) {
>   // Comment about workaround
>   metadataWriteParam = null;
> }
> writer.write(...);

Done.

[johnfargo, 2010-10-19 04:27:18]

LGTM

Thanks Satya!

On 2010/10/18 07:51:04, satya3656 wrote:
>
http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
> (right):
> 
>
http://codereview.appspot.com/2322042/diff/41001/java/gadgets/src/main/java/o...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:185:
> writeParam);
> On 2010/10/14 23:20:37, johnfargo wrote:
> > Could you use this same method call signature at line 190? If so, I'd
> recommend
> > doing:
> > ImageWriteParam metadataWriteParam = writeParam;
> > if (writer instanceof JPEGImageWriter) {
> >   // Comment about workaround
> >   metadataWriteParam = null;
> > }
> > writer.write(...);
> 
> Done.

[satya3656, 2010-10-19 06:45:44]

sync to head(r1024129)

[gagan.goku, 2010-10-21 04:12:10]

Build looks good.
Committed as r1025817.