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

[Laurent Bourgès]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20151211/c3e9bfcb/attachment.html>