[
https://issues.apache.org/jira/browse/IMAGING-251?focusedWorklogId=417619&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-417619
]
ASF GitHub Bot logged work on IMAGING-251:
------------------------------------------
Author: ASF GitHub Bot
Created on: 07/Apr/20 10:41
Start Date: 07/Apr/20 10:41
Worklog Time Spent: 10m
Work Description: gwlucastrig commented on issue #72: IMAGING-251 support
for TIFF floating-point formats
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610313655
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 <[email protected]>
wrote:
> *@kinow* 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
> <https://github.com/apache/commons-imaging/pull/72#discussion_r404590691>:
>
> > @@ -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
> <https://github.com/apache/commons-imaging/pull/72#discussion_r404591250>:
>
> > + *
>
> + * 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
> <https://github.com/apache/commons-imaging/pull/72#discussion_r404591417>:
>
> > @@ -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
> <https://github.com/apache/commons-imaging/pull/72#discussion_r404592789>:
>
> > @@ -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
> <https://github.com/apache/commons-imaging/pull/72#discussion_r404594350>:
>
> > + *
>
> + */
>
> +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
>
<https://github.com/apache/commons-imaging/pull/72#pullrequestreview-388848081>,
> or unsubscribe
>
<https://github.com/notifications/unsubscribe-auth/AEWJDYIVCVGRYQPNDAEKLCTRLLJJNANCNFSM4MAWGVRQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 417619)
Time Spent: 2h (was: 1h 50m)
> Support TIFF standard floating point data
> -----------------------------------------
>
> Key: IMAGING-251
> URL: https://issues.apache.org/jira/browse/IMAGING-251
> Project: Commons Imaging
> Issue Type: New Feature
> Components: Format: TIFF
> Affects Versions: 1.x
> Reporter: Gary Lucas
> Priority: Major
> Fix For: 1.x
>
> Attachments: Imaging252_USGS_n38w077.jpg
>
> Time Spent: 2h
> Remaining Estimate: 0h
>
> Commons Imaging does not support the floating-point format included in the
> TIFF specification. There are prominent data sources that issue products in
> this format. The ability to support this information would open up new
> application areas for Commons Imaging.
> TIFF is often used as a mechanism for distributing data from geophysical
> applications in the form of GeoTIFF files. Some of this is not imagery, but
> data. For example, the US Geological Survey is currently releasing
> high-resolution elevation data grids for the 3DEP program under the name
> Cloud-Optimized GeoTIFF (COG). It is a substantial data set with significant
> potential commercial and academic applications.
> To access this data means modifying the TIFF DataReaderStrips and
> DataReaderTile classes to recognize floating point data (which is typically
> indicated using TIFF tag #339, SampleFormat). Also, returning the data in the
> form of a BufferedImage makes no sense at all, so the API on the
> TiffImageParser and supporting classes would need additional methods to
> return arrays of floats. The good news here is that that requirement would
> mean adding new methods to the classes rather than making significant changes
> to existing classes. So the probability of unintended consequences or new
> bugs in existing code would be minimized.
> Specification details for floating-point are given in the main TIFF-6
> documentations and Adobe Photoshop TIFF Technical Note 3.
>
> I am willing to volunteer to make these changes provided that there is
> interest and a high probability that my contributions would be evaluated and,
> if suitable, integrated into the Commons Imaging code base.
> Thank you for your attention in this matter.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
