Skip to content
This repository has been archived by the owner on Jun 26, 2019. It is now read-only.

Add support for TIFF compression #42

Merged
merged 3 commits into from
Jan 12, 2017
Merged

Add support for TIFF compression #42

merged 3 commits into from
Jan 12, 2017

Conversation

rleigh-codelibre
Copy link
Contributor

@rleigh-codelibre rleigh-codelibre commented Dec 18, 2016

  • Allow introspection of TIFF codec support on a per-pixeltype basis; add getCodecNames and getCodecScheme to the tiff Codec support functions
  • Allow the writer interface to query valid codecs for a given pixel type by adding getCompressionTypes(PixelType) which is a rather more useful variant than the existing getPixelTypes(codec) (which might be deprecated and removed). In the common case, you might want to select a compression type for a given set of pixel data, but altering the pixel data to use a different type to support a certain codec is less common
  • Add the supported codecs to the TIFF writers, so that they can be queried and set with the FormatWriter interface
  • Add unit tests to test with LZW and Deflate compression for the core libtiff wrappers and the Minimal TIFF writer (the OME TIFF writer is derived from this and will work identically).

Testing:

Should all be taken care of by the included unit tests. Search for FormatWriterTest.CompressionTypes and TIFFTest.FieldWrapCompression for the tiff unit tests, and TIFFWriter.CompressionTypes for the full set of compression types we support (we exclude esoteric, incompatible and ancient ones). Check that the per-pixeltype support makes sense (as output in the TIFFWriter.CompressionTypes test).

To confirm manually that compression is working, apply this patch on top of this branch:

diff --git a/test/ome-files/tiff.cpp b/test/ome-files/tiff.cpp
index ef8073a..8aa9fbc 100644
--- a/test/ome-files/tiff.cpp
+++ b/test/ome-files/tiff.cpp
@@ -1448,9 +1448,9 @@ class PixelTest : public ::testing::TestWithParam<PixelTestParameters>
   TearDown()
   {
     // Delete file (if any)
-    const PixelTestParameters& params = GetParam();
-    if (boost::filesystem::exists(params.filename))
-      boost::filesystem::remove(params.filename);
+    // const PixelTestParameters& params = GetParam();
+    // if (boost::filesystem::exists(params.filename))
+    //   boost::filesystem::remove(params.filename);
   }
 };

If using the superbuild, change into the ome-files-build directory, re-run the build with make or ninja, and then re-run the TIFF tests with ctest -R tiff. This will not delete the generated TIFFs, so you can look at them to check the compression. Example:

tiffinfo test/ome-files/data/data-layout-uint8-43x37planar-pi2-compDeflate-tile-32x32-ordered-optimal.tiff

