@s-n-i please try this out!

@manzt Do you see an issue with this conceptually?

Have been struggling to catch up on related viv thinfs. Will try to review this weekend!

I built Avivator from this codebase and loaded the following dataset into it: https://viv-files.ci.aws.labshare.org/LuCa-7color_Scan_c(0-5)/

Here are my observations:

The black box no longer increases in size, relative to the image size, when zooming out. So, this issue looks fixed.

I also found a new issue. The dataset disappears on levels 7 and above. Here is a screenshot, I changed the background to white:

So there is something wrong with my logic then probably - an edge case i had not considered. I'll have a look.

@s-n-i should be fixed now, thanks!

Just following up on this.

@s-n-i I lost track of this but I recall this actually not having mattered much in the end for you. Happy to push this ahead if you feel you need this. Let me try to refresh myself of the issue here.

Right yes, you had said the performance difference was actually not that great. So I sort of stopped working on this, but if you still feel you need it, we can restart the effort. Happy to oblige.

Restarting the effort would be great. Currently certain datasets appear to get padded with 0 intensity values. I think this PR would allow them to render correctly.

I was also wondering what the approximate timeframe for this effort is going to be.

I can take care of this by the end of next week. I'll start working on it tomorrow.

@s-n-i please have a look. if this works, i will merge

Whether the padding (black box) gets drawn appears to depend on the amount of zoom. Here are two screenshots with background-color: white; on <div id="deckgl-wrapper" and slightly different amounts of zoom.

is this data public?

I spent some time to find and test a publicly sharable dataset because the one above is not public.

Here is a link:

https://www.filemail.com/d/gnejlokjxpfodfg

The size of the black box changes based on zoom:

I am following up on this. Please let me know if anything else is needed.

Can you repost the file since it expired? I must have missed this while I was out on vacation. Happy to finish this.

Here it is:

https://www.filemail.com/d/omclxgvgiqanbpc

hey @s-n-i can you be sure you had the latest version of my PR? I am not seeing the behavior you're describing. I am seeing the "extra" tile in the height direction when you move to the first level away from the lowest one but that is not part of this fix.

P.S I have tried this in both FF and Chrome.

I am getting the padding shown in this screenshot. Is this what you mean by

when you move to the first level away from the lowest one

?

@s-n-i I think that might be a bit out of scope here. The goal was only to do this for the "top levels" of the pyramid (i.e., those levels, not tiles, that are too small) but what you are describing would have to be done at all levels of the pyramid since zarr tiles are often (always?) padded. I understand that this might be disappointing but I am not sure I am up for doing this for all zarr tiles. @manzt are you aware of some workaround or the like that would stop zarr from returning padded tiles?

Also @s-n-i, why do you feel this is needed? Are you not using a black background? Are you using color maps a lot? Is there a huge performance knock you are seeing?

Yes, I am using color maps and different background colors. In fact, I have a custom shader, which allows the 0 intensity color to be specified in a look up table. Therefore, the 0 intensity color (which is also the color of the padding) can be different form the background color.

Are you asking about the performance difference between a dataset with small top levels and the same one without?

I have also tested the PR with a dataset with small top levels. The size of the padding stays the same relative to the size of the image as the user zooms.

https://www.filemail.com/d/fgxqvbcdetjkcbx

@s-n-i I see what is happening. The background image is displaying black, which gets me back to the fact that this might be worth doing on the loader directly, as we do for tiff. @manzt Do you have any opposition to adding this feature to the zarr loader? I think this "padding" is a unique feature of zarr in that tiff file readers are actually supposed to clip the data if they are good: https://www.verypdf.com/document/tiff6/pg_0066.htm: "good
TIFF readers display only the pixels defined by ImageWidth and ImageLength
and ignore any padded pixels"

I think I see the problem. FWIW, I believe the underlying tiles for tiffs are also square, it's just we are using a higher-level API from geotiff than in zarr.js. Yes, tiles (or chunks) are always complete in Zarr. This way any key-value pair is the exact shape described in the .zarray metadata. If it were different, the individual chunks would some how need to encode their shape as well.

We currently use getRawChunk API from zarr.js, which is used to directly access chunks. This is used internally to stitch together slices of data (when using the getRaw or get APIs), including "trimming" the excess pixels.

Since the pixelSource interface has a tileSize, part of me thinks the return from getTile should aways be a tileSize * tileSize array, and then it's the responsibility of the caller to trim tiles based on the PIxelSource.shape if necessary. Otherwise each loader has to reimplement similar logic internally.

Either way, I think we should certainly handle this case in Zarr, it's just a matter of whether the logic should live within the loaders or at a higher level.

I think one argument for doing this at the loader level (either universally shared among all loaders or per-loader) is that this problem exists on non-multiscale images i.e., those that have to be stitched together from tiles. That was the problem @s-n-i was seeing after implemented the (working) fix for this specific problem. I would be open to writing some sort of inheritable method that all PixelSource classes can hook into if you think that works @manzt.

I think the choice is somewhat arbitrary, but I'll try to describe my thinking. We have a PixelSource which includes a tileSize property and shape property. All the tiled image formats I'm aware of have complete tiles internally, and then the readers are responsible for handling the trimming as you say. My idea with PixelSource was to create a shared interface to these data sources, meaning that other implementations don't need to re-implement tile handling and can integrate minimally.

The getTile method consistently returns a complete tile that is source.tileSize x source.tileSize:

await source.getTile(/* ... */) // { ..., width: tileSize, height: tileSize }

Whereas we have a getRaster method to return a contiguous, stitched plane of the image:

await source.getRaster(/* ... */) // { ..., width: shape[1], height: shape[0] }

With that said, I'm happy to be convinced that this should live in the PixelSource. The biggest priority should be to unify the interfaces and consistently handle this bug, wherever that code lives. More than anything we should be explicit about what we rely on from the PixelSource interface.

I would be open to writing some sort of inheritable method.

Or just a utility function. I'd really like to avoid introducing some unnecessary class hierarchy. Again, this shared behavior is what signals to me we should keep the PixelSource as a simple low-level interface to various formats, because we can handle the "edge" tiles with the public properties exposed by PixelSource.

The getTile method consistently returns a complete tile that is source.tileSize x source.tileSize:

await source.getTile(/* ... */) // { ..., width: tileSize, height: tileSize }

I think is actually not the case since the TiffPixelSource does not do this:

One thing I have thought about recently is that while it is "easy" to pass in coordinates for getRawChunk, _getTileExtent is fairly battle tested now. We could just copy the implementation of the TiffPixelSource and use normal slicing access for zarr.

I agree a unified approach would be good here. But if we mandate that getTile returns a "complete" tile and then implement some sort of utility around it, we have to roll back a change (in theory) to the TiffPixelSource. But if we use slicing access for zarr (via getRaw, I believe), we would be "implementing the utility" into the PixelSource and make no changes to our interfaces. In the future, for people who implement their own PixelSource classes, nothing would change. I also regret the number of utilities I have exported from the this library (they are mostly things I have written) so adding another is not something I really want to do. If we did it, I'd rather make it a class method. I guess the other option would be to use it only internally.

So my top option would be to use "normal" slicing access for zarr if possible via getRaw. Short of that, my next favorite option would be to add an optional class method to the PixelSource interface, have the zarr PixelSource implement it to trim tiles, and then have the MultiscaleImageLayer check for it. That being said, this doesn't solve the problem of the background image showing the border, so all image layers would have to check for this realistically.

I think is actually not the case since the TiffPixelSource does not do this:

Right, sorry for not being clear. I wrote that to point out the original intent with the PixelSource was to have a consistent return type. That is not the case, and I wasn't aware of the inconsistency.

But if we mandate that getTile returns a "complete" tile and then implement some sort of utility around it, we have to roll back a change (in theory) to the TiffPixelSource.

Yes, in order to make our implementations consistent, we will likely need to change the current behavior of either the Zarr or Tiff implementations. My focus in this discussion is to clarify where the change should occur, and my proposal was to make the Tiff source more like Zarr. If I understand you correctly, you are in favor of the opposite.

As I said before, I think the decision is fairly arbitrary, but the semantics/intent for the PixelSource is what motivated my suggestion.

In the future, for people who implement their own PixelSource classes, nothing would change.

This isn't entirely true. If the underlying library for reading the tiled data source supports trimming the tiles, then implementing a PixelSource should be minimal effort. But if it does not, then the developer will need to implement this trimming to be consistent with our implementations.

At the file level, the majority the pyramidal image formats I'm aware of store complete tiles to avoid storing additional tile-specific metadata. My suggestion for returning complete tiles is to make it easier to support these formats by reducing the (repeated) trimming logic that we would now require underlying libraries (or PixelSource implementers) to include.

I guess the other option would be to use it only internally.

Agreed. Whatever we decide, we should not introduce a public API. Since this "trimming" is a common operation, which only requires the public interface from the PixelSource, that's why I thought a utility function in our layers.

let tile = await source.getTile({ x, y, selection, signal });
let { width, height } = getTileExtent({ x, y }, source.tileSize, source.shape);
let trimmed = trimTile(tile, { width, height });

So my top option would be to use "normal" slicing access for zarr if possible via getRaw.

The last thing I want to do is delay the fix which solves this issue. Given the length of our discussion, I have a feeling we are venturing near Fredkin's paradox, and trust you to make a decision.

I was able to generate an overlay layer, which occludes the rendering artifact.

      createLoader(url).then((value) => {
        const lineWidth=value.metadata.Pixels.SizeX+value.metadata.Pixels.SizeY;
        const x = value.metadata.Pixels.SizeX+lineWidth/2;
        const y = value.metadata.Pixels.SizeY+lineWidth/2;
        setCoverLayers([new PolygonLayer({
          id: "polygon-#detail#",
          data: [[[[-lineWidth/2, -lineWidth/2], [x, -lineWidth/2], [x, y], [-lineWidth/2, y], [-lineWidth/2, -lineWidth/2]]]], 
          getLineWidth:lineWidth,
          filled: false,
          getPolygon: d => d
        })]);
      });

I tested that the visualization artifact is no longer visible with this dataset:

https://files.scb-ncats.io/pyramids/Covid_data/dpr-image-uploads/00/002a6681febb7166e51837684fb07e1e9e900cc3fd10107173b630b54719d645.ome.zarr

The code gets the size of the image from value.metadata.Pixels.SizeX and value.metadata.Pixels.SizeY. Can we use these same values to crop the image instead?