DICOM writer: throw an exception if the provided tiles don't match the expected tile size

[melissalinkert]

Without this PR, viewing the smallest resolution from the output of:

bfconvert HandEcompressed_Scan1.qptiff test.dcm -tilex 2048 -tiley 2048 -noflat

shows an almost all black image; only the top 16 rows of pixels are present. Viewing in OMERO, QuPath, or showinf -noflat -resolution 4 test_0_0.dcm will show the problem. HandEcompressed_Scan1.qptiff is from https://downloads.openmicroscopy.org/images/Vectra-QPTIFF/perkinelmer/PKI_scans/.

The source of the problem is a combination of things:

• the chosen 2048x2048 tile size is smaller than all resolutions except resolution 4 (the last one)
• for tile sizes larger than a particular resolution, bfconvert reverts to the reader's optimal tile size
• the reader's optimal size is 1920x16 for resolution 4 (all others have an optimal size of 512x512)
• DicomWriter expects full tiles matching the chosen tile size to be supplied to saveBytes, and will pad any smaller tiles to the correct dimensions. So in this case, the first 1920x16 tile supplied gets padded to 2048x2048, and the image is considered completely written.

The proposed solution here is to throw an exception in saveBytes if a tile is provided that is not on an expected tile boundary (x and y coordinates not a multiple of tile size) or is the wrong width or height without being bottom or right edge tile. I think that's most compatible with #3992, minimizes impact by not changing bfconvert, and catches cases where the provided tile might not be the correct size when bfconvert is not being used.

With this PR, the conversion command above should throw FormatException. Changing the tile size parameters to -tilex 1024 -tiley 1024 or similar should work, and each resolution should be readable.

Updating bfconvert to use the full image instead of the reader's optimal size in cases such as resolution 4 would also potentially work, but I'm not convinced that's appropriate in general.

[sbesson]

Confirmed that with the proposed change, the command that was previously writing incorrect planes for the smallest resolution now fails with an exception indicating the reduce the tile size.

Within the context of this writer, the new error message is definitely a improvements as it allows the user to select a different tile size. Only downside is that this call happens in saveBytes so all the previous resolutions have been successfully generated and the suggested tile size might still be not suitable for the next resolution levels if any.

For comparison, I tried to perform the same tiled writing using OME-TIFF as the format output and the conversion fails with a different error message on the last resolution.

(java11) sbesson@Sebastiens-MacBook-Pro-3 bioformats % ./tools/bfconvert ~/Downloads/HandEuncompressed_Scan1.qptiff test.ome.tiff -bigtiff -tilex 2048 -tiley 2048 -noflat -compression zlib
Output file test.ome.tiff exists.
Do you want to overwrite it? ([y]/n)
y
/Users/sbesson/Downloads/HandEuncompressed_Scan1.qptiff
VectraReader initializing /Users/sbesson/Downloads/HandEuncompressed_Scan1.qptiff
Reading IFDs
Populating metadata
Populating OME metadata
[PerkinElmer Vectra/QPTIFF] -> test.ome.tiff [OME-TIFF]
Tile size = 2048 x 2048
	Series 0: converted 1/1 planes (100%)
Tile size = 2048 x 2048
	Series 0: converted 1/1 planes (100%)
Tile size = 2048 x 2048
	Series 0: converted 1/1 planes (100%)
Tile size = 2048 x 2048
	Series 0: converted 1/1 planes (100%)
Tile size = 1920 x 16
Exception in thread "main" java.lang.IndexOutOfBoundsException: Range [0, 0 + 30720) out of bounds for length 5760
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromIndexSize(Preconditions.java:82)
	at java.base/jdk.internal.util.Preconditions.checkFromIndexSize(Preconditions.java:343)
	at java.base/java.util.Objects.checkFromIndexSize(Objects.java:424)
	at java.base/java.io.ByteArrayOutputStream.write(ByteArrayOutputStream.java:155)
	at java.base/java.io.DataOutputStream.write(DataOutputStream.java:107)
	at loci.formats.tiff.TiffSaver.writeImage(TiffSaver.java:354)
	at loci.formats.tiff.TiffSaver.writeImage(TiffSaver.java:278)
	at loci.formats.out.TiffWriter.saveBytes(TiffWriter.java:282)
	at loci.formats.out.OMETiffWriter.saveBytes(OMETiffWriter.java:219)
	at loci.formats.out.PyramidOMETiffWriter.saveBytes(PyramidOMETiffWriter.java:89)
	at loci.formats.out.OMETiffWriter.saveBytes(OMETiffWriter.java:209)
	at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:966)
	at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:833)
	at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:766)
	at loci.formats.tools.ImageConverter.main(ImageConverter.java:1199)

The error above makes me wonder whether we have a more fundamental issue with the behavior of ImageConverter when converting resolutions smaller than the supplied tile size.

[melissalinkert]

The error above makes me wonder whether we have a more fundamental issue with the behavior of ImageConverter when converting resolutions smaller than the supplied tile size.

Is that something you would like to see addressed in this PR, or would a follow-up issue/PR be reasonable? #3643 and #3630 may be related as well.

[sbesson]

Is that something you would like to see addressed in this PR, or would a follow-up issue/PR be reasonable?

I avoided requesting changes in my review as I am not sure of the best approach and the amount of work required to deal with this tile size adjustment issue.

As mentioned in my previous review, throwing an error message definitely feels like a step-wise improvement compared to silently writing corrupted data. In the context of a patch release, I think it's fair to limit the scope of the changes to this write and schedule a separate discussion regarding changes to the converter logic.
@dgault any thoughts?

[dgault]

Agreed that this has highlighted a more fundamental issue within the ImageConverter (or conversions in general). This PR seems like a sensible first step for DICOM writing though and from the perspective of including it in a patch release then this seems a definite improvement on the existing behaviour. I would be happy including this in the upcoming release and handling the wider use case as a follow up for a future release.