In this case, the file uses Deflate compression and tiffinfo will confirm that. You can also try viewing the file with Bio-Formats showinf to ensure the pixel data is correct. Likewise for LZW compression. Otherwise there will be no compression enabled (None or NoneDefault in the filename). Look in test/ome-files/data to see what got generated (it's a random selection).

TODO: either as a followup or here, add example of enabling compression to our existing pixeldata sample.

- Allow introspection of TIFF codec support on a per-pixeltype basis;
  add getCodecNames and getCodecScheme to the tiff Codec support
  functions
- Allow the writer interface to query valid codecs for a given pixel
  type by adding getCompressionTypes(PixelType) which is a rather
  more useful variant than the existing getPixelTypes(codec) (which
  might be deprecated and removed).  In the common case, you might
  want to select a compression type for a given set of pixel data,
  but altering the pixel type to support a certain codec is less
  common
- Add the supported codecs to the TIFF writers, so that they can be
  queried and set with the FormatWriter interface
- Add unit tests to test with LZW and Deflate compression
@joshmoore
Copy link
Member

NB: since this is fairly often suggested for testing:

To confirm manually that compression is working, apply this patch on top of this branch:

can I suggest as an RFE that a flag or environment variable get added to disable the deleting?

@rleigh-codelibre
Copy link
Contributor Author

We can certainly do that.

@simleo
Copy link
Member

simleo commented Dec 19, 2016

I've built locally using the superbuild. The tests mentioned above look fine, although there seems to be an inconsistency re: JPEG support for int8

In FormatWriterTest.CompressionTypes:

3: Pixel Type: int8
3:   default
3:   jpeg
3:   lzw
3: Pixel Type: uint8
3:   default
3:   jpeg
3:   lzw

In TIFFWriter.CompressionTypes:

12: Pixel Type: int8
12:   AdobeDeflate
12:   Deflate
12:   LZW
12:   default
12: Pixel Type: uint8
12:   AdobeDeflate
12:   Deflate
12:   JPEG
12:   LZW
12:   default

Is this working as intended?

@simleo
Copy link
Member

simleo commented Dec 19, 2016

I've looked at a few generated files as instructed above and the compression scheme reported by tiffinfo was as expected.

@rleigh-codelibre
Copy link
Contributor Author

@simleo Yes, it's working as intended. In the FormatWriter test:

if (i->first == PixelType::INT8 || i->first == PixelType::UINT8)
  codecset.insert("jpeg");

this is completely contrived data for the unit test; it's not reflective of the actual jpeg restriction shown in the testing of the TIFF code, where it's restricted to unsigned 8-bit only.

I can certainly remove INT8 from the FormatWriter test to avoid any confusion though.

This codec is just an example, not actually JPEG, so rename
to avoid any potential confusion regarding the supported
pixel types.
@rleigh-codelibre
Copy link
Contributor Author

@simleo Updated to rename the test (since it's unrelated to JPEG, I just renamed it to not be named jpeg).

@sbesson
Copy link
Member

sbesson commented Jan 5, 2017

Having briefly looked at the diff and builds, the feature addition looks sensible and certainly something the community would greatly benefit in 0.3.0. A couple of specific comments/questions:

  • the addition of the reciprocal method to query to the pixeltype/compression mapping certainly makes sense to me and corresponds to a clear workflow when saving (OME-)TIFF images with a given pixel type
  • generally, are some of the API additions in this PR also candidates for porting to the Java API during the next development cycle of the Bio-Formats API? In particular, IFormatWriter.getCompressionTypes() and IFD.setCompression()? /cc @dgault @melissalinkert

@rleigh-codelibre
Copy link
Contributor Author

rleigh-codelibre commented Jan 5, 2017

@sbesson Regarding Java, depends upon whether we want to do this work in ome-files-java or bioformats?

IFormatWriter.getCompressionTypes() could be added pretty easily.

IFD.setCompression is part of the libtiff wrapper code, and may or may not be useful. The Java IFD class is setting the COMPRESSION field. The C++ wrapper is just doing the same via a convenience method. I think we might also be able to remove it at some point; it's primarily there, along with the other getters and setters, to allow query of set fields since they aren't readable via the API until written to disc, so it's caching the set values for the getters. We might be able to use TIFFCheckpointDirectory to eliminate the caching entirely, along with the getters and setters. This isn't user-visible API, so isn't quite as useful as the FormatWriter enhancement.

Another point to consider for the future: libtiff allows getting and setting of uncompressed pixel data, or compressed pixel data ("encoded" and "raw" variants). We only support uncompressed data in our openBytes/saveBytes API. We might want to consider the addition of a raw variant, so that we can transfer lossy compressed pixel data without introducing recompression artefacts with e.g. bfconvert. Coupled with a proper tile API, it would allow direct transfer of tiles from reader to writer without any decompression or recompression (when the source and destination tile sizes and compression algorithm match).

@dgault
Copy link
Member

dgault commented Jan 5, 2017

I can certainly see the addition of getCompressionTypes(PixelType) being very useful.

Having gone through the code it looks sensible to me and the supported types all appear correct.

The unit tests look fine and run as expected. The manual testing as mentioned also looks to be correct when using both tiffinfo and showinf

@sbesson
Copy link
Member

sbesson commented Jan 12, 2017

Recorded the IFormatWriter.getCompressionTypes API addition as a candidate for the next round of Java breaking changes. The TIFF compression support is definitely a valuable addition sense and might be something we want to consume internally in the near future for benchmarking and/or review of our compression metrics. Otherwise, all tests have been sucessfully green for the past few weeks and this PR has been signed off by multiple people. Merging.

@sbesson sbesson merged commit c6cfbf3 into ome:master Jan 12, 2017
@rleigh-codelibre rleigh-codelibre deleted the tiff-compression branch January 12, 2017 13:40
@sbesson sbesson modified the milestone: 0.3.0 Feb 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants