Switch from PixelBuffer to Zarr metadata and array cache#3
Switch from PixelBuffer to Zarr metadata and array cache#3chris-allan merged 2 commits intoglencoesoftware:masterfrom chris-allan:fix-pixel-buffer-cache
Conversation
sbesson
left a comment
sbesson
left a comment
There was a problem hiding this comment.
Similarly to #1 (comment), integrated these changes into a development branch of omero-ms-image-region for deployment and testing on a development instance.
The rendering of RGB and fluorescent OME-Zarr images as well as OME-Zarr labels hosted on our public S3 bucket is working as expected in PathViewer. For digital pathology data in particular, the navigation experience including the bird's eye view as well as zooming & panning functionality was working without any error while retrieving tiles.
My inline comments are related to possible next steps so I think we could get this included as is but @kkoz might want to take a look at the changes.
| throws IOException { | ||
| // FIXME: Really should be ZarrUtils.readAttributes() to allow for | ||
| // attribute retrieval from either a ZarrArray or ZarrGroup but ZarrPath | ||
| // is package private at the moment. |
There was a problem hiding this comment.
In the same spirit as the work we did with @melissalinkert on bioformats2raw/raw2ometiff, I was considering switching to the more active zarr-developers/jzarr fork on this repository as well
This might be an opportunity to discuss any API improvements we should consider as a preamble of this update.
There was a problem hiding this comment.
Opened as zarr-developers/jzarr#19. I think it makes sense to switch to that fork for testing as soon as possible.
| .maximumSize(0) | ||
| .buildAsync(PixelsService::getZarrArray) | ||
| ); | ||
| } |
There was a problem hiding this comment.
👍 I'll port similar changes to the omero-ms-image-region unit tests instantiating ZarrPixelBuffer .
This raises the question of whether AbstractZarrPixelBufferTest (which originates from the omero-ms-image-region tests) is still relevant and could be consumed by downstream components. In that case, there's probably a case for moving this utility method under this abstract class.
There was a problem hiding this comment.
Sounds good. I'm not sure the utility now to be honest.
2 participants
Caching at the
PixelBufferlevel can cause concurrency issues due to the combination ofsetResolutionLevel()andgetTile()not being used atomically in all situations. We can also improve performance by cachingZarrArrayinstances and not recreating them when switching between resolution levels.