Add TIFF Key-Value Store Implementation (take 2) #230

hsidky · 2025-04-17T02:21:47Z

This PR adds a new (read only) key-value store driver for accessing tiles and strips within TIFF image files. This is my second attempt after finding the time to work on this given previous feedback on #138. I drew heavily upon the zip kvstore implementation and your prior feedback (thanks!), so I hope that things are more faithful to Tensorstore patterns.

The basic idea is this kvstore sits on top of a base kvstore and maps IFDs/tiles/stripes to the following key format: tile/<id>/<row>/<col>.

My plan is to get feedback on whether this something that is suitable to be merged. If I receive a positive response, I work on any feedback I receive in parallel with building out the tiff driver that will pair with this kvstore.

Looking forward to your feedback on this implementation!

P.S. I couldn't for the life of me get subsystem logging to work (aka ABSL_LOG_IF). Would appreciate some advice on how to get that working.

…f IFD.

jbms · 2025-04-18T18:54:33Z

This is great --- I haven't had a chance to review your implementation in detail, but at a high level this is exactly the right implementation strategy.

Do you have a reason to use this tiff kvstore driver on its own? Otherwise I'd be inclined to eliminate the JSON registration for it, such that it can only be created internally (by the to-be-written Tiff TensorStore driver).

jbms · 2025-04-18T18:58:27Z

Regarding logging: what have you tried? Are you testing with bazel? With bazel you can do --test_env=TENSORSTORE_VERBOSE_LOGGING=tiff

hsidky · 2025-04-19T12:51:12Z

This is great --- I haven't had a chance to review your implementation in detail, but at a high level this is exactly the right implementation strategy.

Do you have a reason to use this tiff kvstore driver on its own? Otherwise I'd be inclined to eliminate the JSON registration for it, such that it can only be created internally (by the to-be-written Tiff TensorStore driver).

Thanks for that feedback. No, I don't think there is any reason to use the tiff kvstore on its own. I can eliminate the JSON registration.

Regarding logging: what have you tried? Are you testing with bazel? With bazel you can do --test_env=TENSORSTORE_VERBOSE_LOGGING=tiff

Yes, I'm using bazel though I'm not terribly familiar with it. That works! I had previously tried setting the environment variable as that worked for non-subsystem logging in tests, but it wasn't working for conditional logging. Thank you.

I'm going to start chipping away at the tiff driver as I await detailed feedback on this kvstore.

… Working on crashes.

hsidky · 2025-05-05T23:18:38Z

Hello again. I've managed to develop a driver per your suggestions and I believe I have something that's a pretty solid foundation with a reasonable amount of support. There are some outstanding items + polish but I think it makes sense for me to pause here for feedback in case major changes are required and to understand the likelihood of being able to get this merged first.

A few notes on the driver implementation:

I changed the tiff kvstore to work off of a linear index rather than an xy grid. It made it easier to handle samplesperpixel>1 but did offload more work to getchunkstoragekey, so I did a bit of optimization there.
I haven't worked on transaction support as I haven't fully wrapped my head around the nuts and bolts of how that works yet. Is this a requirement for a merge? I saw, for example, that the single image driver does not support transactions, so I assume not.
Same as above but for storage stats.
I did not commit the test file binaries (generated by the generate.py file). I'll wait until you give me clearance.
I waffled a bit on how much to build out the metadata property. I ended up putting options under a separate tiff property as they mainly correspond to the interpretation of the data. I can revisit if you have strong opinions.

I think that's it for now. Looking forward to any feedback!

jbms · 2025-05-06T16:34:34Z

tensorstore/driver/tiff/schema.yml

+                Optional. Must match the length of `dimensions`. Required if `dimensions` 
+                has more than one entry. If only one dimension is specified, `ifd_count` can 
+                be used instead. If both are specified, their product must match `ifd_count`.
+            ifd_count:


Why does this need to be specified --- can't we determine the number of IFDs from the file?

I suppose it doesn't. I initially was thinking about wanting to use only a subset of IFDs, but then really ifd_count would be one of many parameters that a user would need to specify in order to provide them with the kind of flexibility to do that properly. I will remove.

