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

[Sergey Bylokhov]

On 09/12/15 17:17, Laurent Bourgès wrote:
> Hi Sergey,
>
> Thanks for your comments, did you review the PNGImageWriter changes too ?

A few notes:
  - Should PNGImageWriteParam.unsetCompression resets the 
compressionType to compressionTypes[0]?
  - 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.

> 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.

> 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;
      }
  }

> 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.


-- 
Best regards, Sergey.