Skip to content
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

Merged
merged 8 commits into from
Jul 22, 2024

Conversation

melissalinkert
Copy link
Member

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:

  • update JPEGTurboService so that the repackaged compressed tiles can be retrieved, as well as the tile size and counts
  • move general TIFF tile copying logic out of SVSReader, and into MinimalTiffReader (to prevent duplicate logic in NDPIReader)
    • this should also make it easier to support precompressed tiles in other TIFF-based formats later on
  • update NDPIReader to report optimal sizes based on actual IFD/JPEG stream, instead of constant 1024x1024
  • update DicomWriter to allow different tile sizes in each resolution, since NDPI reports different sizes per resolution
  • update NDPIReader to use the saveCompressedBytes API

The 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

Copy link
Member

@sbesson sbesson left a 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.

@melissalinkert
Copy link
Member Author

I believe the exceptions in #4181 (review) are all fixed by 23780de.

Copy link
Member

@sbesson sbesson left a 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.
@melissalinkert
Copy link
Member Author

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

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 -precompressed is used. I think 4f2a96e is one way to fix, but open to other thoughts.

@imagesc-bot
Copy link

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

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retested with the last commit. The conversion times are identical to the ones reported previously and all images including the label images are now viewable
Screenshot 2024-07-02 at 08 58 26
Screenshot 2024-07-02 at 08 58 34

Copy link
Member

@dgault dgault left a 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.

@dgault dgault merged commit 678fd01 into ome:develop Jul 22, 2024
18 checks passed
@melissalinkert melissalinkert deleted the ndpi-precompressed branch September 6, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants