mirador icon indicating copy to clipboard operation
mirador copied to clipboard

Mirador fails to load Manifests with layers

Open regisrob opened this issue 1 year ago • 6 comments

Testing the mui5-react-18 branch, I notice that Manifests with layers make Mirador crash.

It fails with all the Manifests in the layers.html demo, for instance this one: https://dvp.prtd.app/hamilton/manifest.json

This Manifest in the default index.html demo also fails with same error. To reproduce the bug, open the Biblissima Manifest Manuscrit reconstitué : Châteauroux, Bibliothèque municipale, ms. 5 (Grandes Chroniques de France), open the Index panel, click on the first entry in the ToC ("f. 34r...").

In both cases it raises this JS error:

Uncaught (in promise) Error: Index bigger than number of layers.
    setItemIndex webpack-internal:///./node_modules/openseadragon/build/openseadragon/openseadragon.js:8452
    refreshTileProperties webpack-internal:///./src/components/OpenSeadragonViewer.js:43
    refreshTileProperties webpack-internal:///./src/components/OpenSeadragonViewer.js:43
    addAllImageSources webpack-internal:///./src/components/OpenSeadragonViewer.js:41
world.js:153:18

regisrob avatar Feb 09 '24 18:02 regisrob

I believe I stumbled upon this before but I failed to create an issue, so thank you for doing that! As far as I remember, this is not an issue that was introduced with the MUI 5 or React 18 changes.

I can't reproduce the issue every time consistently; it seems to be caused by race conditions. In certain circumstances, the _items in the OSD viewer object do only contain one item (or at least not one item for each Layer), while the CanvasWorld uses all available image resources of the canvas to calculate the new index:

https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/lib/CanvasWorld.js#L166

https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/lib/CanvasWorld.js#L186

https://github.com/openseadragon/openseadragon/blob/59645e3a0dbc321be0f503df4e6fb4b156c3fbba/src/world.js#L152-L154

lutzhelm avatar Feb 14 '24 14:02 lutzhelm

I think I have found two possible sources for race conditions. The first one has a lot more impact because it depends on network connections.

Image info responses that are added to viewer vs. number of image resources on the canvas according to the manifest

The props that are provided to the OpenSeadragonViewer component do only contain image infoResponses that have already been successfully fetched.

https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/containers/OpenSeadragonViewer.js#L34-L37

On the other hand, the index that is calculated for the OpenSedragonViewer in CanvasWorld.getLayerMetadata() https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/lib/CanvasWorld.js#L157-L158 uses the images resources as they are found in the manifest: https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/lib/MiradorCanvas.js#L68-L82

I'm not sure what a quick fix would look like.

setTimeout() in OSD code

I believe there is another possible source for race conditions in how OSD itself handles the addition of tilesources to the world object. While the viewer component waits for all Promises before it calls the function where the error manifests,

https://github.com/ProjectMirador/mirador/blob/593e81be91f0e9ccee5406235464ea65cb630ec3/src/components/OpenSeadragonViewer.js#L188-L191

OSD's world.addItem() is called via setTimeout():

https://github.com/openseadragon/openseadragon/blob/59645e3a0dbc321be0f503df4e6fb4b156c3fbba/src/viewer.js#L2605

In this setTimeout(), successCallback() is either called directly or via waitUntilReady(); successCallback for getTileSourceImplementation() in viewer.addTiledImage() calls processReadyItems() where world.addItem() is called.

https://github.com/openseadragon/openseadragon/blob/59645e3a0dbc321be0f503df4e6fb4b156c3fbba/src/viewer.js#L1719-L1725

I'm not sure if the resolution of all promises is sufficient or if there might still be asynchronous code running to add those items.

Both issues seem neither easy to debug nor to resolve.

lutzhelm avatar Feb 15 '24 12:02 lutzhelm

Hi @regisrob 👋 based on the findings @lutzhelm has shared above, it appears that the issue you identified has been present for some time, and deeply-seated at that, from before the mui5-react-18 branch was created.

I'm going to update the title of this issue, "Mirador fails to load Manifests with layers on mui5-react-18 branch," to remove the name of the branch specifically since it is not related to it, but also keep it open in the hope that progress can be made.

Apart from this, did you find any other bugs in the mui-react-18 branch?

enriquediaz avatar Mar 03 '24 21:03 enriquediaz

👋 @enriquediaz thanks for that, and thanks @lutzhelm for the investigation on this bug (which I never noticed before with previous versions of Mirador 3). This one looks a bit tricky...

I did not find any other bugs in the mui-react-18 branch so far.

regisrob avatar Mar 11 '24 22:03 regisrob

openseadragon/openseadragon#2457 seems to be related, but I'd assume that the cause is rather in Mirador than in OpenSeadragon.

lutzhelm avatar Apr 04 '24 16:04 lutzhelm

That's my issue. It is caused by Mirador. I have had this error in the console for several years in Mirador 3. I previously thought it was an OSD issue (mistakenly).

BeebBenjamin avatar Jun 17 '24 08:06 BeebBenjamin