Issue 3165041: Adding a utility class for detecting Jpeg image attributes (e.g. compression quality). (Closed)
DescriptionAdding a utility class that can detect subsampling, compression quality and
huffman optimization properties for jpeg images.
This can aid in doing more intelligent optimizations for jpegs in image compression rewriters such as BasicImageRewriter.
For e.g. an image with low compression quality (e.g. 50) need not be compressed with quality 90 since this will only result in increasing the image size.
(Jpeg Reference: http://www.bsdg.org/swag/GRAPHICS/0143.PAS.html)
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing the comments. #
Total comments: 12
Patch Set 3 : Add more funtionality #Patch Set 4 : Added more comments. #Patch Set 5 : Addressing the gagan's comments. #
Total comments: 26
Patch Set 6 : Addressing the gagan's comments. #
Total comments: 16
Patch Set 7 : Addressing the gagan's comments. #
Total comments: 4
Patch Set 8 : Addressing the gagan's comments. #Patch Set 9 : Small refactoring #Patch Set 10 : Minor style fixes #Patch Set 11 : Adding more documentation as per Paul's suggestion. #Patch Set 12 : Addressing the comments. #
MessagesTotal messages: 20
looks good http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java (right): http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:232: if (marker == 0xffd9) make it a const with descriptive name http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:239: // Skip precision, height, width bytes. explain the number of bytes of precison, height and width and how the number 5 came http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:261: // 4:4:4 subsampling -> 0x11 -> 17 probably more readable to make a enum out of it. http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:268: return 0; return -1, thats more clear that some error occured. maybe log a warning as well.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java (right): http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:232: if (marker == 0xffd9) On 2010/11/17 10:32:19, gagan.goku wrote: > make it a const with descriptive name Done. http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:239: // Skip precision, height, width bytes. On 2010/11/17 10:32:19, gagan.goku wrote: > explain the number of bytes of precison, height and width and how the number 5 > came Done. http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:261: // 4:4:4 subsampling -> 0x11 -> 17 On 2010/11/17 10:32:19, gagan.goku wrote: > probably more readable to make a enum out of it. Done. http://codereview.appspot.com/3165041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:268: return 0; On 2010/11/17 10:32:19, gagan.goku wrote: > return -1, thats more clear that some error occured. > maybe log a warning as well. Done.
Sign in to reply to this message.
Also, please pick an existing image and read its subsampling image as a test case. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java (right): http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:236: * 'SOFO' marker. you meant 'SOF' marker ? http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:245: * - sampling factors (bit 0-3 vert., 4-7 hor.) please expand: 2nd byte: sampling factors (bits 0-3 for vertical sampling, 4-7 for horizontal) this might be clearer to you, but not to new novice users. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:278: // Skip precision(1 Byte), height(2 Bytes), width(2 bytes) bytes. give a new line between comment and prev line http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:288: INVALID_JPEG_ERROR_MSG))); might be better to give specific error messages, like "could not read sampling for Y" http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:301: // 4:4:4 subsampling -> 0x11 -> 17 you can move this comment to the top where the enum is defined. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:305: if (subsampling == SamplingModes.YUV444.getModeValue()) { there should be an easier way of doing this without if else. i dont know if there is a simple way to parse the given mode, but at the very least you could iterate over all values of enum using something like: for (SamplingModes mode : SamplingModes.values()) { } and then compare with the value ur looking for.
Sign in to reply to this message.
Addressing the gagan's comments.
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java (right): http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:236: * 'SOFO' marker. On 2010/11/17 17:31:42, gagan.goku wrote: > you meant 'SOF' marker ? Actually it is SOF0 marker. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:245: * - sampling factors (bit 0-3 vert., 4-7 hor.) On 2010/11/17 17:31:42, gagan.goku wrote: > please expand: > 2nd byte: sampling factors (bits 0-3 for vertical sampling, 4-7 for horizontal) > > this might be clearer to you, but not to new novice users. Done. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:278: // Skip precision(1 Byte), height(2 Bytes), width(2 bytes) bytes. On 2010/11/17 17:31:42, gagan.goku wrote: > give a new line between comment and prev line Done. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:288: INVALID_JPEG_ERROR_MSG))); On 2010/11/17 17:31:42, gagan.goku wrote: > might be better to give specific error messages, like "could not read sampling > for Y" Done. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:301: // 4:4:4 subsampling -> 0x11 -> 17 On 2010/11/17 17:31:42, gagan.goku wrote: > you can move this comment to the top where the enum is defined. Reafactored this a bit. See if it is ok now. http://codereview.appspot.com/3165041/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java:305: if (subsampling == SamplingModes.YUV444.getModeValue()) { On 2010/11/17 17:31:42, gagan.goku wrote: > there should be an easier way of doing this without if else. > i dont know if there is a simple way to parse the given mode, but at the very > least you could iterate over all values of enum using something like: > > > for (SamplingModes mode : SamplingModes.values()) { > } > > and then compare with the value ur looking for. Done.
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:122: public float approximateQuality(int[] table, int[] stdTable) { give comments that: 1) quality is defined in terms of the table used by encoder. Q = quant table, and S = table used by encoder, if q > 0.5 then Q = 2 - 2*q*S otherwise Q = (0.5/q)*S Also mention that we dont have access to the table used by encoder. But it is close to the standard table defined by JPEG. Hence, we approx by taking sum of all values of the standard JPEG table and comparing with sum of all values of quant table. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:221: * - quantization table number quantization table index http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:223: private void parseSOFNSegment(int markerLength, byte[] segmentData) parseSOFSegment http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:231: if (toBeProcessed > 6) { add else for this where you log some error. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:239: if (numComponents == 3 && toBeProcessed == 9) { add else for this where you log some error. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:258: * This function tries to parse the Quantizations tables and adds them to JpegImageData mention that if segmentData has more bytes after parsing first DQT, that means QT segment has multiple quantization tables. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:263: * bit 4..7: precision of QT, 0 = 8 bit, otherwise 16 bit bit 4..7: precision of QT, 0 means 8 bits, 1 means 16 bit otherwise bad input. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:267: * information byte. also mention that we currently support only 1 or 2 DQT's per segment. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:289: imageData.addQTable(tableIndex, quanTable); mention in the function comment that that we allow multiple quant tables with same tableIndex and the latter one overrides the previous one. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:297: * Structure of DHT (Define Huffman Table) segment. mention that multiple huffman tables are possible in a DHT segment http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:299: * bit 0..3: number of HT (0..3, otherwise error) index of HT http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:310: private void parseHuffmanTables(int markerLength, byte[] segmentData) just mention that it is highly unlikely that a huffman optimized image has same number of symbols as the standard huffman table. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:334: if ((tableInfo>>4 == 0 && numSymbols != 12) || mention that the position 0 = LSB and not MSB.
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:122: public float approximateQuality(int[] table, int[] stdTable) { On 2010/11/29 13:48:13, gagan.goku wrote: > give comments that: > 1) quality is defined in terms of the table used by encoder. > Q = quant table, and S = table used by encoder, > if q > 0.5 then > Q = 2 - 2*q*S > > otherwise > Q = (0.5/q)*S > > Also mention that we dont have access to the table used by encoder. But it is > close to the standard table defined by JPEG. > Hence, we approx by taking sum of all values of the standard JPEG table and > comparing with sum of all values of quant table. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:221: * - quantization table number On 2010/11/29 13:48:13, gagan.goku wrote: > quantization table index Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:223: private void parseSOFNSegment(int markerLength, byte[] segmentData) On 2010/11/29 13:48:13, gagan.goku wrote: > parseSOFSegment Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:231: if (toBeProcessed > 6) { On 2010/11/29 13:48:13, gagan.goku wrote: > add else for this where you log some error. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:239: if (numComponents == 3 && toBeProcessed == 9) { On 2010/11/29 13:48:13, gagan.goku wrote: > add else for this where you log some error. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:258: * This function tries to parse the Quantizations tables and adds them to JpegImageData On 2010/11/29 13:48:13, gagan.goku wrote: > mention that if segmentData has more bytes after parsing first DQT, that means > QT segment has multiple quantization tables. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:263: * bit 4..7: precision of QT, 0 = 8 bit, otherwise 16 bit On 2010/11/29 13:48:13, gagan.goku wrote: > bit 4..7: precision of QT, > 0 means 8 bits, > 1 means 16 bit > otherwise bad input. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:267: * information byte. On 2010/11/29 13:48:13, gagan.goku wrote: > also mention that we currently support only 1 or 2 DQT's per segment. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:289: imageData.addQTable(tableIndex, quanTable); On 2010/11/29 13:48:13, gagan.goku wrote: > mention in the function comment that that we allow multiple quant tables with > same tableIndex and the latter one overrides the previous one. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:297: * Structure of DHT (Define Huffman Table) segment. On 2010/11/29 13:48:13, gagan.goku wrote: > mention that multiple huffman tables are possible in a DHT segment Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:299: * bit 0..3: number of HT (0..3, otherwise error) On 2010/11/29 13:48:13, gagan.goku wrote: > index of HT Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:310: private void parseHuffmanTables(int markerLength, byte[] segmentData) On 2010/11/29 13:48:13, gagan.goku wrote: > just mention that it is highly unlikely that a huffman optimized image has same > number of symbols as the standard huffman table. Done. http://codereview.appspot.com/3165041/diff/22001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:334: if ((tableInfo>>4 == 0 && numSymbols != 12) || On 2010/11/29 13:48:13, gagan.goku wrote: > mention that the position 0 = LSB and not MSB. Fixed the comments, now i think it more clear.
Sign in to reply to this message.
last round http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:224: * - image height (2 bytes, Hi-Lo), must be >0 if DNL not supported image height (2 bytes, little endian) http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:278: * - QT information (1 byte): bit 0 = LSB bit 7 = MSB http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:279: * bit 3..0: number of QT (3..0, otherwise error) index of QT (tableIndex) http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:281: * - n bytes QT, n = 64*(precision+1) n bytes QT values, n = 64*(precision+1) http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:313: * - HT information (1 byte): also mention 0 = LSB and 7 = MSB here. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:347: if ((tableInfo>>4 == 0 && numSymbols != 12) || change to ((tableInfo>>4) & 1) http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:348: (tableInfo>>4 == 1 && numSymbols != 162)) { change 12 and 162 as constants http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java (right): http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java:56: imageData.getChromaQualityFactor(); surround with assert
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:224: * - image height (2 bytes, Hi-Lo), must be >0 if DNL not supported On 2010/11/30 06:27:13, gagan.goku wrote: > image height (2 bytes, little endian) Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:278: * - QT information (1 byte): On 2010/11/30 06:27:13, gagan.goku wrote: > bit 0 = LSB > bit 7 = MSB Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:279: * bit 3..0: number of QT (3..0, otherwise error) On 2010/11/30 06:27:13, gagan.goku wrote: > index of QT (tableIndex) Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:281: * - n bytes QT, n = 64*(precision+1) On 2010/11/30 06:27:13, gagan.goku wrote: > n bytes QT values, n = 64*(precision+1) Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:313: * - HT information (1 byte): On 2010/11/30 06:27:13, gagan.goku wrote: > also mention 0 = LSB and 7 = MSB here. Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:347: if ((tableInfo>>4 == 0 && numSymbols != 12) || On 2010/11/30 06:27:13, gagan.goku wrote: > change to ((tableInfo>>4) & 1) Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:348: (tableInfo>>4 == 1 && numSymbols != 162)) { On 2010/11/30 06:27:13, gagan.goku wrote: > change 12 and 162 as constants Done. http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java (right): http://codereview.appspot.com/3165041/diff/33001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java:56: imageData.getChromaQualityFactor(); On 2010/11/30 06:27:13, gagan.goku wrote: > surround with assert Done.
Sign in to reply to this message.
LGTM. Fix small nits and send to dev@ http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:278: * and the latter one overrides the previous one. we currently parse upto 2 quantization tables. 100 chars http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java (right): http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java:56: assertTrue(imageData.getChromaQualityFactor() > 0.89); you can use assertEquals(double expected, double actual, double delta) to compare these within some delta difference.
Sign in to reply to this message.
http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java (right): http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtils.java:278: * and the latter one overrides the previous one. we currently parse upto 2 quantization tables. On 2010/11/30 06:58:42, gagan.goku wrote: > 100 chars Done. http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java (right): http://codereview.appspot.com/3165041/diff/20002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JpegImageUtilsTest.java:56: assertTrue(imageData.getChromaQualityFactor() > 0.89); On 2010/11/30 06:58:42, gagan.goku wrote: > you can use assertEquals(double expected, double actual, double delta) > to compare these within some delta difference. Done.
Sign in to reply to this message.
Small refactoring
Sign in to reply to this message.
would be nice to have proper javadoc (@param / @return / @throws) Does any of this code overlap with Sanselan? I'm happy to see this in shindig, but it seems like it might make sense to push this code back up to sanselan (and pray that they push out a release)
Sign in to reply to this message.
Will update the docs. If pushing to sanselen won't take much time, then that is better place for this code. For most part of it it uses JpegParser from Sanselen for parsing the metadata.
Sign in to reply to this message.
Don't bother blocking on sanselan. However I would suggest sending the patch to their mailing list and Henri Yandell henri@yandell.org to see if they're interested. Thanks! On 2010/12/15 16:55:28, satya3656 wrote: > Will update the docs. > > If pushing to sanselen won't take much time, then that is better place for this > code. For most part of it it uses JpegParser from Sanselen for parsing the > metadata.
Sign in to reply to this message.
Updated the javadocs. But function descriptions still look bit odd, as it tries to explain what each byte in the corresponding segment represent, so that it will be easier to understand the code. Sure, will send the patch, and check with them if they are interested in adding it directly to sanselan. Thanks, Satya On 2010/12/15 17:16:38, plindner1 wrote: > Don't bother blocking on sanselan. However I would suggest sending the patch to > their mailing list and Henri Yandell mailto:henri@yandell.org to see if they're > interested. > > Thanks! > > On 2010/12/15 16:55:28, satya3656 wrote: > > Will update the docs. > > > > If pushing to sanselen won't take much time, then that is better place for > this > > code. For most part of it it uses JpegParser from Sanselen for parsing the > > metadata.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
On 2010/12/15 17:36:25, satya3656 wrote: > Addressing the comments. Can you email me the images? Rietveld blocks access to the jpegs. thanks!
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
