bigdataviewer / bigdataviewer-core

ImgLib2-based viewer for registered SPIM stacks and more
BSD 2-Clause "Simplified" License
33 stars 35 forks source link

Use VolatileUnsignedTypes in createCacheArrayLoader? #85

Closed constantinpape closed 4 years ago

constantinpape commented 4 years ago

@tischi and me ran into an issue with loading unsigned integer types for n5 data. This may be related to the fact that in the code below, the unsigned types map to VolatileTypeArray instead of VolatileUnsignedTypeArray: https://github.com/bigdataviewer/bigdataviewer-core/blob/master/src/main/java/bdv/img/n5/N5ImageLoader.java#L365-L378

Is this on purpose? Note: we are not sure that this is actually causing the issues we see, we will investigate this more.

tpietzsch commented 4 years ago

Yes, that is definitely on purpose. Using the volatile arrays allows to put volatile (as well as non-volatile) Types "on top" of the data.

constantinpape commented 4 years ago

Yes, that is definitely on purpose. Using the volatile arrays allows to put volatile (as well as non-volatile) Types "on top" of the data.

Sorry, maybe the initial formulation of the issue was unclear, but the question was not about the use of VolatileArray, but rather why both signed and unsigned types map to the same volatile array. E.g. why is it

case UINT8:
case INT8:
  return new N5CacheArrayLoader(... VolatileByteArray);

and not

case UINT8:
  return new N5CacheArrayLoader(... VolatileUnsignedByteArray);
case INT8:
  return new N5CacheArrayLoader(... VolatileByteArray);
tpietzsch commented 4 years ago

There is no VolatileUnsignedByteArray, because there is no unsigned byte java primitive type. Basically, both UnsignedByteType and ByteType map into a primitive byte[] arrays (which VolatileByteArray is an abstraction over.)

constantinpape commented 4 years ago

Ok, I see. Thanks for the clarification.

We were assuming that there are VolatileUnsignedArrays, because there's VolatileUnsignedTypes, e.g. https://github.com/imglib/imglib2/blob/fb42809c5fa400f1f3a11f43e3931acfb93148ae/src/main/java/net/imglib2/type/volatiles/VolatileUnsignedLongType.java. But indeed the arrays are not there.

tpietzsch commented 4 years ago

@constantinpape What issue are you actually seeing with your n5 loader?

constantinpape commented 4 years ago

The uint64 data is not displayed properly in BigDataViewer: it just "flickers" when we are zooming or scrolling through the dataset. When we are not moving it is not shown. See also this video.

We have encountered this issue a few times before and it was always related to empty n5 chunks, cf #74. Our current solution for the empty chunk issue by @tischi is here and the issue might be related to this code.

tpietzsch commented 4 years ago

Hmm, nothing looks immediately wrong about that code.

Did you check that n has a reasonable value here: https://github.com/tischi/bigdataviewer-core/blob/16c6ab88d131154da3f552f765b1738a5e67a39d/src/main/java/bdv/img/n5/N5GenericImageLoader.java#L322 and that it actually ends up in the UINT64 case (as I expect it should) https://github.com/tischi/bigdataviewer-core/blob/16c6ab88d131154da3f552f765b1738a5e67a39d/src/main/java/bdv/img/n5/N5GenericImageLoader.java#L336 ?

When you are scrolling, it switches to a lower resolution, so that suggests there is data available for some (lower) resolution levels, but not for other ones. You can verify that by zooming out sufficiently far. Then it should stay visible?

constantinpape commented 4 years ago

Ok, it turns out this was due to code duplication and an old version of the empty array check being used. Sorry for the confusion and thanks for having a look @tpietzsch.