Bioformat image server reader pool extraction #1287
Conversation
|
Thanks!
I think that's fine because it makes sense for the OMERO extension to depend upon the Bio-Formats one - at least for raw pixel access via ICE, since many other dependencies are shared. And if we follow the advice of accessing pixels by Zarr then we might still have a Bio-Formats dependency via
Yes, that would be good. But we can merge sooner if it helps.
I don't think you need to worry too much about making them very generic - just to work well enough for Bio-Formats and OMERO. They both have a quite different way of returning pixel arrays that I haven't seen elsewhere. Based on the recent forum discussion, I have the impression that the current working Zarr support for Java uses n5-zarr, which in turn relates to (I think...) imglib2. Since we already plan to explore imglib2, there's a chance that a lot of QuPath's |
It is used with View → Show slide label - but is really only relevant for some file formats (although useful when relevant). It's inspired by the 'associated images' provided by OpenSlide here - since otherwise QuPath would have had no way to provide access to the label etc. But it doesn't map so easily to images from other readers, including Bio-Formats, which doesn't identify label images as being different. |
Do you know a way to test it? This Show slide label window always indicates "No label available" with the images I have. Apart from that, I think this pull request can be merged. The bio-format and omero ice image servers seem to be working with these new changes. I may still have to clean the code a bit but I think having the OMERO extension working properly is more important for now. |
There was a problem hiding this comment.
Thanks, I've added some comments.
The main question (maybe to discuss with @alanocallaghan and @finglis) is whether we should use Optional instead of just returning null. I am slightly in favor of using it sometimes - especially when the return really is optional - but here it seems to be used where throwing an exception would be preferable.
Returning null can be informative, inasmuch as it suggests we have a sparse image without pixels for every location - and shouldn't happen whenever there has been an exception.
Apart from that, I'm not sure if the ReaderWrapper interface and ReaderPool abstract classes have quite the right design for ease of interpretation and reuse.
ReaderWrapper looks very tied to the 'OME' way of doing things (Bio-Formats and OMERO); I'd expect a general image reader to return something more informative than a byte[][], which can only be interpreted with a lot of other return values and Bio-Formats logic. So it isn't very easy to use in a standalone way.
That isn't in itself a problem, but if writing a general image reader for use with the OMERO web API or IIIPImage Server (for example) I imagine it would be far harder to return a byte[][] than a BufferedImage.
So I think it should either 1) embrace being Bio-Formats/OMERO-specific, and prioritise simplicity, or 2) incorporate more of the processing logic that converts the byte[][] into a BufferedImage, and prioritise reusability. If the goal is for ReaderWrapper and ReaderPool to be reusable, it needs to be easy to generate and work with their return values.
ReaderPool then is abstract, but has very few abstract methods. One is to create a ReaderWrapper. This probably isn't helpful outside the class. It could also be handled with composition rather than inheritance by passing a Supplier<ReaderWrapper> as an argument to its constructor if really necessary, like when creating a ThreadLocal. If this change is made, ReaderPool could still be subclassed, but wouldn't have to be subclassed.
Elsewhere ReaderPool contains a lot of logic for image reading, which feels like it belongs in the reader itself - not the pool for managing readers. And it's also quite Bio-Formats-focussed, since the idea of a series within an image is quite Bio-Formats-specific.
So overall I don't have a clear idea of the logical separation between ReaderWrapper and ReaderPool. It feels like the logic of image reading is now more split across more classes + Bio-Formats itself, and it's quite hard to trace what is happening.
Finally, reading this reminds me that there were two important limitations in the original design:
- It only supports returning all pixels for all channels simultaneously. In preparation for the future, it would be beneficial to have an API that optionally supports returning individual channels.
- This isn't needed if the refactoring is minor. But any major refactoring has a chance of regression (in terms of some obscure images failing), so we should try to avoid doing it multiple times.
- Associated images can sometimes be very big - even pyramidal or with multiple channels. So the logic for reading them doesn't have to be fundamentally different to the logic for reading other images. From a Bio-Formats perspective, you might just request the image for a different series.
What do you think?
| * <p>A {@link ReaderPool} to use with Bio-Format images.</p> | ||
| * <p>It uses the {@link BioFormatReaderWrapper}.</p> | ||
| */ | ||
| class BioFormatReaderPool extends ReaderPool { |
There was a problem hiding this comment.
Note it should be BioFormats and not BioFormat (also elsewhere).
Also: please leave a blank line after the class definition and before the first private fields are defined.
Usually there should be a logger first as well - here, I think it would make sense to log at the debug level when createReaderWrapper is called.
(I realise that's not always the case in other classes, but we have a better log viewer now :) )
There was a problem hiding this comment.
Usually there should be a logger first as well - here, I think it would make sense to log at the debug level when createReaderWrapper is called.
Until now I've never used logger.debug, when should I use it?
There was a problem hiding this comment.
I'd say whenever there's info that could be useful to track down an error, but we wouldn't want logged routinely (since info level is the default).
Then logger.trace helps for really fine-grained information (e.g. it might be used to record rendering times in the viewer). Logs at trace level should almost never be turned on, because they will result in enormous logs.
Logging levels aren't used consistently enough in QuPath, but the new log viewer makes them far more useful.
...nsion-bioformats/src/main/java/qupath/lib/images/servers/bioformats/BioFormatReaderPool.java
Outdated
Show resolved
Hide resolved
qupath-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderPool.java
Outdated
Show resolved
Hide resolved
qupath-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderPool.java
Outdated
Show resolved
Hide resolved
...h-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderWrapper.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This logic seems substantially different from the original implementation - are the changes required for any specific purpose?
I'd like to understand the changes since the original version evolved over many years, with bugs were fixed in response to user reports for individual images that failed. But it could still be changed if there is a better way to do things. The code here worked for all images I tried.
There was a problem hiding this comment.
What logic is different from the original implementation? I moved code from BioFormatsImageServer but I tried to keep the same logic. Maybe it's clearer now with the last version which removes the confusion between ReaderPool and ReaderWrapper
qupath-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderPool.java
Outdated
Show resolved
Hide resolved
...h-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderWrapper.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Create an image from the supplied parameters (adapted from |
There was a problem hiding this comment.
Could you use AWTImageTools directly, rather than needing to adapt code from it?
I imagine it's not reusable from OMERO, but might it be possible to create a minimal IFormatReader that wraps around your code for requesting images from OMERO?
(Just an idea, may not be workable or wise...)
There was a problem hiding this comment.
I found it easier like this, because the creation of a IFormatReader seemed to be complicated to me (by looking at the constructor of BioFormatsReaderWrapper). I removed some parameters of the function in the last version
qupath-extension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/ReaderPool.java
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback!
You're right, I was overusing
I changed
I made
I moved the image reading logic from
Should I add a
I'm not sure I understood this point. |
There was a problem hiding this comment.
Thanks! I just looked quickly - first impressions are good, I'll try to check out the code in more detail and run it later.
I changed ReaderWrapper to be as generic as possible (its read function now returns a BufferedImage).
That sounds better for now. You could go a step further and define ReaderWrapper<T> where <T> is a generic parameter that may be BufferedImage.
That's the case with QuPath's ImageServer implementations. In practice it was probably not needed, since we always use BufferedImage. In the distant future, we might get rid of that entirely to avoid BufferedImage (which I think isn't available on android, for example) - but that would be a massive change.
Therefore keeping BufferedImage is fine, but switching to <T> would give you flexibility if you thought that something other than BufferedImage would be meaningful for Bio-Formats or OMERO.
| * @return the image corresponding to these parameters | ||
| * @throws IOException when a reading error occurs | ||
| */ | ||
| BufferedImage getImage(TileRequest tileRequest, int series, int numberOfChannels, boolean isRGB, ColorModel colorModel) throws IOException; |
There was a problem hiding this comment.
Rather than int numberOfChannels, we probably want either int channel (if we support only all channels for -1 or individual channels to be returned), or int[] channels (if we support arbitrary combinations of channels).
Either option would give more flexibility than the current API, but must support a parameter value that will return all channels (since that's the current default behavior, which should remain the default).
The javadoc for series should specify that it's ignored if not relevant for the image reader (since most readers only support one image).
There was a problem hiding this comment.
Do you remember the logic behind the OMEReaderWrapper.createSampleModel() function? Supporting arbitrary combinations of channels will require to edit this function, and I'm a bit confused with this part:
int[] offsets = new int[numberOfChannels];
int[] bandInds = new int[numberOfChannels];
int ind = 0;
int channelCount = getChannelCount(series);
for (int cInd = 0; cInd < channelCount; cInd++) {
int nSamples = getChannelSamplesPerPixel(series, cInd);
for (int s = 0; s < nSamples; s++) {
bandInds[ind] = cInd;
if (isInterleaved()) {
offsets[ind] = s;
} else {
offsets[ind] = s * tileRequest.getTileWidth() * tileRequest.getTileHeight();
}
ind++;
}
}
// TODO: Check this! It works for the only test image I have... (2 channels with 3 samples each)
// I would guess it fails if pixelStride does not equal nSamples, and if nSamples is different for different 'channels' -
// but I don't know if this occurs in practice.
// If it does, I don't see a way to use a ComponentSampleModel... which could complicate things quite a bit
int pixelStride = numberOfChannels / effectiveC;
int scanlineStride = pixelStride * tileRequest.getTileWidth();
return new ComponentSampleModel(
dataBuffer.getDataType(),
tileRequest.getTileWidth(),
tileRequest.getTileHeight(),
pixelStride,
scanlineStride,
bandInds,
offsets
);In particular, what is the difference between numberOfChannels, effectiveC, and channelCount?
There was a problem hiding this comment.
I struggle to remember the logic exactly - only that it was really difficult to get it right...
The general idea is that there are three confusingly inter-related terms that I will attempt to define (at least according to my best understanding):
Channelsare essentially as described here; different 2D images that are usually viewed using different colors that are mixed together (e.g. red, green & blue)Bandsare arrays of pixels, which can correspond to channels - i.e. each channel has a band - but don't have to, becauseSamplesprovide a way to store multiple channels in a single band
In the end, if you store your channels in a single band then the values are interleaved (e.g. RGBRGBRGB... - this would be 1 band with 3 samples per pixel).
Alternatively, you might store your channels in separate bands, which can be referred to as planar storage (RRRR.... GGGG.... BBB... - 3 bands with 1 sample per pixel).
So to construct a BufferedImage, you need a SampleModel in order to be able to make sense of what is actually contained within the bands.
As far as I recall, the scanlineStride is generally the width of the image - but might not be. The main time when it isn't is if you have a subimage that is backed by the same data as the original, larger image.
QuPath refers to channels as the 'intuitive' concept, but the actual technical storage relies upon bands and samples.
You don't necessarily have to bother with this now - especially if there is a chance of regression, since it seems to be working ok. If the API supports arbitrary channels, I think it's ok to throw an UnsupportedOperationException if it's actually called (and document this) - since it can just be a placeholder for the future. QuPath itself doesn't have an API to request fewer channels in ImageServer anyway, but it will likely need one in the future because it's inefficient to request 50 channels all at once if they aren't needed.
If you do change this logic at all, one way to test it is working is to export multichannel images using code like this, with channelsInterleaved() turned on and off.
There was a problem hiding this comment.
Just to add - you'll see bands and samples in the Java documentation, but I think Bio-Formats tends to refer to channels and samples. Unfortunately, in this case channels doesn't necessarily match which QuPath's idea of channels.
I think you should probably consider a Bio-Formats channel to be like a BufferedImage band.
There was a problem hiding this comment.
OK, thanks for the explanation.
If that's fine with you, I can change only the public API for now (and throw UnsupportedOperationException like you mentioned) and will come back to this later (like when a first version of the omero extension will be ready)
...n-bioformats/src/main/java/qupath/lib/images/servers/bioformats/BioFormatsServerOptions.java
Outdated
Show resolved
Hide resolved
...tension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/readers/ImageData.java
Outdated
Show resolved
Hide resolved
|
I added a commit with the changes you requested. Since |
There was a problem hiding this comment.
I think this design is better, but I think it's worth trying to simplify even further.
The Bio-Formats API is very complex - justifiably, since it has to handle so many complex formats - and not thread-safe, so there is a high risk of bugs when we try to use it efficiently and in a parallelized way.
One of the biggest dangers is that an ImageReader can have its ID set (effectively choosing a different input file), and also its series set (effectively choosing a different image within a file). Any metadata we request from an ImageReader is only consistent if the ID and series have not been changed, which makes lazily requesting metadata risky - and is where I think problems could occur here.
Therefore an important question is where exactly the metadata should be parsed - and ideally this happens just once.
I think it would be useful to define the preferred public API to support Bio-Formats and OMERO first - identifying where code reuse makes sense.
One idea might be to decouple the conversion of byte arrays into BufferedImage objects. Here's a rough sketch of a class, which could be reused across all readers for a single image and threadsafe (caching color and sample models for reuse).
class OMEPixelParser<WritableRaster> {
// Method to convert byte arrays according to the known metadata
BufferedImage parse(byte[][] pixels, int width, int height) {
// ...
}
// We might want to treat this as a special case, because if we create a BufferedImage
// using a ColorModel, Raster and SampleModel, then it may not have the correct
// RGB image type set
BufferedImage parseRGB(byte[][] pixels, int width, int height) {
// ...
}
// Optional method that makes it possible to handle arbitrary channel combinations,
// at the cost of requiring color and sample models to also be created
WritableRaster parseRaster(byte[][] pixels, int width, int height) {
// ...
}
// Create a sample model for the specified number of channels
SampleModel createSampleModel(int nChannels) { ... }
// Create a color model for arbitrary channel combination (if empty, assume all channels)
ColorModel createColorModel(int... channels) { ... }
// Builder to set all necessary metadata values once -
// and these cannot be changed
static class Builder {
Builder isInterleaved(boolean isInterleaved) { ... }
Builder isRGB(boolean isRGB) { ... }
// We will need the default channel names & colors for anything other than RGB
Builder channels(List<ImageChannel> channels) { ... }
Builder pixelType(PixelType pixelType) { ... }
OMEPixelParser build() {
return new OMEPixelParser(...);
}
}
}I don't know if this is a workable design - it's really just a suggestion for consideration and discussion.
...ion-bioformats/src/main/java/qupath/lib/images/servers/bioformats/readers/ReaderWrapper.java
Outdated
Show resolved
Hide resolved
...ension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/readers/ReaderPool.java
Outdated
Show resolved
Hide resolved
...ension-bioformats/src/main/java/qupath/lib/images/servers/bioformats/readers/ReaderPool.java
Outdated
Show resolved
Hide resolved
| * Wrapper suited for readers that return arrays of bytes when reading pixel values. | ||
| * </p> | ||
| */ | ||
| public abstract class OMEReaderWrapper implements ReaderWrapper<BufferedImage> { |
There was a problem hiding this comment.
I can see the logic in this class for supporting both Bio-Formats and OMERO, but it means that it must go beyond the ReaderWrapper implementation.
I have the impression that the metadata parsing then becomes fragmented.
It is already slightly fragmented within Bio-Formats itself, since you can request some overlapping information from the ImageReader/IFormatReader or from the OMEPyramidStore.
QuPath needs metadata values to be parsed into the ImageServerMetadata object used by the ImageServer, and the logic for this is inside the BioFormatsImageServer class. But then information is also provided from OMEReaderWrapper directly, independent of whatever was parsed by BioFormatsImageServer. It seems that they could be inconsistent.
And, in fact, the metadata probably will be inconsistent sometimes in BioFormatsReaderWrapper - at least if this class is used different from how it currently is - because of an awkward fact of image files that contain multiple series: they don't necessarily have the same metadata. For example, they can be different sizes, have different channel counts etc.
So when this returns a value for getPixelType() (for example), then that may only be correct for one particular series - and the API doesn't tell us which one. In fact, the returned value might change if there are calls to reader.setSeries(int).
Often this may be unnoticed because different series happen to share enough similar metadata, but it isn't guaranteed.
If returning metadata is necessary in this class, then a way to overcome this is to forbid taking the series as a parameter to getImage, and instead passing it during construction. This means that a new instance would need to be created to access a different series, but it would help reduce the risk of subtle bugs in the future.
Sidenote: this replicates some of
IFormatReader, but with different naming (e.g.getEffectiveChannelCount()rather thangetEffectiveSizeC()) - but you still need to know the Bio-Formats API to use it, and to make sense of the name shift fromseriestoimageIndexin some method parameters.
| */ | ||
| T getImage(TileRequest tileRequest, int[] channels, boolean isRGB, ColorModel colorModel, int series) throws IOException; | ||
|
|
||
| /** |
There was a problem hiding this comment.
It may be possible to remove this method, and rely entirely on the other getImage() method - but it would require some thought.
Basically, getAssociatedImage() makes complete sense when using OpenSlide: an associated image is guaranteed to be 'small' and RGB - at least, that's what the official Python API seems to assume here, and OpenSlide is inherently limited to RGB.
It also makes sense at the level of the ImageServer API, because users will often want to see the label or overview image associated with their whole slide image. Initially, ImageServer was also designed with OpenSlide in mind.
I'm not sure it makes sense at the Bio-Formats API level, because the concept doesn't exist there: every image is a series. QuPath attempts to figure out which series in a file are likely to be labels or macro images based upon their names, but (annoyingly) these 'probably associated' images are not guaranteed to be small (see here) or RGB (see here). They could be pyramidal and non-RGB. Which is largely a consequence of Bio-Formats supporting different file formats to OpenSlide, and some of them do more awkward things.
This means that the pixel requests don't have to be different from requesting any other series. The logic for converting them into a manageable size could exist within the ImageServer, and doesn't have to exist here.
Relying on getImage(int series) for associated images effectively means that we have no way to request a low-resolution version if the image happens to be large and pyramidal, because there's no way to pass TileRequest info.
| * @return the image corresponding to these parameters | ||
| * @throws IOException when a reading error occurs | ||
| */ | ||
| T getImage(TileRequest tileRequest, int[] channels, boolean isRGB, ColorModel colorModel, int series) throws IOException; |
There was a problem hiding this comment.
Extending some of my other comments, I guess there are two options:
- this interface assumes that any implementing reader can handle multiple images, and so requires a
seriesparameter. This assumes that the images are accessed by index (rather than name). - this interface assumes that the internal reader already knows exactly which image it refers to (i.e. if a
seriesis required, it is stored internally as a field)
The ImageServer interface takes the second approach: you don't need to specify an series when requesting pixels or metadata, because the server returns pixels and metadata only for one specific series. If you want a different series, you need a different ImageServer.
The argument for the first approach is that you can then reuse the same Bio-Formats ImageReader multiple times: setting the series and/or ID, and not creating a new reader every time. These readers would then be managed by the ReaderPool.
Care would need to be taken because Bio-Formats isn't threadsafe, and it's especially important to avoid setting the ID or series on a reader when it is still in use by another thread.
|
I marked the comments that I completely understood as resolved. About the rest, am I correct if a recap of what you ask is:
However I didn't understand where the |
Yes. See https://downloads.openmicroscopy.org/images/Vectra-QPTIFF/perkinelmer/PKI_fields/ and This contains a 32-bit float image, along with an 8-bit thumbnail. If I try to open the thumbnail with this PR it fails, I believe because it is using a mixture of metadata (i.e. assuming that it has enough bytes for 32-bit data, and failing with an
Possibly - it is one option to overcome the issue. Currently, the implementation of Without the reset, then the reader has changed into a different state - and the values returned by any call that requests metadata from the reader are subject to giving different results (example at the end of this post). Additionally, both methods are potentially broken in a multithreading context because there is no synchronization done on the reader. Excessive synchronization could harm performance. Forbidding the series and ID to be changed anywhere inside the class - and forbidding the reader from being accessed outside (i.e. not providing a The alternative is to synchronize everything that uses the reader, and then taking care to design the class in such a way that it's not possible to get around the synchronization. To do that, the A third option is to make the class really minimal and keep the The third option puts much more responsibility on the caller, but has the advantage of allowing the same reader to be reused for different images / series. This might have some small improvements in performance (especially if initializing a reader is slow), but could be brittle and easy to get wrong.
Ideally yes. As the
I think we should go back to thinking about the ideal design here, based upon what needs to be reusable - and also what are the simplest and safest changes that can be made before the v0.5.0 release. My understanding of the original requirements is
Achieving 1. requires a class to do the parsing, but doesn't necessarily require reader wrappers and reader pools at all. These seem to be where the main dangers lie, because Bio-Formats is complex to use in a multithreaded context. On the other hand, the parsing doesn't need to know anything about an If you can extract the bytes-to-BufferedImage logic in an entirely threadsafe way, and leave as much as possible of In general, I am cautious about about extensive refactoring of I realise it's incredibly hard (/ impossible) to write this without failing examples, and most public examples won't fail because we get lucky with the different series types. My guess is that .czi is one of the more awkward formats. Based on that, I found another failing example here: https://zenodo.org/record/7149674 Specifically, check out the label and macro images with the PR vs. in QuPath v0.4.4. In this case, the problem is related to the 'interleaved' interpretation: there's no exception thrown, but the image obtained via the PR is incorrect. If you open the image in QuPath, this Groovy script shows that the def wrapper = getCurrentServer().readerPool.getDedicatedReaderWrapper()
println "Original interleaved: " + wrapper.isInterleaved()
wrapper.getReader().setSeries(0)
println "Series 0 interleaved: " + wrapper.isInterleaved()
wrapper.getReader().setSeries(1)
println "Series 1 interleaved: " + wrapper.isInterleaved()
wrapper.getReader().setSeries(2)
println "Series 2 interleaved: " + wrapper.isInterleaved() |
|
OK, I just created another pull request only with the |
Initial refactoring of the bio-format image server to extract code that could be reused in the OMERO extension.
Basically, the bio-format image server uses a pool of readers to retrieve pixel values in parallel. This pull request extracted this behavior from the bio-format image server.
4 classes were created:
ReaderPool: abstract class that can read pixel values of an image using several readers in parallel. A class extending this class must define how to fetch pixel values, but the rest is handled byReaderPool.BioFormatReaderPool: implementation ofReaderPoolwith specific code to open bio-format images.ReaderWrapper: interface which is a wrapper around an image reader. It is suited for readers that return arrays of bytes when reading pixel values.BioFormatReaderWrapper: implementation ofReaderWrapperwith specific code to open bio-format images.The
BioFormatImageServerandBioFormatServerBuilderwere slightly changed to adapt to the new classes.The unit tests passed with these changes.
This pull request should not be merged now because I still have to address a few things:
ReaderPoolandReaderWrapperclasses? Currently there are in theservers.bioformatspackage, but they are not specific to bio-format.ReaderPoolandReaderWrapperin the OMERO extension, so I may have to change a few things if I realize that theses classes are not completely generic.I also have a question:
getAssociatedImage(String)function ofqupath.lib.images.servers.ImageServeris used? I don't think I was able to test it