Actually, I double checked and I see now that IFDs may be ignored in OME XML so I will I need to add full specification functionality rather than remove this. https://docs.openmicroscopy.org/ome-model/5.6.3/ome-tiff/specification.html

jbms · 2025-05-06T16:37:10Z

tensorstore/driver/tiff/schema.yml

+              description: |
+                Optional. Specifies the total number of IFDs involved in the stack.
+                Required if `dimension_sizes` is not specified for a single stack dimension.
+                If specified along with `dimension_sizes`, their product must match `ifd_count`.


I think dimensions, dimension_sizes and ifd_sequence_order can be removed as options. Instead, there should be a single unnamed dimension as the "stack" dimension. Then a separate reshape adapter TensorStore driver (not yet implemented, but on the todo list can be responsible for doing the equivalent of those options.

(Instead of an unlabeled dimension, the stack dimension could also be assigned some default label like stack)

I think the issue with that, if I understand enough about how tensorstore works, is that this will become an problem when we are pulling that metadata from the file itself (e.g. OME-TIFF), versus having the user specify that information (which they may not know since it's embedded in the image) using a reshape adapter.

The reason I added these in here was to build out the necessary functionality and options such that I can later on just read the XML data from the appropriate TIFF tag and assign it to existing parameters. I did not add that in yet because I wanted to get the base, non-OME-specific, TIFF reader cleared first. Then adding in the XML parsing and populating these fields becomes a trivial exercise.

If you feel differently, or would still prefer that I just assign a single stacking dimension, I can do that. But (a) I think that means a longer runway for OME-TIFF support and (b) I don't know then how the XML metadata would work its way up to the reshape adapter.

Please advise.

jbms · 2025-05-06T16:37:46Z

tensorstore/driver/tiff/schema.yml

+                the last dimension varying fastest.
+          required:
+          - dimensions
+        sample_dimension_label:


I think this is unnecessary because the dimension labels can instead be overridden via the index transform.

The reason I added this is because OME XML has dimension labels in its metadata (XYZCT). I referenced the n5 driver a lot in my development and I saw that it has an axes member for dimension labels in N5Metadata. As with my previous comment, right now, they are manually populated, but when the OME XML is parsed, I will populate this.

I realize I am in some ways optimizing for OME XML while trying to make a generic solution for TIFF images. The way I had envisioned it was that these are generic options for anyone to use to craft some interpretation of their TIFF file, though I can see how it can look a bit odd. Also happy to follow your guidance on this. Just explaining my rationale.

jbms · 2025-05-06T16:39:32Z

tensorstore/driver/tiff/schema.yml

+      allOf:
+      - type: object
+        properties:
+          dtype:


This can be excluded since the data type can be specified generically via the dtype and schema properties common to all drivers.

For the metadata property (which is just dtype and shape below), I think I ultimately got confused. I was trying to square three things: (a) the concept of metadata constraints that I saw in n5, (b) come up with a generic TIFF solution, and (c) preempt support for OME-XML. The properties that I put inside the tiff object - if we set aside OME XML - are technically not constraints. They specify different ways of interpreting the same underlying data (e.g. whether IFDs are stacked as a dimension or treated as separate images is in the mind of the user, not something the TIFF file is stipulating). It's only with OME-XML that it becomes true metadata.

I used the metadata object for properties that are not subject to interpretation, like the dtype, and shape (at least partially). I realize this is quite messy. I could perhaps move the tiff object into metadata, and then it functions the same way it does with n5: all members are optional, and they are populated if found (e.g. OME XML). But it will also serve one additional function: which is to specify the interpretation of the underlying data if no OME metadata is present. This is probably cleaner from a schema perspective, but it does mix up two distinct responsibilities.

Let me know what you prefer.

jbms · 2025-05-06T16:39:48Z

tensorstore/driver/tiff/schema.yml

+            $ref: DataType
+            title: Data type constraint.
+            description: Constrains the expected data type of the TIFF dataset.
+          shape:


This can likewise be excluded since it can be specified via the schema.

laramiel · 2025-05-09T17:19:33Z

tensorstore/driver/tiff/compressor_registry.h

+// This registry maps string IDs (like "lzw", "deflate") to factories/binders
+// capable of creating JsonSpecifiedCompressor instances.
+internal::JsonSpecifiedCompressor::Registry& GetTiffCompressorRegistry();
+


It seems unlikely that we need to register tiff compressor to start with, also the tiff tag value could map directly to a compressor instance (via an enum, or similar).

laramiel · 2025-05-09T17:20:34Z

tensorstore/kvstore/tiff/tiff_details.h

+};
+
+// Common compression types
+enum class CompressionType : uint16_t {


I'd probably find the hex values easier to track back to the spec.

laramiel · 2025-05-09T17:20:51Z

tensorstore/kvstore/tiff/tiff_details.h

+
+namespace tensorstore {
+namespace internal_tiff_kvstore {
+


Add a reference to the spec sections here as comments.

hsidky added 18 commits April 12, 2025 11:53

Initial read implementation. Single failed test for offsets outside o…

e994e70

…f IFD.

Tiff dir cache.

11b46e3

Added TIFF details.

6481868

Added image directory parsing.

a95d554

Added parsing into tiff dir cache.

46b7eec

Eager loading of external arrays.

648aece

Updated tiff KVstore to use new cache.

e0c7d06

Moved byte range request to its proper location.

9050dd7

Multi IFD parsing in tiff cache checkpoint. Tests failing.

5737b6e

Fixed bugs. Multi-IFD cache working.

ca4a562

Added test for multi-ifd + external arrays.

90d962d

Tiff KVStore testing

a5d1144

Added more tags + more external arrays + tests.

63190d7

Code formatting update.

4da1f4a

Cleaned up tests + consolidated tiff builder

919cade

General cleanup

8947819

Added index.rst and schema.yml

43b1fd7

moved raw data to buffer + sorted EstimateHeapUsage

9d6bcee

hsidky added 9 commits April 19, 2025 13:11

tiff driver metadata initial buildout.

1970a86

Tiff metadata schema validation + fixes + tests

0c5f078

Refactor of resolvemetadata + new tests

1fab2b8

Missed formatting on metadata.

c5b477c

Initial TIFF Compressor

c010a31

TIFF CodecSpec and tests.

1aff8ac

Implemented tiff decodechunk + tests.

ad16397

Added GetParseResult to tiff kvstore.

836f3e9

Metadata fixes + enhancements.

c3d977e

hsidky added 21 commits April 27, 2025 11:43

Initial tests + associated fixes. Still failing.

059d180

More testing. More fixing. Fixed inner order issue too.

dd6e8b3

More fixes. All basic testing passes.

5e428a8

Cleanup.

34ca3de

Updated tiff details to chunks.

f2589d1

Fully linearized tiff kvstore.

2d666b2

mid tiff metadata refactor checkpoint

1c74faf

Finished metadata refactor. Cleanup necessary. Tests pass.

701c974

Finished first pass refactor of driver + supporting metadata changes.…

58769df

… Working on crashes.

fixed decode chunk logic. old tests passing.

efe26dc

sneaky bug fix

066cd48

golden file tests + bug fixes.

42d528c

code clean up + fixed label tests.

316f6ab

GetChunkStorageKey optimization.

55e55c0

Comment + code cleanup.

4d7856f

Fixed dangling reference in stacking info order.

5292db1

Added proper compression + refactor + bug fixes

a603025

added zlib compressor

9544989

fixed tiff driver schema validation + metadata comrpessor test.

d4f808c

updated golden file test files path.

5150abe

Added schema and index rst + updated build files.

61087ad

jbms reviewed May 6, 2025

View reviewed changes

laramiel reviewed May 9, 2025

View reviewed changes


		namespace tensorstore {
		namespace internal_tiff_kvstore {

Add TIFF Key-Value Store Implementation (take 2) #230

Are you sure you want to change the base?

Add TIFF Key-Value Store Implementation (take 2) #230

Uh oh!

Conversation

hsidky commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbms commented Apr 18, 2025

Uh oh!

jbms commented Apr 18, 2025

Uh oh!

hsidky commented Apr 19, 2025

Uh oh!

hsidky commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hsidky commented Apr 17, 2025 •

edited

Loading

hsidky commented May 5, 2025 •

edited

Loading