Always marshal tile metadata on presence of pyramid

[chris-allan]

It is possible, in particular for floating point data, that a pyramid is
present even if the server thinks a pyramid is not required. The
condition for tile metadata being available is now pyramid presence.

See:

• Presence of existing pyramids for floating point data masked omero-romio#24

/cc @kkoz, @melissalinkert, @knabar

[will-moore]

Looks good.

[sbesson]

Tested using our sample multi-resolution OME-TIFF derived from an EM-based floating-point image:

Prior to this PR

{
    "id": 10865,
    "meta": {
        "imageName": "BGal_000438_frames.ome.tiff",
        "imageDescription": "",
        "imageAuthor": "xxxx xxx",
        "projectName": "Multiple",
        "projectId": null,
        "projectDescription": "",
        "datasetName": "Multiple",
        "datasetId": null,
        "datasetDescription": "",
        "wellSampleId": "",       "wellId": "",
        "imageTimestamp": 1595837815.0,
        "imageId": 10865,
        "pixelsType": "float"
    },
    "perms": {
        "canAnnotate": true,
        "canEdit": true,
        "canDelete": true,
        "canLink": true
    },
    "tiles": false,
    "interpolate": true,
    "size": {
        "width": 7676,
        "height": 7420,
        "z": 38,
        "t": 1,
        "c": 1
    },
    "pixel_size": {
        "x": 6.375423073768616e-05,
        "y": 6.375423073768616e-05,
        "z": 1.0
    },
    "init_zoom": 0,
    "nominalMagnification": 105000.0,
    "pixel_range": [
        -2147483648,
        2147483647
    ],
    "channels": [
        {
            "emissionWave": null,
            "label": "0",
            "color": "808080",
            "inverted": false,
            "reverseIntensity": false,
            "family": "linear",
            "coefficient": 1.0,
            "window": {
                "min": -2147483648.0,
                "max": 2147483647.0,
                "start": 0.0,
                "end": 7.636034965515137
            },
            "active": true
        }
    ],
    "split_channel": {
        "g": {
            "width": 7680,
            "height": 7424,
            "border": 2,
            "gridx": 1,
            "gridy": 1
        },
        "c": {
            "width": 15358,
            "height": 7424,
            "border": 2,
            "gridx": 2,
            "gridy": 1
        }
    },
    "rdefs": {
        "model": "greyscale",
        "projection": "normal",
        "defaultZ": 19,
        "defaultT": 0,
        "invertAxis": false
    }
}

After this PR, the tile_size and levels values are populated as expected.

{
    "id": 136705,
    "meta": {
        "imageName": "BGal_000438_frames.ome.tiff",
        "imageDescription": "",
        "imageAuthor": "xxx xxx",
        "projectName": "Multiple",
        "projectId": null,
        "projectDescription": "",
        "datasetName": "Multiple",
        "datasetId": null,
        "datasetDescription": "",
        "wellSampleId": "",
        "wellId": "",
        "imageTimestamp": 1595837581.0,
        "imageId": 136705,
        "pixelsType": "float"
    },
    "perms": {
        "canAnnotate": true,
        "canEdit": true,
        "canDelete": true,
        "canLink": true
    },
    "tiles": true,
    "tile_size": {
        "width": 1024,
        "height": 1024
    },
    "levels": 5,
    "interpolate": true,
    "size": {
        "width": 7676,
        "height": 7420,
        "z": 38,
        "t": 1,
        "c": 1
    },
    "pixel_size": {
        "x": 6.375423073768616e-05,
        "y": 6.375423073768616e-05,
        "z": 1.0
    },
    "init_zoom": 0,
    "nominalMagnification": 105000.0,
    "pixel_range": [
        -2147483648,
        2147483647
    ],
    "channels": [
        {
            "emissionWave": null,
            "label": "0",
            "color": "808080",
            "inverted": false,
            "reverseIntensity": false,
            "family": "linear",
            "coefficient": 1.0,
            "window": {
                "min": -2147483648.0,
                "max": 2147483647.0,
                "start": 0.0,
                "end": 7.636034965515137
            },
            "active": true
        }
    ],
    "split_channel": {
        "g": {
            "width": 7680,
            "height": 7424,
            "border": 2,
            "gridx": 1,
            "gridy": 1
        },
        "c": {
            "width": 15358,
            "height": 7424,
            "border": 2,
            "gridx": 2,
            "gridy": 1
        }
    },
    "rdefs": {
        "model": "greyscale",
        "projection": "normal",
        "defaultZ": 19,
        "defaultT": 0,
        "invertAxis": false
    }
}

In terms of the marshalling, the change proposed here make complete sense. As discussed in ome/omero-romio#24, the current implementation of RenderingService.requiresPixelsPyramid returns whether the server should attempt to create a pyramid but does not authoritatively reflect whether the image contains pyramidal data especially in the case of floating point data.

While looking at the rendered data using OMERO.iviewer with and without this PR in our CI servers, the data access feel much slower with this change included than without. Roughly each tile seems to render 5 times slower. I will take a look at the server logs as well as compare with other pyramidal images to confirm whether this issue is caused by this change or might be the consequence of other unrelated open changes.

A few additional comments:

• while re-reading the logic, I generally found this code introduces some tight coupling and confusion between the concepts of tiled data and sub-resolutions although these are technically independent of each other
• Beyond the scope of this PR, would we foresee any issue with marshalling tile_size and the levels for every image as this metadata always exists for every image? Or should we assume downstream clients check the existence of such keys to switch behaviors?
• as shown by this PR, the current semantics of tiles: True really means multiresolutions: True (or similar) and we might want to look into deprecating this key in a future
• we might want to review the places in our code where this API is used (https://github.com/search?p=1&q=org%3Aome+requiresPixelsPyramid&type=Code) and update if necessary

[chris-allan]

@sbesson: The floating point renderer is quite slow if memory serves so you might just be seeing the realities of accepting that performance hit once rather than multiple times.

There is certainly scope beyond the limited change to fundamentally adjust the way clients are informed as to the nature of the underlying pixel data. Such clients can then make informed decisions on how they want to approach visualization. We already do this to a large extent with the image-region microservice and PathViewer.

[sbesson]

Retested after excluding ome/omero-iviewer#316 which was the source of the issue described in #193 (comment).

Performance is now comparable with and without this PR while accessing the various resolutions and Z-planes of the pyramidal floating-point image. As discussed above, the metadata addition makes sense and might be something worth generalizing to all images.

[chris-allan]

@sbesson: 👍

Do we have a sense of when a release would be made including the change?

[sbesson]

As far as I am concerned, this release can happen immediately. Thoughts about the version increment? Would you consider this as a bug fix?

[chris-allan]

I do consider it a bugfix but it is definitely changing the behaviour of an existing and established method so I can see it both ways.

[sbesson]

👍 now released as https://pypi.org/project/omero-web/5.7.1/

[chris-allan]

@knabar: Pointed out that zoomLevelScaling is missing. iviewer must be smart enough to do the calculation itself, or not be relying on it, if this metadata is missing.

Might be worth fixing ImageWrapper.getZoomLevelScaling() as well:

• https://github.com/ome/omero-py/blob/master/src/omero/gateway/__init__.py#L8714

[chris-allan]

Restoration of zoomLevelScaling should now happen with ome/omero-py#239. Will need to update the version requirement here and bump in order to pick up all the changes unfortunately.