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.
