Issue 3082041: Support for retaining subsampling parameter for jpeg image rewrite. (Closed)
|
Created:
15 years, 4 months ago by satya3656 Modified:
15 years, 1 month ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionSupport for retaining subsampling parameter for jpeg image rewrite.
Patch Set 1 #Patch Set 2 : Added optional subsample parameter #
Total comments: 23
Patch Set 3 : Fixing the comments #
Total comments: 8
Patch Set 4 : Addressing the comments. #Patch Set 5 : Adding Tests #
Total comments: 4
Patch Set 6 : Addressing the comments. #Patch Set 7 : Add more funtionality #Patch Set 8 : Small refactoring #
Total comments: 4
Patch Set 9 : Addressing Nikhil's comments #Patch Set 10 : Svn up #
Total comments: 6
Patch Set 11 : Addressing Gagan's comments #
Total comments: 2
MessagesTotal messages: 32
Added optional subsample parameter
Sign in to reply to this message.
Thanks for debugging and making this highly tricky change! Please also add a few more details to the patch description, so that the necessity for this change is clear. http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... java/common/conf/shindig.properties:75: shindig.image-rewrite.jpeg-444-subsample = false enable-jpeg-444-subsampling? http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java:43: outputter = new ImageIOOutputter(writer, null, config.getJpeg444Subsample()); It seems somewhat counter-intuitive that we need to pass along a "jpeg" specific flag value to other image format optimizers (like this one - BMPOptimizer). Just wondering if there are any other options available (similar to how you added the huffman param to only Jpeg specific cases in http://codereview.appspot.com/2322042/). http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:163: boolean do444Subsample; It will be good to use similar names for this parameter throughout (e.g isJpeg444SubsamplingEnabled) for methods, flag names and variables, so that it is easier to track/understand the feature. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:193: metaImageWriteParam); If I understand right, ideally our ImageWriteParam (or IIOParam as explained in http://download.oracle.com/javase/6/docs/api/javax/imageio/IIOParam.html) could have carried the subsampling information right here. But, since we are forced to pass null here (because of the buggy processing you mention in the above comment), we need to create the IIOMetadata instance and update it in the next line. Did I understand this correctly? http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:205: private void UpdateMetadataFor444Subsampling(IIOMetadata metadata) UpdateMetadataFor444Subsampling -> enableJpeg444Subsampling? http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:207: // Tweaking the image metadta to override default subsampling(4:2:0) with metadta -> metadata http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:218: String name = node.getNodeName(); name -> nodeName toLowerCase needed here? http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:223: NodeList componentSpecs =node.getChildNodes(); Space after =. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:236: // read the updated metadata from the metadata node tree. read -> Read http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:307: Revert this change? http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:44: @Named("shindig.image-rewrite.jpeg-444-subsample") boolean jpeg444Subsample) { Indentation off.
Sign in to reply to this message.
Thanks Anupama for the quick review, will fix the comments soon. Just replied regarding the IIOParam. On 13 November 2010 20:39, <anupama.dutta@gmail.com> wrote: > Thanks for debugging and making this highly tricky change! > Please also add a few more details to the patch description, so that the > necessity for this change is clear. > > > > http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... > File java/common/conf/shindig.properties (right): > > > http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... > java/common/conf/shindig.properties:75: > shindig.image-rewrite.jpeg-444-subsample = false > enable-jpeg-444-subsampling? > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java > (right): > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java:43: > outputter = new ImageIOOutputter(writer, null, > config.getJpeg444Subsample()); > It seems somewhat counter-intuitive that we need to pass along a "jpeg" > specific flag value to other image format optimizers (like this one - > BMPOptimizer). Just wondering if there are any other options available > (similar to how you added the huffman param to only Jpeg specific cases > in http://codereview.appspot.com/2322042/). > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java > (right): > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:163: > boolean do444Subsample; > It will be good to use similar names for this parameter throughout (e.g > isJpeg444SubsamplingEnabled) for methods, flag names and variables, so > that it is easier to track/understand the feature. > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:193: > metaImageWriteParam); > If I understand right, ideally our ImageWriteParam (or IIOParam as > explained in > http://download.oracle.com/javase/6/docs/api/javax/imageio/IIOParam.html) > could have carried the subsampling information right here. But, since we > are forced to pass null here (because of the buggy processing you > mention in the above comment), we need to create the IIOMetadata > instance and update it in the next line. Did I understand this > correctly? > > IIOParam values are applied to entire image, not for individual channels like luminance and chroma channels. These two are completely different. Subsampling for jpeg images is kind of hard coded to use 4:2:0, that why i have to read constructed metadata into a node tree, and update the relevant values to get the effect of 4:4:4 subsampling, and write back the metadata object from the updated node tree. Currently i have made it work, but i still need to check all corner cases to aviod NPE's in the new code snippet. > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:205: > private void UpdateMetadataFor444Subsampling(IIOMetadata metadata) > UpdateMetadataFor444Subsampling -> enableJpeg444Subsampling? > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:207: > // Tweaking the image metadta to override default subsampling(4:2:0) > with > metadta -> metadata > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:218: > String name = node.getNodeName(); > name -> nodeName > toLowerCase needed here? > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:223: > NodeList componentSpecs =node.getChildNodes(); > Space after =. > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:236: > // read the updated metadata from the metadata node tree. > read -> Read > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java > (right): > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:307: > > Revert this change? > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java > (right): > > > http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:44: > @Named("shindig.image-rewrite.jpeg-444-subsample") boolean > jpeg444Subsample) { > Indentation off. > > > http://codereview.appspot.com/3082041/ >
Sign in to reply to this message.
Fixing the comments
Sign in to reply to this message.
Fixing the comments
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/3082041/diff/3001/java/common/conf/shindig.prop... java/common/conf/shindig.properties:75: shindig.image-rewrite.jpeg-444-subsample = false On 2010/11/13 15:09:29, anupama.dutta wrote: > enable-jpeg-444-subsampling? Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BMPOptimizer.java:43: outputter = new ImageIOOutputter(writer, null, config.getJpeg444Subsample()); Output gets everything it needs from Writeparam, since it is not part of the writeparam we need to send it. BTW, we can just use false here. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:163: boolean do444Subsample; On 2010/11/13 15:09:29, anupama.dutta wrote: > It will be good to use similar names for this parameter throughout (e.g > isJpeg444SubsamplingEnabled) for methods, flag names and variables, so that it > is easier to track/understand the feature. Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:193: metaImageWriteParam); On 2010/11/13 15:09:29, anupama.dutta wrote: > If I understand right, ideally our ImageWriteParam (or IIOParam as explained in > http://download.oracle.com/javase/6/docs/api/javax/imageio/IIOParam.html) could > have carried the subsampling information right here. But, since we are forced to > pass null here (because of the buggy processing you mention in the above > comment), we need to create the IIOMetadata instance and update it in the next > line. Did I understand this correctly? Explained in my previous mail http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:205: private void UpdateMetadataFor444Subsampling(IIOMetadata metadata) On 2010/11/13 15:09:29, anupama.dutta wrote: > UpdateMetadataFor444Subsampling -> enableJpeg444Subsampling? Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:207: // Tweaking the image metadta to override default subsampling(4:2:0) with On 2010/11/13 15:09:29, anupama.dutta wrote: > metadta -> metadata Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:218: String name = node.getNodeName(); On 2010/11/13 15:09:29, anupama.dutta wrote: > name -> nodeName > toLowerCase needed here? Not needed http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:223: NodeList componentSpecs =node.getChildNodes(); On 2010/11/13 15:09:29, anupama.dutta wrote: > Space after =. Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:236: // read the updated metadata from the metadata node tree. On 2010/11/13 15:09:29, anupama.dutta wrote: > read -> Read Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:307: On 2010/11/13 15:09:29, anupama.dutta wrote: > Revert this change? Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:307: On 2010/11/13 15:09:29, anupama.dutta wrote: > Revert this change? Done. http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java (right): http://codereview.appspot.com/3082041/diff/3001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:44: @Named("shindig.image-rewrite.jpeg-444-subsample") boolean jpeg444Subsample) { On 2010/11/13 15:09:29, anupama.dutta wrote: > Indentation off. Done.
Sign in to reply to this message.
LGTM. Gagan, could you take a look, after which this could be sent out to dev@?
Sign in to reply to this message.
Maybe add a test case that ensures that 4:4:4 is happening when you set the config param to true (if its not too much work) http://codereview.appspot.com/3082041/diff/12003/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/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:214: if (rootNode.getLastChild() != null) { Please add a comment here explaining that first level of tree has only 2 nodes and 2nd node contains markers http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:222: if (node.getNodeName().equals("sof") && node.hasChildNodes() && equalsIgnoreCase maybe ? http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:223: node.getChildNodes().getLength() == 3) { like we discussed, also give a comment telling that 3 child nodes corresponds to Y Cb Cr and 1 for greyscale 4 for YCMK http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:224: NodeList componentSpecs = node.getChildNodes(); you can getFirstChild
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/12003/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/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:214: if (rootNode.getLastChild() != null) { On 2010/11/16 04:41:20, gagan.goku wrote: > Please add a comment here explaining that first level of tree has only 2 nodes > and 2nd node contains markers Done. http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:222: if (node.getNodeName().equals("sof") && node.hasChildNodes() && On 2010/11/16 04:41:20, gagan.goku wrote: > equalsIgnoreCase maybe ? Not needed, but safe to have it. http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:223: node.getChildNodes().getLength() == 3) { On 2010/11/16 04:41:20, gagan.goku wrote: > like we discussed, also give a comment telling that 3 child nodes corresponds to > Y Cb Cr > and 1 for greyscale > 4 for YCMK Done. http://codereview.appspot.com/3082041/diff/12003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:224: NodeList componentSpecs = node.getChildNodes(); On 2010/11/16 04:41:20, gagan.goku wrote: > you can getFirstChild Done.
Sign in to reply to this message.
Lgtm for code. As discussed, please add a test if possible
Sign in to reply to this message.
Adding Test
Sign in to reply to this message.
Adding Tests
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java (right): http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:107: createResponse("org/apache/shindig/gadgets/rewrite/image/testImage420.jpg", "image/jpeg"); can we not convert a png to jpeg with 4:4:4 enabled, and dump the image. you can externally check to make sure that the image is 4:4:4. Here all your testing is that if size is greater than original, then original is returned. http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:112: HttpResponse rewrite(HttpResponse original, OptimizerConfig config) you could make another function HttpResponse rewrite(HttpResponse original) { return rewrite(original, new OptimizerConfig()); } this will remove many of the diffs.
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java (right): http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:103: public void test444Subsmapling() throws Exception { you can ignore this change since http://codereview.appspot.com/3165041/ already adds functionality of reading subsampling image.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java (right): http://codereview.appspot.com/3082041/diff/75001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:112: HttpResponse rewrite(HttpResponse original, OptimizerConfig config) On 2010/11/17 10:04:22, gagan.goku wrote: > you could make another function > HttpResponse rewrite(HttpResponse original) { > return rewrite(original, new OptimizerConfig()); > } > > this will remove many of the diffs. Done.
Sign in to reply to this message.
Addressing the gagan's comments.
Sign in to reply to this message.
Small refactoring
Sign in to reply to this message.
Small refactoring
Sign in to reply to this message.
LGTM http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:26: import org.w3c.dom.NamedNodeMap; Order imports alphabetically, here and below. http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:100: outputter = new ImageIOOutputter(writer, param, samplingMode); This is slightly confusing since we could be setting outputter in a function called getOutputter. Was there anything wrong with the previous way of doing this?
Sign in to reply to this message.
Addressing Nikhil's comments
Sign in to reply to this message.
Addressing Nikhil's comments
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:26: import org.w3c.dom.NamedNodeMap; On 2010/12/30 11:55:54, nikhilmadan23 wrote: > Order imports alphabetically, here and below. Done. http://codereview.appspot.com/3082041/diff/150001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:100: outputter = new ImageIOOutputter(writer, param, samplingMode); Previously it is initialized in the constructor. Now i need to pass more state to output namely source parameters, which needs bigger code change if need to a add a one more constructor argument. And hence this. Made minor refactoring, just check if this one looks better.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:77: this.sourceImageParams = sourceImageParams; why not add sourceImageParams param to the constructor? or add a new constructor which takes in sourceImageParams, and call initializeOutputter from both constructors. and get rid of extra calls to getOutputter() in the functions below. http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java:37: remove extra sapce http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:112: public boolean getJpegRetainSubsampling() { fix indentation
Sign in to reply to this message.
Addressing Gagan's comments
Sign in to reply to this message.
http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:77: this.sourceImageParams = sourceImageParams; On 2011/02/02 19:27:49, gagan.goku wrote: > why not add sourceImageParams param to the constructor? > or add a new constructor which takes in sourceImageParams, and call > initializeOutputter from both constructors. > and get rid of extra calls to getOutputter() in the functions below. Done. http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java:37: On 2011/02/02 19:27:49, gagan.goku wrote: > remove extra sapce Done. http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java (right): http://codereview.appspot.com/3082041/diff/192001/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/OptimizerConfig.java:112: public boolean getJpegRetainSubsampling() { On 2011/02/02 19:27:49, gagan.goku wrote: > fix indentation Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/3082041/diff/204004/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/3082041/diff/204004/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:68: protected JpegImageUtils.JpegImageParams sourceImageParams; make this final http://codereview.appspot.com/3082041/diff/204004/java/gadgets/src/main/java/... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java (right): http://codereview.appspot.com/3082041/diff/204004/java/gadgets/src/main/java/... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:309: jpegImageParams = JpegImageUtils.getJpegImageData(response.getContentBytes(), uri.getPath()); 100 chars
Sign in to reply to this message.
|
