[REVIEW] Resource factory, example app, and z-order

[nein09]

Moved over from https://github.com/sul-dlss/iiifManifestLayouts/pull/105

Contains the fixes for #39, #40, and #98.

[iangilman]

So far so good! :)

[nein09]

@aeschylus I'm loving these test manifests by the way- they're really helping me see whether I have everything right or not. Clipping and bounds aren't playing well together right now in image 37; once I address that, the next step is to add the ability to shuffle z-order around.

[iangilman]

Just checked out your changes. Code looks good to me.

[nein09]

Things I'm still missing:

• a non-naive implementation of dynamic vs. static for the imageResource
• making clipping work properly for image 37
• Making the z-order sortable in the example app

[nein09]

@aeschylus @iangilman Okay! It's massive, and I think it's ready- aside from the dynamic vs. static detail, which might be better to address in a follow-on PR.

What else am I missing in here? I'm sure there's something.

Also, note that I added some of jquery-ui for the sortable UI components- that was just the easiest way to do it.

[iangilman]

It's up to @aeschylus of course, but I figure adding jQuery UI is good...keeps the example code simple (and it's only used in the example, of course).

Generally the code looks good to me. The big issue I'm seeing is that you're tying the ImageResource OSD index directly to its position in the array in its CanvasObject. This will work fine if there is only one CanvasObject, but as soon as there are two, you'll run into conflicts.

In the OSD World, an image's index is just its position in the World's image array, so no two images can have the same index. If you have one CanvasObject with three ImageResources and another with three images, the first three can be 0, 1, 2, and the second three will be 3, 4, 5.

That said, I suppose CanvasObjects will never overlap each other, right @aeschylus? So we don't necessarily need to keep track of the z-index for CanvasObjects...we just need to make sure that their ImageResources are in order. This could be accomplished by always bringing all of a CanvasObject's ImageResources to the top of the stack whenever you need to reorder them.

Btw, on a more general code organization note, I think it makes sense for all of the image ordering code to be in CanvasObject, since it's the owner of the images array. When needed it can ask the ImageResource to update its OSD index.

[nein09]

Hmm! I had noticed the World image order behaving like that, but I hadn't thought about the ramifications. The way that we have the example app set up, it doesn't matter so much.

Thanks for this! It's helpful.

[nein09]

Well, I actually put all the resources at the bottom of the stack before manipulating them, but that means that their indices in the world match their indices in the canvas during that time.

@aeschylus @iangilman Let me know if you have any other feedback for me - I'll be out until Monday :christmas_tree: :santa: though.

[nein09]

I felt ambitious, so I implemented the dynamic vs. static distinction as described in #98. :gift:

[nein09]

This still takes a preposterous amount of memory when working with http://manifests.ydc2.yale.edu/manifest/Osbornfa1v2.json - after I selected canvas 53r, which has all the alternates, my memory usage climbed to 7.5 GB in Firefox.

[iangilman]

The code is looking good to me.

With the memory issue, how many OSD TiledImages do you have stacked on top of each other? OSD is good about loading only the tiles it needs with one exception: it doesn't know if one TiledImage is occluding another, so if you stack a hundred of them on top of each other, it's going to load all of the tiles for each as if they were the top image.

Furthermore, looking at the OSD code, it appears we still draw the image even if it has opacity 0, which means it's still loading the tiles for it. Seems like that's probably worth fixing in OSD, though we don't expect another release soon, so it'll be a while before it would be up on npm. Meanwhile, we could manage the issue by removing images from the world when not needed (if that is in fact the issue).

[nein09]

Upon further investigation, I see the memory issue sooner than that. It happens when I switch to overview mode in order to find canvas 53r and select it.

There are 401 canvases in that manifest, and most of them seem to have an image service rather than a static thumbnail. So when I switch into overview mode, I have 401 TiledImages there, and the ones that are visible are all wanting to load themselves.

[iangilman]

Hmm...doesn't seem like that should require that much memory...there should be less than 100 images visible, and each one of them should only need a relatively small tile.

At any rate, maybe we should investigate that in a separate issue and merge this patch in. What do you think, @aeschylus?

[aeschylus]

I'm merging this because it restores most of the functionality we need and adds the layering. We can proceed from here.

As for the tiles, I think we should not load all tiles when we zoom in, only the "main" one(s). So no alternates, for instance. I don't think that will be enough to fix the performance, though. I'll do a top-to-bottom read-through today and see if anything sticks out at me.

[nein09]

@aeschylus Right now we don't load alternates until they're requested (i.e. when someone selects a checkbox for them). You can test this by moving unloaded alternates higher in the z-order - you won't see them until you ask the app to load them.

Could the memory problems be from instantiating a whole load of tileSource objects? If they were static images that we were creating pyramids for on the fly, I'd be a lot less surprised. Or perhaps the image service isn't being used, when it should be, and they're doing that anyway.

[iangilman]

Hmm...we're using ImageTileSource now for our place holders, right? It occurs to me that maybe that tile source doesn't properly lazy load, so it'll actually load all 400 images at once even though they're not on the screen. @avandecreme can you confirm?

Of course it'll make a big difference how big the placeholder images are... @nein09 what are their dimensions?

[nein09]

Okay, I just refreshed my memory about how all this works.

Thumbnails use ImageTileSource even in the presence of an image service. Right now the widths of the placeholders are a quarter of what the full size is- meaning that for the manifest I link to above, the thumbnails will all be around 950 pixels wide (and roughly 1000 pixels tall).

I made the thumbnails quarter-size because I was running into memory problems before, and that helped quite a lot- but I'm not sure how small we want to go for them, either.

[avandecreme]

@iangilman Yeah I am pretty sure it is not lazy loaded. I am not sure what this project does but if it displays a big grid of images I guess you would also need an "unloading" feature to get the images no longer displayed GC.

[iangilman]

Ideally ImageTileSource would lazy load and also would unload with the usual OSD tile purge. I guess the challenge with lazy loading is you don't know the dimensions until you load the image. Anyway, I'll start a thread on that over in OSD.

Meanwhile, @nein09, maybe we should switch back to LegacyTileSource, which does lazy load and auto purge.

[iangilman]

I've started a ticket here for memory on this project:

https://github.com/sul-dlss/iiifManifestLayouts/issues/111

...and here is the OSD issue:

https://github.com/openseadragon/openseadragon/issues/813

[nein09]

Here's another thing that I did in the ThumbnailFactory that's relevant - If there is no thumbnail and only one image in this canvas, there's no reason to make a thumbnail for it- the canvasobject will fall back to opening the main tilesource in the absence of a thumbnail.

In the case of the Osborn example, this happens for 401 of the canvases. So even if I switch the thumbnails to using LegacyTileSource, it doesn't help, because they're all just opening the main tilesource all the time!

Should we reconsider that optimization?

[nein09]

Of course, if I don't open the info.json files all at once, this won't be nearly as much of a problem, so I think I'll try that first.

[iangilman]

Yes, we definitely don't want to be loading the info.json for all of the images; that's 400 server requests we want to avoid. Do we have some way to figure out some sort of thumbnail for them?

[nein09]

We're requesting info.json because we're using the image service for all of them- I could always use the resource id to make thumbnails, instead of looking at the service at all. I've tried that here: https://github.com/sul-dlss/iiifManifestLayouts/pull/112

[iangilman]

Awesome :)