Use multiple-of-16 tile dimensions when writing TIFFs

[jmuhlich]

This solves the problem in #3708, but it may also be helpful to add a check and error message to the command-line tools and top-level API entrypoints when requested tile sizes are not a multiple of 16, to prevent unexpected results.

Fixes #3708

[dgault]

Thanks @jmuhlich for raising this Issue and opening the PR. Normally this should be checked by the top level API based on the particular granularity of a given format (for example TiffWriter has a granularity of 16 https://github.com/ome/bioformats/blob/develop/components/formats-bsd/src/loci/formats/out/TiffWriter.java#L557), however the ImageConverter is bypassing this and setting the IFD values directly which leads this to be a possible issue. My initial thought is that the ImageConverter should probably be modified to go through the Writer API if possible. I will try to test that approach on my side and may push a follow up commit if it looks good.

On a side note we have recently introduced a Contributor License Agreement. If possible would you be able to sign and return the form following the instructions on https://ome-contributing.readthedocs.io/en/latest/cla.html

[dgault]

Hi @jmuhlich, I have pushed a commit which changes the handling of tiling in the ImageConverter to use the FormatWriter API. This will mean that the tile size requirements will be handled by each underlying writer.

With this change converting to a tiff file with a tile size of 15 using:
bfconvert -tilex 15 -tiley 15 path/to/input.tiff path/to/output.tiff
will round up to 16 and can be verified using tiffinfo:

TIFF Directory at offset 0x8 (8)
  Image Width: 512 Image Length: 512
  Tile Width: 16 Tile Length: 16

With this change converting to a tiff file with a tile size of 25 using:
bfconvert -tilex 25 -tiley 25 path/to/input.tiff path/to/output.tiff
will round up to 32 and can be verified using tiffinfo:

TIFF Directory at offset 0x8 (8)
  Image Width: 512 Image Length: 512
  Tile Width: 32 Tile Length: 32

[jmuhlich]

The output image is garbled if the tile size gets rounded by this new code, and with certain values an exception is even thrown (see example below with bio-formats-logo.png and tilex=tiley=34). I think the problem is that the source image is still being read in tiles of the originally requested size, so after TiffWriter rounds off the size the resulting buffers have wrong data or the computed indexes and coordinates are nonsensical.

$ ./tools/bfconvert -tilex 34 -tiley 34 components/formats-gpl/src/loci/formats/bio-formats-logo.png test-tile-rounding.tif
components/formats-gpl/src/loci/formats/bio-formats-logo.png
APNGReader initializing components/formats-gpl/src/loci/formats/bio-formats-logo.png
[Animated PNG] -> test-tile-rounding.tif [Tagged Image File Format]
Tile size = 34 x 34
Exception in thread "main" java.lang.NegativeArraySizeException: -768
	at loci.formats.out.TiffWriter.getTile(TiffWriter.java:595)
	at loci.formats.out.TiffWriter.saveBytes(TiffWriter.java:252)
	at loci.formats.out.TiffWriter.saveBytes(TiffWriter.java:475)
	at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:920)
	at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:789)
	at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:717)
	at loci.formats.tools.ImageConverter.main(ImageConverter.java:1093)

[jmuhlich]

Here is the result of specifying tilex=tiley=33 for bio-formats-logo.png (converted back to PNG since apparently TIFFs aren't supported here):

[dgault]

Thanks for testing @jmuhlich, it seems the values used within the ImageConverter need to be reset to the updated rounded values. The latest commit should resolve the issue.

[sbesson]

Tested the last commit with

./bfconvert -tilex 34 -tiley 34 test.fake test.tif

The generated TIFF now has the expected metadata esp. in terms of tile size

t(base) sbesson@ls30630:~ $ tiffinfo test.tif 
TIFF Directory at offset 0x8 (8)
  Image Width: 512 Image Length: 512
  Tile Width: 32 Tile Length: 32
  Resolution: 0, 0 pixels/cm
  Bits/Sample: 8
  Sample Format: unsigned integer
  Compression Scheme: None
  Photometric Interpretation: min-is-black
  Samples/Pixel: 1
  Planar Configuration: single image plane
  ImageDescription: ImageJ=
hyperstack=true
images=1
channels=1
slices=1
frames=1
  Software: OME Bio-Formats 6.8.0-SNAPSHOT

However, the binary data is corrupted for the last column:

In terms of logging,

bioformats/components/bio-formats-tools/src/loci/formats/tools/ImageConverter.java

Line 827 in f1b9643

LOGGER.info("Tile size = {} x {}", w, h);

should also be moved after the API call to display the selected tile size rather than the user-supplied tile size. Optionally, a logging statement could also be emitted to indicate that the writer selected a different tile size.

Finally, I would suggest to increase the coverage of https://github.com/ome/bioformats/blob/develop/components/bio-formats-tools/test/loci/formats/tools/ImageConverterTest.java to cover this scenario and check that the tile size is correctly adjusted.

[dgault]

I don't seem to be able to reproduce the issue you are seeing with the last column, do you see that with any other files or was it only that 1 fake?

I have moved the API calls up above any remaining tile logging or calculations and added some additional test.

[sbesson]

I can no longer reproduce the conversion artifact. Assuming someone went wrong, quite possibly in my local environment, and is now fixed

A conversion with a non-symmetrical tile with sizes which are not multiples of 16 is now adjusted to the closest multiple of 16 by the converter as expected

(base) sbesson@ls30630:bioformats ((79e3e287f3...)) $ ./tools/bfconvert -tilex 34 -tiley 45 test.fake test.tiff
test.fake
FakeReader initializing test.fake
[Simulated data] -> test.tiff [Tagged Image File Format]
Tile size = 32 x 48
	Converted 1/1 planes (100%)
[done]
1.224s elapsed (1.0+479.0ms per plane, 731ms overhead)

The generated image has the correct metadata and renders as expected:

(base) sbesson@ls30630:bioformats ((79e3e287f3...)) $ tiffinfo test.tiff 
TIFF Directory at offset 0x8 (8)
  Image Width: 512 Image Length: 512
  Tile Width: 32 Tile Length: 48
  Resolution: 0, 0 pixels/cm
  Bits/Sample: 8
  Sample Format: unsigned integer
  Compression Scheme: None
  Photometric Interpretation: min-is-black
  Samples/Pixel: 1
  Planar Configuration: single image plane
  ImageDescription: ImageJ=
hyperstack=true
images=1
channels=1
slices=1
frames=1
  Software: OME Bio-Formats 6.7.0-SNAPSHOT

The new ImageConverter unit test also covers the tile size adjustements and should catch regressions in this codepath.

Ready to merge and include in the upcoming Bio-Formats 6.8.0 release. @jmuhlich are you happy with the state of the last commits which should address your initial issue?