slim icon indicating copy to clipboard operation
slim copied to clipboard

Tile grid shifts upon zooming

Open Punzo opened this issue 3 years ago • 36 comments

From Steve: as Bill reported there's a shift to the down-right when high res tiles come in which may be a problem with the coordinate systems or maybe the data but probably the viewer.

Punzo avatar Jul 22 '21 14:07 Punzo

we need to test, it looks like the images get a shift, so it is not a panning of the camera.

Steps to reproduce: go to https://dev-slim-viewer.canceridc.dev/studies/1.3.6.1.4.1.5962.99.1.2440258434.818002713.1625937929090.3.0/series/1.3.6.1.4.1.5962.99.1.2440258434.818002713.1625937929090.2.0

zoom at the maximum level and just wait. When the high resolution tiles get loaded, there is a visible shift, between the low resolution and the high resolution images.

Punzo avatar Jul 22 '21 14:07 Punzo

@hackermd maybe a bug in static method deriveImageGeometry? https://github.com/MGHComputationalPathology/dicom-microscopy-viewer/blob/3b6b5afaae419e2f006e90fba297ad9612de6a8a/src/channel.js#L451

Punzo avatar Jul 22 '21 14:07 Punzo

I perfomed further testing on this. In the following video we can see that the "image shift" occurs only when the tile do a transition between "preview" (i.e. when is downloading) to fully loaded:

https://www.dropbox.com/s/m9ccicz5tstnr1x/2021-07-26%2015-29-26.mp4?dl=0

Therefore the fully downloaded images position is right in world coordinates (in the video you can focus on the annotation and see that the relative distances are maintained when zooming).

I agree seeing this shift of the images during the download transition is annoying, but it looks like it is not related to the grid parametes defined in deriveImageGeometry method (e.g. the offset: https://github.com/MGHComputationalPathology/dicom-microscopy-viewer/blob/3b6b5afaae419e2f006e90fba297ad9612de6a8a/src/channel.js#L589), but it is internal in OpenLayer.

@pieper @fedorov @hackermd how much priority has this issue? Should I try to further debug/investigate this issue in OpenLayer (i.e. trying to understand what happens to the tile world coordinates during the download)?

Punzo avatar Jul 26 '21 13:07 Punzo

Thanks for investigating further @Punzo. We'll need to discuss what priority this has. So far it's been stated by @dclunie that this wouldn't be acceptable for clinical pathology review systems. But IDC may be okay with a viewer that is "good enough" to see the images. At the same time we don't want people to think that the data itself is somehow wrong or that the dicom conversion of the data has introduced issues. It also depends on how much effort fixing it is likely to require.

pieper avatar Jul 26 '21 13:07 pieper

Thanks for investigating further @Punzo. We'll need to discuss what priority this has. So far it's been stated by @dclunie that this wouldn't be acceptable for clinical pathology review systems. But IDC may be okay with a viewer that is "good enough" to see the images. At the same time we don't want people to think that the data itself is somehow wrong or that the dicom conversion of the data has introduced issues. It also depends on how much effort fixing it is likely to require.

ok then for the moment I will stop working on it until we have the occasion to discuss about it.

Punzo avatar Jul 26 '21 13:07 Punzo

Would also be good to hear from @ahomeyer about how important this issue is for the researchers.

fedorov avatar Jul 26 '21 14:07 fedorov

don't stop working on this ... there is clearly a problem that needs to be fixed, regardless of the user ... it's not annoying, it's unprofessional

dclunie avatar Jul 26 '21 15:07 dclunie

don't stop working on this ... there is clearly a problem that needs to be fixed, regardless of the user ... it's not annoying, it's unprofessional

we will definitely come around with a proper fix. However, as any bug involving third party libraries, it may take time and resources. It is @pieper @fedorov call to assign the level of priority for the issues and allocate resources. I will look forward to discuss it in the a videocall at the OHIF-IDC prioritization meetings (as I largely anticipated I will be in vacation in August, so for me it would be in September).

Punzo avatar Jul 26 '21 15:07 Punzo

I cannot access the dev-slim-viewer.canceridc.dev link above but took a look at the video.

We had a similar issue in the past with Hamamatsu NDPI files. It turned out, that higher levels in the image pyramid were shifted to some small degree and did not correspond to proper downsampled versions of the base layer. We had to solve this problem by resampling the entire image pyramid from the base layer.

For research applications, I don't find this shift too disruptive. Also, for the pathomics use case, we only consider full resolution base level images. It should be ensured, however, that there is no fundamental problem with image conversion/loading.

ahomeyer avatar Jul 26 '21 15:07 ahomeyer

Based on the detailed assessment in https://github.com/openlayers/openlayers/issues/12768, this issue appears to be caused by the tiles rather than a bug in the viewer or the underlying libraries.

hackermd avatar Sep 19 '21 13:09 hackermd

I don't see how you reach that conclusion - if the issue is onyl when upsampled proxy tiles are showing, how can this be blamed on the original tiles?

Bottom line is the application behavior that the user experiences is wrong and needs to be fixed, regardless of whether it is the application code or the library it depends on, so closing this doesn't solve the user's problem.

dclunie avatar Sep 19 '21 15:09 dclunie

Bottom line is the application behavior that the user experiences is wrong and needs to be fixed, regardless of whether it is the application code or the library it depends on.

What should we (or the Openlayers developers) do about it in your opinion?

We could turn off up-sampling and not show any interim tiles upon zooming, but that would also not be a desirable behavior in general.

We haven't seen this application behavior with data generated by other acquisition devices, have we? The viewer application ultimately has to make assumptions about characteristics of the data that are not well defined by the standard. As far as I am aware, the standard does not specify how DERIVED images should be derived from ORIGINAL images and the entire image pyramid is only implicitly defined.

We could describe the derivation process via the Derivation Code Sequence attribute, add relevant values to context group CID 7203 "Image Derivation"](http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_7203.html), and dynamically adjust application behavior accordingly. However, the attribute is type 3 and therefore also not guaranteed to be present.

