[OpenJDK 2D-Dev] RFR 6488522: PNG writer should permit setting compression level and iDAT chunk maximum size

[Sergey Bylokhov]

Only one thing remain which I would like to clarify.

In the PNGImageWriter the new field deflaterLevel was added(it can be 
private?). This field is initialized by default to 
DEFAULT_COMPRESSION_LEVEL and updated according to the current params. 
Is it possible a situation if two images will use the same writer. 
During the write of the first image deflaterLevel will be changed via 
params. But in the write of the second image the params will be reset to 
null -> the deflaterLevel was not changed. It seems that in this case 
for the second image non-default level will be used?


On 11/12/15 02:45, Laurent Bourgès wrote:
> 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


-- 
Best regards, Sergey.