-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for reading compressed NDPI tiles #4181
Conversation
This provides better precompressed tile support for input formats that don't have a constant tile size across all resolutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, very supportive of the migration of the SVS functionality for accessing the raw tiles to MinimalTiffReader
. Beyond the scope of this change, it opens the door to supporting the reading of compressed tiles in other TIFF-based whole slide image formats. Also this starts exposing an initial API allowing direct raw tile access in TIFF formats from Bio-Formats.
Using the reference CMU-1.svs and CMU-1.npdi from https://openslide.cs.cmu.edu/download/openslide-testdata/, I was able to successfully run bfconvert -noflat -precompressed -compression JPEG
into a DICOM file which has a sensible size.
However, the changes seem to have affected conversion with other options (no -precompressed
flag or JPEG-2000 compression) as the utility now fails with an java.lang.ArrayIndexOutOfBoundsException
+ mkdir from_svs_precompressed_jpeg
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -precompressed -compression JPEG CMU-1.svs from_svs_precompressed_jpeg/CMU-1.dcm
+ mkdir from_svs_precompressed_j2k
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -precompressed -compression JPEG-2000 CMU-1.svs from_svs_precompressed_j2k/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
+ mkdir from_svs_jpeg
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -compression JPEG CMU-1.svs from_svs_jpeg/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
+ mkdir from_svs_j2k
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -compression JPEG-2000 CMU-1.svs from_svs_j2k/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
+ mkdir from_ndpi_precompressed_jpeg
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -precompressed -compression JPEG CMU-1.ndpi from_ndpi_precompressed_jpeg/CMU-1.dcm
+ mkdir from_ndpi_precompressed_j2k
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -precompressed -compression JPEG-2000 CMU-1.ndpi from_ndpi_precompressed_j2k/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
+ mkdir from_ndpi_jpeg
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -compression JPEG CMU-1.ndpi from_ndpi_jpeg/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
+ mkdir from_ndpi_j2k
+ /Users/sbesson/Documents/GitHub/bioformats/tools/bfconvert -noflat -compression JPEG-2000 CMU-1.ndpi from_ndpi_j2k/CMU-1.dcm
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1
at loci.formats.out.DicomWriter.saveBytes(DicomWriter.java:587)
at loci.formats.ImageWriter.saveBytes(ImageWriter.java:260)
at loci.formats.tools.ImageConverter.convertTilePlane(ImageConverter.java:1076)
at loci.formats.tools.ImageConverter.convertPlane(ImageConverter.java:908)
at loci.formats.tools.ImageConverter.testConvert(ImageConverter.java:841)
at loci.formats.tools.ImageConverter.main(ImageConverter.java:1318)
I retested with Bio-Formats 7.3.0 and confirm that bfconvert -noflat -compression JPEG CMU-1.svs from_svs_jpeg/CMU-1.dcm
was running to completion so I suspect this is a regression introduced by this PR.
I believe the exceptions in #4181 (review) are all fixed by 23780de. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest fixes, the conversion utility completed in all my tests. Tables below give the execution times measured in seconds with a standard deviation computed from 3 runs.
Using Bio-Formats 7.3.0 and CMU-1.svs
Compression / Precompressed | yes | No |
---|---|---|
JPEG | 78.6 +/- 0.6 | 228.4 +/- 2.2 |
JPEG-2000 | 682.8 +/- 5.6 | 674.0 +/- 11.0 |
Using this PR and CMU-1.svs
Compression / Precompressed | yes | No |
---|---|---|
JPEG | 80.6 +/- 1.1 | 231.9 +/- 2.3 |
JPEG-2000 | 686.4 +/- 4.2 | 684.8 +/- 3.6 |
Using this PR and CMU-1.ndpi
Compression / Precompressed | yes | No |
---|---|---|
JPEG | 15.7 +/- 0.4 | 116.9 +/- 7.2 |
JPEG-2000 | 648.7 +/- 4.5 | 645.9 +/- 2.6 |
Measurements are consistent with the previous values and as per above, there is a substantial gain of performance (even better than SVS) when using the precompressed flag and the native tile compression (JPEG).
Loading the generated DICOM files into OMERO for visual assessment and comparison to the original, all datasets worked as expected with the exception of the DICOM dataset created from the SVS file using -precompressed
and -compression JPEG-2000
as the import failed with
java.lang.RuntimeException: Failure response on import!
Category: ::omero::grid::ImportRequest
Name: import-request-failure
Parameters: {stacktrace=java.lang.RuntimeException: File is neither valid JP2 file nor valid JPEG 2000 codestream
at com.sun.media.imageioimpl.plugins.jpeg2000.J2KReadState.initializeRead(J2KReadState.java:729)
at com.sun.media.imageioimpl.plugins.jpeg2000.J2KReadState.<init>(J2KReadState.java:241)
at com.sun.media.imageioimpl.plugins.jpeg2000.J2KImageReader.readRaster(J2KImageReader.java:551)
at ome.codecs.services.JAIIIOServiceImpl.readRaster(JAIIIOServiceImpl.java:177)
at ome.codecs.JPEG2000Codec.decompress(JPEG2000Codec.java:296)
at loci.formats.codec.WrappedCodec.decompress(WrappedCodec.java:86)
at loci.formats.codec.JPEG2000Codec.decompress(JPEG2000Codec.java:63)
at loci.formats.in.DicomReader.getTile(DicomReader.java:1622)
at loci.formats.in.DicomReader.openBytes(DicomReader.java:304)
at loci.formats.ImageReader.openBytes(ImageReader.java:467)
at loci.formats.ChannelFiller.openBytes(ChannelFiller.java:169)
at loci.formats.ChannelFiller.openBytes(ChannelFiller.java:161)
at loci.formats.ChannelSeparator.openBytes(ChannelSeparator.java:202)
at loci.formats.ReaderWrapper.openBytes(ReaderWrapper.java:350)
at loci.formats.ReaderWrapper.openBytes(ReaderWrapper.java:350)
at loci.formats.MinMaxCalculator.openBytes(MinMaxCalculator.java:269)
…used This makes sure that the tiles actually saved match the tile sizes supplied to the writer.
In this case the pyramid is converted tile-wise, but the extra images are not, which was a problem since the reader's optimal tile sizes are now sent to the writer any time |
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/file-format-to-store-images-using-ngff-coverter/98320/4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual code changes look good from my side and moving the logic to MinimalTiffReader makes sense.
I tested some conversions from images at https://downloads.openmicroscopy.org/images/Hamamatsu-NDPI/openslide/ to DICOM, with and without the new option. Similar to Seb's finding, the conversions were all successful and images opened and displayed as expected. I also saw similar speed up times as mentioned above when converting with the precompressed option.
This expands on the "precompressed" features added in #3992.
The main goal was to just add compressed tile reading to the NDPI reader, so that conversion from NDPI to DICOM wouldn't require much if any recompression. This required a variety of changes not limited to NDPI though:
JPEGTurboService
so that the repackaged compressed tiles can be retrieved, as well as the tile size and countsSVSReader
, and intoMinimalTiffReader
(to prevent duplicate logic inNDPIReader
)NDPIReader
to report optimal sizes based on actual IFD/JPEG stream, instead of constant 1024x1024DicomWriter
to allow different tile sizes in each resolution, since NDPI reports different sizes per resolutionNDPIReader
to use thesaveCompressedBytes
APIThe end result is that something like this should now work:
bfconvert -noflat -precompressed -compression JPEG CMU-1.ndpi cmu.dcm
and should complete within a few seconds as all tiles can be copied without any recompression.
I would not expect SVS -> DICOM support to change, but it probably makes sense to double-check just in case the refactoring caused any problems.
I don't expect this to fail tests, but excluding for now so this doesn't distract from 7.3.0. Given the API changes in
JPEGTurboService
, this probably does require a major version.cc @dclunie, @fedorov