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

Pyramid TIFF: support multiple planes and OME-XML comments #2747

Closed

Conversation

melissalinkert
Copy link
Member

Backports recent changes to PyramidTiffReader to allow multiple planes in each file and an OME-XML comment to determine Z/C/T dimensions and supply additional metadata.

To test, use the new data_repo/curated/pyramid-tiff/samples/multi-channel-4D-series.tiff file (see also https://github.com/openmicroscopy/data_repo_config/pull/173). Verify that showinf detects the reader as PyramidTiffReader and that the Z, C, and T sizes and ordering matches the text in each plane.

@@ -52,6 +66,8 @@

// -- Fields --

private IMetadata omexml = null;;
Copy link
Member

Choose a reason for hiding this comment

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

double semicolon

@dgault
Copy link
Member

dgault commented Feb 7, 2017

Without this PR it read using the OMETiffReader with the following planes:
Plane #0 <=> Z 0, C 0, T 0
Plane #50 <=> Z 0, C 1, T 3
Plane #51 <=> Z 1, C 1, T 3
Plane #52 <=> Z 2, C 1, T 3
Plane #53 <=> Z 3, C 1, T 3
Plane #54 <=> Z 4, C 1, T 3
Plane #104 <=> Z 4, C 2, T 6

With the PR applied it correctly read with the PyramidTiffReader and the following planes:
Plane #0 <=> Z 0, C 0, T 0
Plane #50 <=> Z 0, C 1, T 3
Plane #51 <=> Z 1, C 1, T 3
Plane #52 <=> Z 2, C 1, T 3
Plane #53 <=> Z 3, C 1, T 3
Plane #54 <=> Z 4, C 1, T 3
Plane #104 <=> Z 4, C 2, T 6

Dimension order = XYZCT

The dimensions haven't changed but the sizes and ordering are correct according to the text on the planes when the image is viewed.

The associated config PR is also showing the correct values and the build is green: https://ci.openmicroscopy.org/view/Bio-Formats/job/BIOFORMATS-DEV-merge-repository-subset/385/

@dgault
Copy link
Member

dgault commented Feb 8, 2017

With that last commit this PR should be good to merge

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.

My main concern with the current proposal is the clear overlap with the OME-TIFF specification as reflected by the readers.txt re-ordering.

As a proof of concept, taking the sample file in the description and renaming it as multi-channel-4D-series.ome.tiff. The extension and the OME-XML in the ImageDescription tag are necessary and sufficient conditions to identify it
as an OME-TIFF as per the specification. The implicit format specification of this reader would also identifies this file as an extended Pyramid TIFF. The format detection is de facto ambiguous and becomes the responsibility of the implementation.

Bio-Format resolves this conflict via the readers order and will detect this file as a Pyramid TIFF:

[sbesson@ome-c6100-1 ~]$ cp /ome/data_repo/curated/pyramid-tiff/samples/multi-channel-4D-series.tiff multi-channel-4D-series.ome.tiff 
[sbesson@ome-c6100-1 ~]$ /opt/hudson/workspace/BIOFORMATS-DEV-merge-build/tools/showinf -nopix multi-channel-4D-series.ome.tiff 
Checking file format [Pyramid TIFF]
Initializing reader
PyramidTiffReader initializing multi-channel-4D-series.ome.tiff
...

On the other side, the reference C++ implementation would read it as an OME-TIFF:

# ome-files info multi-channel-4D-series.ome.tiff 
Image: multi-channel-4D-series.ome.tiff
Using reader: OME-TIFF (Open Microscopy Environment TIFF)
Reader setup took 00:00:00.229998

Filename = "/root/multi-channel-4D-series.ome.tiff"
Used files = ["/root/multi-channel-4D-series.ome.tiff"]

Reading core metadata
Series count = 1

Series #0:
	Image count = 105
	RGB = [false, false, false] ([1, 1, 1]) 
	Interleaved = true
	Indexed = false
	Width = 439
	Height = 167
	SizeZ = 5 (effectively 5)
	SizeT = 7 (effectively 7)
	SizeC = 3 (effectively 3)
	Thumbnail size = 128 × 48
	Endianness = little
	DimensionOrder = XYZCT (certain)
	PixelType = int8
	Bits per Pixel = 8
	MetadataComplete = true
	ThumbnailSeries = false

No global metadata

An additional technical complexity is related to the Bio-Formats approach for OME-TIFF detection. Since 0b29025, any file with a TIFF extension can be detected as an OME-TIFF. This lenient detection mechanism forces the readers to be inverted in order to get the logic working. Technically, setting suffixNecessary=true in the OMETiffReader might partly solve the problem above but would be a huge breaking change for our community.

I definitely appreciate the ongoing efforts to extend a extending TIFF format with subresolution support. As expressed above, my main concerns is the potential precarious position this proposal puts us in with regard to our specification as it effectively opens the door to custom OME-TIFF flavors.

I would we turn this into a design discussion on the existing specifications, their limitations and the possibilities and make a collegial decision with all the parties involved for the next steps. From my perspective, there are 2 main questions:

  • do we want to initially ship this extension as a separate file format from our own TIFF-based format?
  • do we want to built on top of this proposal and work towards an official extension of the OME-TIFF specification providing some implicit resolution support?

@rleigh-codelibre
Copy link
Contributor

Regarding @sbesson's comments:

  • For the C++ code, there is no ImageReader; ome-files info uses the OMETIFFReader exclusively, so I wouldn't be too concerned about the C++ detection here--until we implement ImageReader, there is no detection of the format at all.

Regarding OME-TIFF, supporting subresolutions there would be great, but I think that work should be done in the model (if needed) and the OME-TIFF reader and writer, treating this as a separate format unrelated to OME-TIFF. As mentioned in the last meeting, there are several ways to do subresolutions, and for OME-TIFF using existing approaches like the SUBIFD tag; using this would allow readers to continue to read OME-TIFFs without any subresolution support since it's both optional and backward compatible. The Pyramid TIFF reader relies on specific ordering of IFDs and as such is not a drop-in solution.

That said, I don't think we would object to third-party formats embedding OME-XML--it was after all one of the goals of the data model, and so I don't think our reuse of OME-XML in a custom format is bad per se. But I do think we need to be explicit about this being a totally separate file format which is completely unrelated to and incompatible with the OME-TIFF specification. And I think at a minimum we should have a clear documented specification for the format, including exactly how the IFDs are laid out. How exactly are multiple series and multiple dimensions and the subresolutions of each laid out? Which IFDs are referred to by the TiffData blocks in the embedded OME-XML?

@melissalinkert
Copy link
Member Author

New design issue started here for discussion: ome/design#74
That outlines the plane ordering assumptions made here, and proposes some possible next steps both for making pyramids a formal part of OME-TIFF and making these hybrid pyramid TIFFs more obviously a distinct format. If you all would prefer to discuss here, I can can close the issue and paste the text here - whatever is easier.

@sbesson
Copy link
Member

sbesson commented Mar 9, 2017

As this PR still requires some design and will not make it into the upcoming Bio-Formats 5.4.0, closing in favor of the design issue where we can pursue the discussion.

@sbesson sbesson closed this Mar 9, 2017
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.

None yet

5 participants