hackermd avatar Sep 19 '21 16:09 hackermd

Upsample correctly.

David

On 9/19/21 1:00 PM, Markus D. Herrmann wrote:

Bottom line is the application behavior that the user experiences is wrong and needs to be fixed, regardless of whether it is the application code or the library it depends on.

What should we (or the Openlayers developers) do about it in your opinion?

We could turn off up-sampling and not show any interim tiles upon zooming, but that would also not be a desirable behavior in general.

We haven't seen this application behavior with data generated by other acquisition devices, have we? The viewer application ultimately has to make assumptions about characteristics of the data that are not well defined by the standard. As far as I am aware, the standard does not specify how DERIVED images should be derived from ORIGINAL images and the entire image pyramid is only implicitly defined.

We could describe the derivation process via the Derivation Code Sequence http://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_C.12.4.html#para_9ebc2c04-47fb-491c-b9fd-cec5a5017318 attribute, add relevant values to context group CID 7203 "Image Derivation"](http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_7203.html http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_7203.html), and dynamically adjust application behavior accordingly. However, the attribute is type 3 and therefore also not guaranteed to be present.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MGHComputationalPathology/slim/issues/39#issuecomment-922504715, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBLYVM5CQAF75QDMWZFWBLUCYJJDANCNFSM5A2CJZ6A. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

dclunie avatar Sep 19 '21 17:09 dclunie

So summing up OpenLayers dev told us:

  1. the lower resolution tiles are used on higher resolution levels only during the donwload and they are upsampled.
  2. to upsample they use at least a bilinear interpolation (right?)

My question for you guys @dclunie @hackermd : are some levels of the pyramid generated by resampling? if there are resampled images, which interpolation do you used?

because the OpenLayers devs are saying this bug would happen only if the images are interpolated with nearest neighbor. Let's say if we have images interpolated with bilinear or bicubic, then we can move forward with testing (and see if the problem is still there) and in finding a solution (both for the images interpolated using nearest neighbor and bilinear, etc...).

Punzo avatar Sep 20 '21 07:09 Punzo

Can it really be a problem with the tiles, base layer or downsampled, if the supplied tiles (not the OpenLayers upsampled ones) display correctly (in the correct locations)?

Since the supplied tiles do display correctly, how can the vendor's means of interpolation to produce the downsampled tiles matter, since they are demonstrably "correct" in the sense of not producing any displacement (wrt. however you are describing their position in the coordinate space to OpenLayers)?

I suggested on the other thread checking the images on the NEMA ftp site, from Leica and from other vendors, which may have lower pyramid layers down-sampled using different techniques.

You can also create downsampled pyramids yourself, using a very crude downsampling mechanism, averaging 4 pixels from adjacent tiles to make 1 with no pre-filtering, with the com.pixelmed.apps.TiledPyramid utility.

