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 <notificati...@github.com> 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: us...@infra.apache.org With regards, Apache Git Services
