-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setting jpeg quality in JPEGCodec #26
Conversation
As a simple way to make the jpeg quality property settable, e.g. for use in QuPath, the approach of this PR uses System.setProperty() and System.getProperty to support adjustable jpeg quality while avoiding passing the 'jpegquality' value from QuPath through the entire class hierarchy from QuPath to Bioformats to ome.codecs. Related to: ome/bioformats#3370
Thanks @iwbh15 for getting the PR open. I have tagged it for inclusion in our CI builds and tests, I will aim to get a full review of the PR next week. |
Conflicting PR. Removed from build BIOFORMATS-push#376. See the console output for more details.
--conflicts |
Conflicting PR. Removed from build BIOFORMATS-push#1525. See the console output for more details.
--conflicts |
String OME_JPEGQUALITY = "ome.codec.jpegquality"; | ||
|
||
double jpegquality = 0.75; | ||
String sJPEGQuality = System.getProperty(OME_JPEGQUALITY, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general 👍 for making this configurable. I went looking to see if I could find a way that this could be passed via CodecOptions
or MetadataOptions
so this wouldn't be pinned to the entire JVM. I didn't see a way and assume it would be an API addition to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another option that has a decisive influence on the Jpeg image quality: 'Chroma Subsampling'. Unfortunately, access to the 'Chroma Sampling' property is not configurable via the 'ImageWriteParam' (like e.g. the JpegQuality). Unfortunately, so far have not been able to find a direct and simple description. In ImageJ, access to the 'Chroma Sampling' property is already integrated. For example, chroma subsampling can be disabled with the following macro command: My source at that time was Apache/Shindig. There is a test script from Wayne here https://imagej.nih.gov/ij/macros/js/JpegQualityTests.js . I think it would be worth and important to have the 'Chroma Sampling' property in mind, especially with the current code changes to support variable Jpeg quality. Unfortunately I lack the overview of ome-codecs to elaborate this hint more concretely. Here are some further and interesting links: |
Conflicting PR. Removed from build BIOFORMATS-push#401. See the console output for more details.
--conflicts |
Conflicting PR. Removed from build BIOFORMATS-push#1553. See the console output for more details.
--conflicts |
Conflicting PR. Removed from build BIOFORMATS-push#402. See the console output for more details.
--conflicts |
@iwbh15, agreed that having the option to disable the sub sampling similar to ImageJ seems like a good idea here. You should be able to reuse much of the same code from the ImageJ implementation in https://github.com/imagej/ImageJ/blob/master/ij/plugin/JpegWriter.java#L80-L123, perhaps pulling the core of it out into a separate method (https://github.com/ome/bioformats/blob/develop/components/formats-api/src/loci/formats/codec/CodecOptions.java) Also to handle the configuration of this option it would also be best to do this through the CodecOptions. This would mean opening an accompanying PR to add an additional option and default value to https://github.com/ome/bioformats/blob/develop/components/formats-api/src/loci/formats/codec/CodecOptions.java Perhaps the Chroma Subsampling could be a handled in a follow up PR after we have got these initial JPEG quality changes merged. |
Conflicting PR. Removed from build BIOFORMATS-push#403. See the console output for more details.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the changes here look good, manually testing using the CodecOptions showed the quality value being correctly passed from the CodecOptions into the compressionQuality of the jpegWriteParams.
PR looks good to merge.
Follow up to PR ome#26 see ome#26 (comment)
As a simple way to make the jpeg quality property settable, e.g. for use in QuPath, the approach of this PR uses System.setProperty() and System.getProperty to support adjustable jpeg quality while avoiding passing the 'jpegquality' value from QuPath through the entire class hierarchy from QuPath to Bioformats to ome.codecs.
Related to:
ome/bioformats#3370