I wrote the tool to make pyramids from Philips J2K base layer images (since they don't supply a pyramid); output from this tool is already available on the NEMA server for the Philips Connectathon test images (some are pretty old and may have various header issues though, the more recent ones are better but still do not describe the ImageType as resampled when they should). Try Philips^Amy in WG26Demo2020_PV as a relatively recent example.

I don't know what downsampling (and pre-filtering, if any) methods the different vendors use. We could try asking them on the WG 26 mailing list, but I am not sure if they would respond.

David

On 9/20/21 3:43 AM, Davide Punzo wrote:

So summing up OpenLayers dev told us:

  1. the lower resolution tiles are used on higher resolution levels only during the donbwload and they are upsampled.
  2. to upsample they use at least a bilinear interpolation (right?)

My question for you guys @dclunie https://github.com/dclunie @hackermd https://github.com/hackermd : are some levels of the pyramid generated by resampling? if there are resampled images, which interpolation do you used?

because the OpenLayers devs are saying this bug would happen only if the images are interpolated with nearest neighbor. Let's say if we have images interpolated with bilinear or bicubic, then we can move forward with testing (and see if it a problem of the tiles or of OpenLayers) and in finding a solution.

dclunie avatar Sep 20 '21 11:09 dclunie

Can it really be a problem with the tiles, base layer or downsampled, if the supplied tiles (not the OpenLayers upsampled ones) display correctly (in the correct locations)? Since the supplied tiles do display correctly, how can the vendor's means of interpolation to produce the downsampled tiles matter, since they are demonstrably "correct" in the sense of not producing any displacement (wrt. however you are describing their position in the coordinate space to OpenLayers)?

Also for me it is quite strange (in fact I tought it was simply an issue in the computation of the center of the tiles during the donwloading transition of the tile of higher resolution) and I am still really puzzled by it.

For example I see the same bug with GeoTiff data https://openlayers.org/en/latest/examples/cog-overviews.html . So either:

  1. the GeoTiff data are sampled with nearest neighbor too;
  2. in the other examples (e.g. maps data) the bug is just not noticable because the data are donwloaded very fast and the effect is not visible.

However, I also think we should check if bilinear resampled images do not present the issue as the OpenLayer dev told us (i.e. be scientific about this). On the other hand, in parallel I also think we should look what is going on during the upsampling in OpenLayers and it may give us more insights.

I suggested on the other thread checking the images on the NEMA ftp site, from Leica and from other vendors, which may have lower pyramid layers down-sampled using different techniques. You can also create downsampled pyramids yourself, using a very crude downsampling mechanism, averaging 4 pixels from adjacent tiles to make 1 with no pre-filtering, with the com.pixelmed.apps.TiledPyramid utility. I wrote the tool to make pyramids from Philips J2K base layer images (since they don't supply a pyramid); output from this tool is already available on the NEMA server for the Philips Connectathon test images (some are pretty old and may have various header issues though, the more recent ones are better but still do not describe the ImageType as resampled when they should). Try Philips^Amy in WG26Demo2020_PV as a relatively recent example. I don't know what downsampling (and pre-filtering, if any) methods the different vendors use. We could try asking them on the WG 26 mailing list, but I am not sure if they would respond.

ok, thanks for the info.

Punzo avatar Sep 20 '21 12:09 Punzo

FYI, this is a nice summary of resampling methods and issues:

Parker JA, Kenyon RV, Troxel DE. Comparison of Interpolating Methods for Image Resampling. IEEE Transactions on Medical Imaging. 1983 Mar;2(1):31–9. Available from: http://www.cs.uic.edu/~kenyon/Papers/Comparison.of.Interpolating.Methods.Parker.Kenyon.Troxel.pdf

And this is an interesting paper on the effects of mismatched downsampling and upsampling (not sure if it is relevant, but it is interesting):

Frajka T, Zegar K. Downsampling dependent upsampling of images. Signal Process Image Commun. 2004;19. Available from: http://code.ucsd.edu/~zeger/publications/journals/FrZe04-SPIC-Resize/FrZe04-SPIC-Resize.pdf

dclunie avatar Sep 20 '21 13:09 dclunie

I created a freshly downsampled pyramid of the slide that is on the IDC portal front page, using an updated version of the com.pixelmed.apps.TiledPyramid tool, for you to try to see if it manifests the OpenLayers upsampling problem, or not.

The downsampling is by a factor of 2 and is created by averaging four adjacent pixels to produce one, which I understand to be bi-linear interpolation.

See:

https://www.dropbox.com/s/3sr6a2pzka55kkb/CPTAC-LSCC_C3L-00965-26.zip

dclunie avatar Sep 20 '21 17:09 dclunie

@dclunie did you try these with the isomics.dev slim installation?

pieper avatar Sep 20 '21 18:09 pieper

No, I don't have the images in a Google DICOMStore.

dclunie avatar Sep 20 '21 19:09 dclunie

Do you want me to put them in one? Is there an existing one that would be appropriate or should we create a dedicated one?

pieper avatar Sep 20 '21 19:09 pieper

Reposting my email on this subject, since it should be attached to this issue ...

I am still not convinced that the problem is with the SVS tiles, and that Slim and/or OpenLayers should not be doing better.

The attached animated GIF illustrates, using the usual "old" SVS data from the default image on the portal home page:

  • a section of 4 by 4 tiles from the middle of CPTAC-LSCC C3L-00965-26 base layer
  • the same area from the 4x SVS downsampled image
    • upsampled 4x with netpbm pamscale using its default options
    • upsampled 4x with netpbm pamscale using -filter sinc

and then converted into an Animated GIF in that order.

Note that there does not seem to be any significant "movement" of the visible structures in any consistent direction, unlike the very noticeable shift one sees when viewing in Slim.

Since the shift in Slim always seems to be in a specific direction, not necessarily towards the highest intensity pixels, I still wonder if this is not something to do with coordinate system conversions and assumptions, but am surprised this would not also affect the final full resolution tile display.

merged_withupsampled_layers

dclunie avatar Sep 24 '21 14:09 dclunie

Thanks @dclunie for putting this animation together!

Note that there does not seem to be any significant "movement" of the visible structures in any consistent direction, unlike the very noticeable shift one sees when viewing in Slim.

I have the impression that is still "movement". Have you also tried the opposite order and without the third option?

  1. the same area from the 4x SVS downsampled image (upsampled 4x with netpbm pamscale using its default options)
  2. a section of 4 by 4 tiles from the middle of CPTAC-LSCC C3L-00965-26 base layer

This would more realistically simulate the behavior of Slim when a user zooms in.

hackermd avatar Sep 27 '21 11:09 hackermd

There is certainly slight movement of some structures, but my impression is that it is not nearly as severe as the dramatic shift that one sees in Slim, and not consistently in the same direction.

Here is the same patch as a 2-frame "flicker" test (base layer and pamscale default upsample, 0.1 sec between frames):

merged_withupsampled_layers_2frame_flicker

And here with the sinc filter instead of the default:

merged_withupsampled_layers_2framesinc_flicker

And, for comparison, here it is using my own downsampling (vide supra) rather than the SVS supplied downsampling, with the pamscale default upsampling; this looks pretty much exactly the same to me as the SVS-based result:

merged_withupsampled_layers_2framedefault_flicker

If you right-click on each of these and open them in a new browser tab, +/- zooming in on each, you can easily switch between them for comparison.

dclunie avatar Sep 27 '21 13:09 dclunie

Thanks @dclunie! That's super helpful!

There is certainly slight movement of some structures, but my impression is that it is not nearly as severe as the dramatic shift that one sees in Slim, and not consistently in the same direction.

Hmm...to me, the magnitude of the shift looks comparable. The direction indeed appears to be less consistent, but I am wondering whether this effect may just be perceived differently visually when many tiles trickle in on the OpenLayers tile grid with small delays.

Given @ahocevar's description of the internal workings of OpenLayers, the only other explanation I can come up with is that OpenLayers computes the offset or dimension of interim tiles incorrectly for the custom tile grid. However, @ahocevar seemed to be highly convinced that this is not the case.

hackermd avatar Sep 27 '21 14:09 hackermd

@hackermd To verify the tile offsets and dimension, it might be useful to add another tile layer with a TileDebug source, configured with the same tileGrid and zDirection, to the map. Then you can see the tile borders and tile coordinates.

ahocevar avatar Sep 28 '21 09:09 ahocevar

To verify the tile offsets and dimension, it might be useful to add another tile layer with a TileDebug source, configured with the same tileGrid and zDirection, to the map. Then you can see the tile borders and tile coordinates.

That's a very good idea. I temporarily added a TileDebug source at some point for debugging, but did not include it as a feature of the dicom-microscopy-viewer library.

hackermd avatar Sep 28 '21 09:09 hackermd

@hackermd @Punzo there is an update on this issue at https://github.com/openlayers/openlayers/issues/12768#issuecomment-1030463105

dclunie avatar Feb 05 '22 11:02 dclunie

@dclunie @hackermd this should fix the issue https://github.com/MGHComputationalPathology/dicom-microscopy-viewer/pull/73. It looks like OpenLayer does not accept "zoom" factors (resolutions) in the Grid which are not integer (which makes 100% sense). I will post https://github.com/openlayers/openlayers/issues/12768, they may provide further explanations.

See video: https://www.dropbox.com/s/haugu2uoifea9ol/2022-02-07%2014-44-26.mp4?dl=0

Punzo avatar Feb 07 '22 14:02 Punzo

OpenLayers does work fine with fractional resolutions, but there are other constraints you should keep in mind. See my explanations in https://github.com/openlayers/openlayers/issues/12768#issuecomment-1031652135.

It looks like someone involved in developing the Dicom Viewer was aware of the underlying problem, because I found this TODO:

https://github.com/MGHComputationalPathology/dicom-microscopy-viewer/blob/master/src/channel.js#L616-L618

ahocevar avatar Feb 07 '22 16:02 ahocevar