Hi Sergey, Here is the updated webrev: http://cr.openjdk.java.net/~lbourges/png/PNGWriter-6488522.1/
> A few notes: > - Should PNGImageWriteParam.unsetCompression resets the compressionType > to compressionTypes[0]? > Fixed. > - Is it necessary to copy the checks in PNGImageWriteParam from the > parent? We can just call super() and expect that the parent class validate > current state. > Agreed: I simply call super.getCompressionQualityXXX() before returning the appropriate value. > Yes, I could check that every png file is not zero-length or that better >> compression gives smaller file size ! >> > > It would be good, because right now it check not all changes in the fix. > Fixed: I now check that better compression gives smaller file (but it may be not always the case as some algorithms use several passes or an adaptive strategy ...) > However, my changes rely on the public Deflater API that supports >> compression levels between 0 to 9. >> >> I suppose that this Deflater class is already covered by its own tests >> so I would prefer leaving the test as it is (no verification): if the >> Deflater raises any RuntimeException, then the PNGWriterCompressionTest >> will fail. >> > > But this test is passed even for such fix: > > --- > a/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java > Thu Dec 10 14:21:44 2015 +0300 > +++ > b/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java > Thu Dec 10 15:15:41 2015 +0300 > @@ -280,6 +280,7 @@ > super(); > this.canWriteProgressive = true; > this.locale = locale; > + this.canWriteCompressed = true; > } > } > Agreed, but if the size remains constant, it is not always a failure ! > > Why not but it is a different scope ? >> >> I wrote the test focused on testing my patch to validate all my changes >> and fix ASAP that very old bug (created in 2006) ! >> > > I understood, but to change the test and cover of existed/future plugins > should not require a big effort. It does not mean that you need to fix some > bugs(if any then create a new CR) in other plugins. > See for example this bug JDK-8144245 which was found in a new plugin just > because the test cover all of them. But this is up2you. > I tried generalizing it and moved it into shared/ImageWriterCompressionTest. However I disabled bmp/gif as the file compression does not work but also for JPG !!! It seems jpg fails with the native error (on jdk9/client) but it works on my jdk8_60: Caused by: javax.imageio.IIOException: Invalid argument to native writeImage Look at the IGNORE_FILE_SUFFIXES to re-enable JPEGImageWriter test if you want. To concluse: only PNG and the new TIFF pass (only LZW compression type is tested). Regards, Laurent
