IMAGING-251 support for TIFF floating-point formats #72
Conversation
There was a problem hiding this comment.
Hi @gwlucastrig , thanks for the PR!
I think Travis is unhappy with some checkstyle issues. Take a look at its output and it should tell you the line and error it found 👍 we need this to pass before merging it. Will review it over next days.
|
I ran check style and see that I introduced tabs and trailing spaces. Serves me right. I turned off the auto-format in my IDE because I didn't want to introduce white-space changes to existing code. I ran checkstyle from Maven using "mvn checkstyle:checkstyle". Is that the command the project recommends? What should I do to address this? Submit a new PR? |
You can run just Some developers are able to integrate the checkstyle rules in their IDE's. I am a bit behind the time, and still use the command line for most things. So my workflow is normally to change the files, have a look at the IDE warnings (Eclipse), then Doesn't work every time as I can forget to run it too :)
In your branch you should be able to resolve the style issues, by editing the files Travis is showing with errors. Then run After this, this pull request should be automatically updated. If you look near the PR title, where it says "gwlucastrig wants to merge 1 commit into...", you should see that this PR is supposed to merge code into But I can see you didn't use a branch in your fork, as it says that it would merge it from "gwlucastrig:master". It will work fine. But now this PR is "linked" to your branch Which means that if you start working on another issue, and you push to your Furthermore, once it's merged, you may have to use something like For that, I always start new work with That way I am able to work on multiple issues 👍 just a note, as we should be able to work on IMAGING-251 just fine this way, but just in case you want to work on multiple issues, then having branches and not using |
|
Thanks for the information. It clarifies some of the confusing area of
submitting a pill request. I have been using git for about a year now, but
still don't understand the underlying logic of the system.
…On Sun, Apr 5, 2020, 6:20 PM Bruno P. Kinoshita ***@***.***> wrote:
I ran check style and see that I introduced tabs and trailing spaces.
Serves me right. I turned off the auto-format in my IDE because I didn't
want to introduce white-space changes to existing code. I ran checkstyle
from Maven using "mvn checkstyle:checkstyle". Is that the command the
project recommends?
You can run just mvn as Commons Imaging has a default Maven goal in its
pom.xml. That's how Travis does it
<https://github.com/apache/commons-imaging/blob/1397ca92cd3268b434ad8a18529ee3544bbcf3c5/.travis.yml#L32>
too.
Some developers are able to integrate the checkstyle rules in their IDE's.
I am a bit behind the time, and still use the command line for most things.
So my workflow is normally to change the files, have a look at the IDE
warnings (Eclipse), then mvn and prepare a commit (i.e. run tests and
some other checks).
Doesn't work every time as I can forget to run it too :)
What should I do to address this? Submit a new PR?
In your branch you should be able to resolve the style issues, by editing
the files Travis is showing with errors. Then run mvn and once it passes
prepare a new commit and push to your fork.
After this, this pull request should be automatically updated.
------------------------------
If you look near the PR title, where it says "gwlucastrig wants to merge 1
commit into...", you should see that this PR is supposed to merge code into
apache:master.
But I can see you didn't use a branch in your fork, as it says that it
would merge it from "gwlucastrig:master". It will work fine. But now this
PR is "linked" to your branch master on your fork.
Which means that if you start working on another issue, and you push to
your master branch, that would update this PR as well.
Furthermore, once it's merged, you may have to use something like git
reset --hard to sync your fork again.
For that, I always start new work with git checkout -b name-of-my-branch
(where normally I have name-of-my-branch as something like IMAGING-123,
or fix-color-palette-8-bits-etc.
That way I am able to work on multiple issues 👍 just a note, as we
should be able to work on IMAGING-251 just fine this way, but just in case
you want to work on multiple issues, then having branches and not using
master in your fork might be helpful.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#72 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWJDYIDN44BQ4HSOQDNCB3RLD7Z5ANCNFSM4MAWGVRQ>
.
|
|
I've pushed the checkstyle fixes up to my branch. Let me know if any
further action is required on my part. I am continuing to work toward
completing the changes for the issue. As a side effect, these changes may
eventually address Issue 102.
Gary
…On Mon, Apr 6, 2020 at 6:21 AM Gary Lucas ***@***.***> wrote:
Thanks for the information. It clarifies some of the confusing area of
submitting a pill request. I have been using git for about a year now, but
still don't understand the underlying logic of the system.
On Sun, Apr 5, 2020, 6:20 PM Bruno P. Kinoshita ***@***.***>
wrote:
> I ran check style and see that I introduced tabs and trailing spaces.
> Serves me right. I turned off the auto-format in my IDE because I didn't
> want to introduce white-space changes to existing code. I ran checkstyle
> from Maven using "mvn checkstyle:checkstyle". Is that the command the
> project recommends?
>
> You can run just mvn as Commons Imaging has a default Maven goal in its
> pom.xml. That's how Travis does it
> <https://github.com/apache/commons-imaging/blob/1397ca92cd3268b434ad8a18529ee3544bbcf3c5/.travis.yml#L32>
> too.
>
> Some developers are able to integrate the checkstyle rules in their
> IDE's. I am a bit behind the time, and still use the command line for most
> things. So my workflow is normally to change the files, have a look at the
> IDE warnings (Eclipse), then mvn and prepare a commit (i.e. run tests
> and some other checks).
>
> Doesn't work every time as I can forget to run it too :)
>
> What should I do to address this? Submit a new PR?
>
> In your branch you should be able to resolve the style issues, by editing
> the files Travis is showing with errors. Then run mvn and once it passes
> prepare a new commit and push to your fork.
>
> After this, this pull request should be automatically updated.
> ------------------------------
>
> If you look near the PR title, where it says "gwlucastrig wants to merge
> 1 commit into...", you should see that this PR is supposed to merge code
> into apache:master.
>
> But I can see you didn't use a branch in your fork, as it says that it
> would merge it from "gwlucastrig:master". It will work fine. But now this
> PR is "linked" to your branch master on your fork.
>
> Which means that if you start working on another issue, and you push to
> your master branch, that would update this PR as well.
>
> Furthermore, once it's merged, you may have to use something like git
> reset --hard to sync your fork again.
>
> For that, I always start new work with git checkout -b name-of-my-branch
> (where normally I have name-of-my-branch as something like IMAGING-123,
> or fix-color-palette-8-bits-etc.
>
> That way I am able to work on multiple issues 👍 just a note, as we
> should be able to work on IMAGING-251 just fine this way, but just in case
> you want to work on multiple issues, then having branches and not using
> master in your fork might be helpful.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#72 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEWJDYIDN44BQ4HSOQDNCB3RLD7Z5ANCNFSM4MAWGVRQ>
> .
>
|
|
This time it will work, I hope. I ran mvn findbugs:findbugs and, sure enough, it found a bug. I've submitted a fix. Unfortunately, I missed a lot of the build procedures because I have not been able to get maven to execute the site lifecycle under Windows (I do miss having a Linux computer). Apparently, it won't run if I don't have a version of subversion installed. Trying to get that set up now. Is there a recommended version that works with commons-imaging? |
|
I am using Maven 3.5.4, with Java 8. Not sure if there's a recommended version @gwlucastrig . But you are doing just fine regarding build & git in my opinion. The code part you are doing is really the hard part. Also, because of how GitHub and git work, I am also able to edit this branch. So if necessary I can amend the changes to help fixing issues, or edit the commits (e.g. squashing it). So feel free to update it as much as you can, but if you get stuck you can let me know and I can try to help so that the build passes and we can focus on the new code. |
There was a problem hiding this comment.
Few comments from a quick review Gary. Already know I will need some more time to read the code and (great) comments with calm. Will send a pull request later to update some javadocs, and will try to update the test code to use JUnit 👍
I saw you found some test data too, that's really great!
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
Show resolved
Hide resolved
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
Show resolved
Hide resolved
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
Outdated
Show resolved
Hide resolved
| @@ -115,7 +264,7 @@ private void interpretTile(final ImageBuilder imageBuilder, final byte[] bytes, | |||
|
|
|||
| // End of May 2012 changes | |||
|
|
|||
| try (final BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) { | |||
| try (BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) { | |||
There was a problem hiding this comment.
Why did you have to remove the final here?
| int yMax; | ||
|
|
||
| double sumFound; | ||
| int nFound; |
There was a problem hiding this comment.
Could we make the members above private or some other access modifier?
|
Thanks for the review and comments. In answer to your first question, I
removed the final modifiers because the PMD analysis cited them as
unnecessary. I figured I would take the opportunity to reduce some of the
PMD issues. There were also some unnecessary parentheses cited in the PMD
analysis. I have mixed feelings about that one because what's unnecessary
for Java is sometimes useful for a human being. But it this case I didn't
think they helped the code.
In answer to your second comment, I agree. All of those member elements
should be private.
In terms of the documentation. Thanks for the positive feedback. I had a
hard time figuring out the details, some of which were (at least for me)
not intuitive. So I wanted to document them to make things easier for the
next guy. Consequently, the Commons Imaging project may be the only place
on the Internet where the floating-point predictor format is clearly
documented.
Gary
…On Tue, Apr 7, 2020 at 3:32 AM Bruno P. Kinoshita ***@***.***> wrote:
***@***.**** commented on this pull request.
Few comments from a quick review Gary. Already know I will need some more
time to read the code and (great) comments with calm. Will send a pull
request later to update some javadocs, and will try to update the test code
to use JUnit 👍
I saw you found some test data too, that's really great!
------------------------------
In
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
<#72 (comment)>:
> @@ -166,7 +166,7 @@ private void interpretStrip(
// this logic will handle all cases not conforming to the
// special case handled above
- try (final BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
+ try (BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
Why did you need to remove the final here?
------------------------------
In
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
<#72 (comment)>:
> + *
+ * Once the predictor transform is complete, the data is stored using
+ * conventional data compression techniques such as Deflate or LZW.
+ * In practice, floating point data does not compress especially well, but
+ * using the above technique, the TIFF process typically reduces the overall
+ * storage size by 20 to 30 percent (depending on the data).
+ * The TIFF Technical Note 3 specifies 3 data size formats for
+ * storing floating point values:
+ * 32 bits IEEE-754 single-precision standard
+ * 16 bits IEEE-754 half-precision standard
+ * 24 bits A non-standard representation
+ * At this time, we have not obtained data samples for the smaller
+ * representations. There are also reports of 64-bit data
+ * (see Commons Imaging JIRA issue IMAGING-102), though documentation
+ * for that format was not available when these notes were written.
+ */
This-is-some-great-documentation! I will send a pull request later to
format it as Javadoc while I re-read it with more calm 👍 👏
------------------------------
In
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
<#72 (comment)>:
> @@ -61,7 +139,78 @@ public DataReaderTiled(final TiffDirectory directory,
}
private void interpretTile(final ImageBuilder imageBuilder, final byte[] bytes,
- final int startX, final int startY, final int xLimit, final int yLimit) throws ImageReadException, IOException {
+ final int startX, final int startY, final int xLimit, final int yLimit) throws ImageReadException, IOException {
+
+ // March 2020 change to handle floating-point with compression
+ // for the compressed floating-point, there is a standard that allows
+ // 16 bit floats (which is an IEEE 754 standard) and 24 bits (which is
+ // a non-standard format implemented for TIFF). At this time, this
+ // code only supports 32-bite.
s/bite/bits?
------------------------------
In
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
<#72 (comment)>:
> @@ -115,7 +264,7 @@ private void interpretTile(final ImageBuilder imageBuilder, final byte[] bytes,
// End of May 2012 changes
- try (final BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
+ try (BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
Why did you have to remove the final here?
------------------------------
In
src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloat.java
<#72 (comment)>:
> + *
+ */
+public class PhotometricInterpreterFloat extends PhotometricInterpreter {
+
+ ArrayList<IPaletteEntry> rangePaletteEntries = new ArrayList<>();
+ ArrayList<IPaletteEntry> singleValuePaletteEntries = new ArrayList<>();
+
+ float minFound = Float.POSITIVE_INFINITY;
+ float maxFound = Float.NEGATIVE_INFINITY;
+ int xMin;
+ int yMin;
+ int xMax;
+ int yMax;
+
+ double sumFound;
+ int nFound;
Could we make the members above private or some other access modifier?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#72 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEWJDYIVCVGRYQPNDAEKLCTRLLJJNANCNFSM4MAWGVRQ>
.
|
|
Let me know what you'd like me to do to with the code. Sorry if the PMD changes created more work for you than they were worth. I can back them out if you'd like. |
|
I think I will be able to improve the coverage as soon as I can get some test images to exercise the sections of code that are not currently being covered. I'm a bit confused by Coverall's results because in the last update I made no code changes, but added additional unit tests to cover more of the code. I expected that to improve the coverage rating, but it actually went down. No luck in my search for relevant test images, so I am trying to figure out how to use the Commons Imaging API to write TIFF files that carry floating-point data. That way the project will be able to generate its own test images with the different combination of options. Most of the pieces are already present in the API. The original authors put a remarkable amount of work into the code... But figuring out how to use the API is not always easy. At least at first, I will not create test files that use the predictor/compressors for floating-point data. The predictors have a lot of nuances and I wouldn't trust my code to produce "valid" TIFF for those combinations of options where I don't have standard samples to test against. All the samples I have so far are 32-bit tile-based files. I've found hundreds of those from the US Geological Survey. But I haven't found any strip-based files. |
|
The 14 April commit (e390778) concludes the preliminary work of enabling Commons Imaging to read images from TIFF files that are based on floating-point data. I am now shifting to implementing new functionality to extract floating-point raster data from the TIFF. I will be adding new blocks of code to the data-reader classes and the TiffImageParser, but do not plan on any further modifications to the existing code. |
|
New classes and methods for accessing floating-point data are now included. Only a few details remain for the code is ready for review. I extended some of the unit tests, but still need to add new ones for the TiffRasterData and TiffRasterStatistics classes. I also have a new example class and want to see if I can raise the Coverall scores for a few of my changes. I hope to have it ready by the end of the weekend. |
|
Had all sorts of trouble with maven reporting a Package Container issue on the verify phase. Naturally, it offered no useful information about what the problem could be. Code compiled fine. I had to back out all my changes and start over. The only thing I can think of is that it may be related to a Java versioning issue because I added lambda's to the photometric interpreters (as suggested in the code review, which I thought was a good idea). But that seems unlikely since my JDK is version 1.8.0_211. So I am puzzled. Anyway, I am past that and will continue working to integrate the recommendations related to the photometric interpreters. |
|
I tracked down the change that resulted in the Maven failure. Since I haven't really studied Maven (I just treat it as a black box), I'm not really sure how to interpret this. But I am working around the problem by simply not making the change. In PhotometricInterpreterFloat.java, line 158. I change the code from to the following I get a report of an API incompatibility in animal-sniffer-maven-plugin:1.17 complaining |
|
Found the problem. The CharBuffer.position() method appears to have been introduced in Java 9. So the Java 8 API that I build with doesn't support it. I have been very reluctant to use more recent versions of Java because of Oracle's changes to licensing policies. I may have to revise my thinking on that one. |
|
Hopefully there should be some way (maybe not as simple) to achieve the same with Java 8 @gwlucastrig . If not we can think in other alternatives. |
|
I believe that I have now integrated all the changes from the review comments with two exceptions. The TiffRasterData.getData() method still exposes and internal array. However, I've added Javadoc explaining the rationale and use of the method. The Collections.sort() methods in PhotometricInterpreterFloat are still there. When I made the recommended changes, animal-sniffer threw an exception. I then installed Java JDK 11 (picking 11 because that's what Travis uses) and the same thing happened. So I am leaving the older code in place. My plan is to do a bit more testing over the next few days. I do not plan to make any further code changes unless I encounter an unexpected error. |
|
Excellent Gary! I have time to review it again this weekend. I think it should be ready to merge now if I find nothing else in the review Should we delay mergimg until you finish your tests? |
|
All my tests completed successfully. I have some pretty nice pictures to show for it. When accepted, I believe you will be able to close both JIRA item 251 and 102. I am currently working on a tutorial showing how to use the Commons Imaging API to read elevation data. When it's done, I will be posting it on my wiki page at Gridfour Wiki. I will, of course, make sure it links to Commons Imaging. |
|
Nice! Will start going through it tomorrow. Thanks!!!! |
| * Defines an interface for specifying color assignments to floating point | ||
| * values. | ||
| */ | ||
| public interface IPaletteEntry { |
There was a problem hiding this comment.
No need to have the I in interface names (there were components using I and A—for abstract— but that changed long ago).
|
No blockers so far, another 9 files to go, and then review is done. Comments added now can either be fixed before merging, or afterwards. @gwlucastrig do you mind if when merging I squash the commits when merging? |
|
@kinow I would prefer that you squash the commits. I didn't know that there was a git command that allowed you to do that. I renamed IPaletteEntry to simple PaletteEntry. I got a list of all Java files in the commons-imaging source code and saw that none of the others used the leading-I convention. So it makes sense to rename the class. |
|
Brilliant! Then tomorrow I will finish the review. Final things I'm looking at are
If nothing is wrong, then I will just squash. Several ways of doing it I think; I normally do with Thanks for updating it so quickly! |
Some of the new code is not covered by tests. But alas I think it's quite hard to find images for these tests. Later we may look into having some sort of image generator, or try to locate more test images. https://coveralls.io/builds/30831964
|
Nothing obvious. A fuzzer could still find some images that could lead to exceptions, but I couldn't find a case where it would lead to DDoS attacks (:crossed_fingers:) |
Nothing too drastic—i.e. API changes OK IMHO for next alpha release (this will change once we have 1.0 out) |
|
Approved. Rebasing and merging. Thanks @gwlucastrig ! |
This change adds the ability to obtain images from TIFF files that give data in the floating-point format. It partially addresses IMAGING-251. It also adds two example applications to show how to use the Commons Imaging API for TIFF images and adds one additional small sample TIFF data file that uses the floating-point format.
I saw that the project pom.xml specified PMD, so I run PMD and made minor PMD changes. In keeping with the project guidelines, PMD changes were done only for the files that I otherwise modified.
I ran all test cases successfully.
Additional changes to support numerical access for these files will be provided in a future patch.