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

Implement mode in backends #207

Merged
merged 9 commits into from
Feb 26, 2024
Merged

Implement mode in backends #207

merged 9 commits into from
Feb 26, 2024

Conversation

jonasteuwen
Copy link
Contributor

This implements a mode flag in the SlideImage backends. This is required since the ICC profiles need to know what the output type is (e.g. RGB or RGBA). While OpenSlide always returns RGBA, this is not the case for libvips.

@jonasteuwen jonasteuwen marked this pull request as ready for review December 8, 2023 23:10
@github-actions github-actions bot added the python label Dec 8, 2023
@BPdeRooij
Copy link
Contributor

The mode flag implementation seems like a good solution. An error will be automatically raised if either inMode or outMode is not supported when building the ImageCmsTransform. ICC were correctly applied when tested on a TCGA image for both PyVipsSlide and OpenSlideSlide backend.

I do have some comments/questions about the PyVipsSlide backend:

  1. The mode can already be set when initializing PyVipsSlide. For _read_as_openslide this can always be "RGBA". For _read_as_tiff a combination of self._images[0].get("interpretation") (documentation) and number of bands can be used.
  2. Slightly off-topic, but related: could we use pyvips.Image attributes self._images[0].get("xoffset") and self._images[0].get("yoffset") as offset in the slide_bounds for _read_as_tiff?
  3. libvips reserves "icc-profile-data" for the ICC profile. ICC profiles are also stored in this property when pyvips.Image.tiffload is used. After some testing, I believe the NotImplementedError is not needed when using "tiffload".

@jonasteuwen
Copy link
Contributor Author

jonasteuwen commented Feb 23, 2024

I don't know about self._images[0].get("xoffset"), we should test that. We might also need to write those, so if we can write mrxs -> tiff we could use something like this if it works.

I don't think it will always be RGBA with openslideload, this will depend on the image you're loading (in contrast with openslide-python)

@jonasteuwen jonasteuwen added this to the v0.4 milestone Feb 23, 2024
Copy link
Contributor

@BPdeRooij BPdeRooij left a comment

Choose a reason for hiding this comment

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

I believe images will be loaded as RGBA when using openslideload, but checking the number of bands in images will also generate the correct results in any case. The changes look good to me.

@jonasteuwen jonasteuwen merged commit 5e2a630 into main Feb 26, 2024
8 checks passed
@jonasteuwen jonasteuwen deleted the bug/fix-icc-for-rgb-mode branch February 26, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants