pearcetm / GeoTIFFTileSource

Implementation of a TileSource for OpenSeadragon based on GeoTIFF.js, enabling local and remote TIFF files to be viewed without using an image server
MIT License
27 stars 5 forks source link

Address #17 by filtering Macro and Label images from SVS files #18

Closed mdushkoff closed 3 weeks ago

mdushkoff commented 5 months ago

This should fix #17 by filtering out macro & label images from SVS slides. Currently looking for publicly available test data to validate this change.

abiswas97 commented 5 months ago

@mdushkoff instead of filtering out the macro and label, what do you think of separating them into different image sets? Since the code currently groups images by aspect ratio and maintains them as separate image sets - which then get flagged as tiled or not. I'm assuming the issue you're facing is an image where the aspect ratio of the macro or label matches the regular tiled image, causing it to accidentally be grouped with the pyramid.

It might be desirable to keep track of the macro or label image for rendering elsewhere.

@pearcetm what do you think?

mdushkoff commented 5 months ago

Strangely, the scenario that prompted this was actually a case where the macros and labels had completely different aspect ratios from the real tile images and were still getting rendered over the real slide data. It could definitely be useful to keep the macro and label images for rendering elsewhere, but they should definitely be excluded from the pyramid. If they were kept what would be a good way to represent them to the user, @abiswas97 ?

pearcetm commented 5 months ago

Strangely, the scenario that prompted this was actually a case where the macros and labels had completely different aspect ratios from the real tile images and were still getting rendered over the real slide data.

That's strange. When I use the demo page to open an SVS file, it opens the images in sequence mode in OpenSeadragon. The image pyramid works correctly as the first image:

image

If you use the button to navigate to the next pages, you'll see the macro and then the label:

image

Is the demo page giving you the same results with your SVS files as your local build?

mdushkoff commented 5 months ago

With this branch, the demo page displays the slides correctly and only the slides, no macro/label.

In the previous branch using the demo above, my slide sample renders the label as part of the pyramids just as I was having trouble with before.

mdushkoff commented 5 months ago

Upon further inspection @abiswas97 was correct about the aspect ratio being relatively close in that they fall within the current tolerance, however they're at the very far end of this. I've modified the existing filter to include information about the image description. With this change, the label/macro images are preserved as separate sets as mentioned above, but never included in the pyramid.

pearcetm commented 5 months ago

Upon further inspection @abiswas97 was correct about the aspect ratio being relatively close in that they fall within the current tolerance,

I figured this must be true. With the wide variety of TIFF files out there, I tried to detect which pages belong in a pyramid automatically, but obviously this approach can fail in edge cases like the one you found.

I'm definitely open to checking for special cases we know about in advance (like SVS files). Let's just make it as robust as possible while we're making the change.

mdushkoff commented 5 months ago

I've included your suggestions above to the latest.

Speaking of robustness, do you believe that there should be additional checks based on the file type itself or should they all be handled in the mapping function regardless? I'm not sure how likely or if ever it would be the case that a non-SVS file implements a label/macro that is expected to be part of the tileset.

abiswas97 commented 5 months ago

@mdushkoff My setup is similar to the demo page, where I use OSD's sequence mode to allow the user to view supplementary images if present.

I think having a check beforehand might help, since having file type specific context can be useful for the entire class. Maybe an ImageReader class that can have its own pre/post processing step when determining tilesets?

As for how likely other file types are to have a macro, I've noticed that most tiff based format (svs, tiff, ome.tiff, qptiff etc) do include one

mdushkoff commented 5 months ago

@abiswas97 In those tiff based formats, do you know if it is expected that the macro is a separate image set to the pyramid, or is that an SVS-specific case to keep it separate?

pearcetm commented 5 months ago

@abiswas97 In those tiff based formats, do you know if it is expected that the macro is a separate image set to the pyramid, or is that an SVS-specific case to keep it separate?

Although I'm not aware of the details of the other TIFF formats that include macro images, I suspect that they, like SVS, would not expect the macro to be part of the main image pyramid.

pearcetm commented 4 months ago

This is looking good to me, @mdushkoff.

@abiswas97 did you have any further thoughts, or should I go ahead and merge this as-is?

pearcetm commented 2 months ago

Checking back in on this @abiswas97 and @mdushkoff. Do you think this is ready to merge?

mdushkoff commented 2 months ago

@pearcetm I'd say so, unless there's any other concerns.

abiswas97 commented 1 month ago

@pearcetm @mdushkoff just getting to this - sorry for being late to this discussion.

In images where I've noticed macro images, they've been separate images independent of the main image pyramid. So far I've seen this in tiff, ome.tiff, qptif, and svs (mostly svs). I'll see if I can find an open source example to attach here.

Given how freeform TIFF is, maybe some won't adhere to this standard? But maybe we can take it into account if we get pointed at an example from any future issue?

I think this looks good to merge. My only nit would be some clarity what s is, but this should be a very helpful change!

pearcetm commented 3 weeks ago

Thanks for this fix! Merging now.