Cache Overhaul for OpenSeadragon (reviewed).

Second attempt to redesign OSD cache. Follows up on https://github.com/openseadragon/openseadragon/pull/2329.
Areas to discuss are marked with FIXME comment - please, let's discuss these issues.
Why do we need this?
Extending abilities of OpenSeadragon require more flexible system to work with data. This overhaul fixes many issues, bugs and API inconsistencies.
What is solved

inconsitent TileSource data management: newly, tiles can own multiple caches, multiple plugins can affect tile data - tile sources should only govern how the data is retrieved, not managed - depreacated tile cache life cycle handling methods
automatic support for data type conversions: drawers in the future will require complex interactions between available sources and the system: out of the box, the system 'jsut works' (though it might not be as performant as one would wish, but there is API to make this as performant as possible - depends on the developer effort)
deprecated getContext2D, destroy for TiledImage - hacky way for imagetilesource to work in older versions of OSD, the tilesource has been rewritten
deprecated tile.context2D - hacky way of rendering, storing evergrowing amount of data in the browser memory
fixed docs, syntax bugs and test behavior along the way

What needs to be solved (apart from FIXME)

[x] add tests (once the design is approved)
[ ] missing support for shared cache between viewers
[x] asynchronous data type conversion
[x] it is now impossible for the tile not to use cache, do we want this feature?

IMHO, most developers incorrectly went with context2D which was a performant, but memory-expensive choice, one should increase cache size instead of hardcoding data

[x] how much data is inside a cache object and whether to cache also conversion mid-results
[ ] thoroughly profile on a demo that cache does not leak (and all references are properly removed)
[ ] wait for the WebGL PR and integrate drawers

drawers must respect tile cache API as well - they must declare what type they can render, they must delay tile rendering when the cache is in a loading state

Follow up on https://github.com/openseadragon/openseadragon/pull/2329#issuecomment-1505948219:

Simple path: nothing to write - works out of the box

The 10% red overlay.

this.viewer.addHandler("tile-loaded", (event) => {
    //simple :)
    const context2D = event.tile.getData("context2d");
    const canvas = event.tile.getData("canvas"); //we could do it directly, but let's flex our API ;)
    // context2D... set color ... draw rect canvas width height ...
    event.tile.save(); //now just sets _needsDraw = true;
});

Each tile's "ultimate data" is a 50% merge of two separate JPGs loaded from server (suppose equal size).
then we can a):

downloadTileStart: function(imageJob) {
    let context = imageJob.userData;
    context.images = [new Image(), new Image()];

    let count = 0;
    for (let image of context.images) {
        image.onerror = image.onabort = function() {
            count = -5; //prevent onload
            imageJob.fail("Failed to parse tile data as an Image", context.promise);
        };
        image.onload = function() {
            if (count++ === 0) {
                const canvas = document.createElement('canvas');
                canvas.width = image.width;
                canvas.height = image.height;
                context.canvas = canvas.getContext('2d');
                context.drawImage(image, image.width, image.height);
                return;
            }
            const result = this.context.canvas;
            result.globalAlpha = 0.5;
            result.drawImage(image, image.width, image.height);

            delete context.canvas;
            imageJob.finish(result, context.promise, 'contex2d'); //recognized automatically, we could leave the type out
        };
    }

    function getImage(img, url) {
        //bit ugly since we keep just one...
        context.promise = fetch(url, {
            method: "GET",
            mode: 'cors',
            cache: 'no-cache',
            credentials: 'same-origin',
            headers: imageJob.ajaxHeaders || {},
            body: null
        }).then(blob => {
            img.src = URL.createObjectURL(blob);
        }).catch(e => {
            img.onerror(e);
        });
    }

    getImage(context.images[0], imageJob.firstUrl);
    getImage(context.images[1], imageJob.secondUrl);
}

or b) if we suppose the data was not sent out in an incompatible format:
... (see above)

        imageJob.fail("Failed to parse tile data as an Image", context.promise);
    };
    image.onload = function() {
        if (count++ === 0) return;

        //if we do not set type, the resulting type is: OpenSeadragon.DataTypeConvertor.guessType(context.images)
        //which will result in something like 'Array [image]' which is a bit too generic, since our type is
        // exactly two images,  returned by exactly this tilesource
        imageJob.finish(context.images, context.promise, "aiosa two images");
    };
}

...

OpenSeadragon.convertor.learn("aiosa two images", "context2D", data => {
    const canvas = document.createElement( 'canvas' ),
        image1 = data[0], image2 = data[2], context = canvas.getContext('2d');
    //context.drawImage ...etc
    return context;
});

or c):
this.viewer.addHandler("tile-loaded", (event) => {
    //incompatible data type within cache still valid - until the tile-loaded handler is over
    const data = event.tile.getData(); //"aiosa two images" is default, _unless_ some other plugin modified the data before us
    const canvas = document.createElement( 'canvas' ),
        image1 = data[0], image2 = data[2], context = canvas.getContext('2d');
    //context.drawImage ...etc

    // then we cannot actually call save since we need to update the cache! system does not know "aiosa two images" type
    event.tile.setData(context, 'context2d'); //or leave the type out, can be detected by the system
});

Generative data, e.g. Mandlebrot... How do the new changes affect your earlier example?

Simplified! Almost no changes, except:
createTileCache* functions are deprecated, no implementation needed since context2D data is returned, for example the function from the docs below will get implemented for us implicitly:
getTileCacheDataAsImage: function() {
     // not implementing all the features brings limitations to the
     // system, namely tile.getImage() will not work and also
     // html-based drawing approach will not work
     throw "Lazy to implement";
 },

downloadTileStart now distinguises finish and fail (fail does not happen with generated data so no change)

Tiles that change over time, like fading from original JPG to black and back again.

this.viewer.addHandler("tile-loaded", (event) => {
    //touching event.data directly is now not really a good idea, since we loose all the API.
    //we could set event.tile.myData = event.data and have 'dirty' access anytime, lets use cache instead (or, we would
    // repeat stuff covered in earlier examples)

    //let's work more closely with the cache api
    const tile = event.tile;
    //get the original data
    const cache = tile.getCache(tile.cacheKey);
    //we can now set the data directly to the cache, but it might not get freed by GC -> cache.destroy() is hardcoded
    // cache.myData = event.data;

    //instead, third option: use multiple caches per tile! note that we are 100% type safe (though not as optimal as
    // possible in corner cases, unless we allow getData to specify a range of possible types we accept - optimization,
    // see FIXME comment in the convertor)

    //switch to a new key: allow the original data to be used by somebody else...
    tile.originalCacheKey = tile.cacheKey;
    tile.cacheKey = `${tile.cacheKey}-opacity-change`;
    tile.setCache(tile.cacheKey, cache.getData("context2d")); //we will use this to render

    //if someone after us would use the cache, they can use the tile regardless of what we do here - they just get some
    //data to be rendered...
});

const tiledImage = ...; //our reference
setInterval(() => {
    tiledImage.lastDrawn.forEach(tile => {
        //we should be guaranteed to have cache loaded, as the cache is removed when a tile gets unloaded

        //internally, *-opacity-change is used as the default data
        const data = tile.getData("context2d");
        //we have two choices: we access the data as-is and have optimal time, or we require "image"
        // -> we could force the system into conversion, but we are guaranteed to get an image object
        const origData = tile.getCache(tile.originalCacheKey).getData("image");
        data.drawImage(origData, origData.width, origData.height);
        tile.save();
    });
}, 500);

Two plugins; one colors the tile 10% red, and the other inverts it. Make sure they work together and they are happening in the right order (how do we control that?).

I think it is obvious from the previous example, that we can modify the data regardless of what other plugins do, if we use
the API correctly. To control the order, OSD already supports event priority (see https://openseadragon.github.io/docs/OpenSeadragon.EventSource.html#addHandler) - my another PR.

I was thinking about the asynchronous data processing, and IMHO we would
transitively make everything async. Insead, we could adopt Maybe
data type, and allow users to process data in synchronous way, unless the data is unavailable.
Tile data access could opt for callback when the data is ready. Tile drawing
would inspect whether the tile data (wrapped as Maybe) is ready and

draw tile if data available
skip drawing and set _needsDraw=true (~ call tile.save())

Example:
//possibly provide helper methods to do the checks automatically, provide something like `Promise.all()`
viewer.addHandler('tile-loaded', (e) => {
  //here cache is used - raw API example, we could simplify this by wrapping the result of tile.getData(...)
  const cache = e.tile.getCache();
  if (cache.ready) {
    //we can work with the data
  } else {
    cache.onload = () => {
      //...
    };
  }
});

Advantage: flexible. Disadvantage: more complicated, more overhead.
For example, we need to provide helper functions for making sure
multiple data sources are ready at once, etc... possibly we could
behind the scenes wrap everything in Promises and execute everything
in the right order when data is available.
Another approach is to split the data operation into two phases: before and during tile-loaded
and afterwards. Data use on tile-loaded event could use async processing, we would
extend raise event to await promise handlers in the execution (I already
made PR containing the implementation), but afterwards, only
synchronous conversions would be allowed. Advantage: cleaner API, clean
distinction. Disadvantage: less flexibility.

Test were OK on my localhost, but failed here. I noticed that sometimes the navigator did not refresh and I had to call viewer.navigator.world.draw(), other than that, this implementation should be more or less functional. The performance is another question and I would like to also rewrite the tile deletion process using the binary heap implementation, which would not loop through all cached tiles, but simply call heap.remove() in O(log n)

This is awesome, and a lot to absorb! I am going to be busy for a bit upcoming, so I'm not going to be able to get to this right away, but I will! Meanwhile, I'm hoping @pearcetm can I help as a sounding board, since he's got the other major patch in the works, and there's probably some touch points between them.
Thank you, @Aiosa, for tackling this!

I moved on with the async decition once thinking this through several times. tile-loaded event is now asynchronous, meaning instead of ugly getCompletionCallback (deprecated), you can simply perform async stuff or resolve a promise once you are done. The thing is that the old approach had a race condition here when multiple plugins would touch the tile, now with strict execution order and event priority capabilities it is possible to do these things in a stable way.
The cache object follows the 'maybe' type, meaning the cache might be loaded (cache.data) or not ('cache.loaded=false'), then something like 'cache.getData().then(...)' can be done to do stuff once the data is ready. Supports both sync and async approach. Drawers can just access the data immediatelly and when not ready, delay the rendering.
Furthermore, tile has new originalCacheKey prop that allows you to keep the original cache data in a system-defined way in case you modify your data somehow:

plugins can uniformly access original data
system can create tiles without server access relying on the original data presence https://github.com/openseadragon/openseadragon/blob/750d45be81c09f4651cdd60fd2cd25e65a07ea19/src/tiledimage.js#L1533
originalCacheKey != cacheKey only when the data contents is modified, not necessarily the data type

no overhead, single cache in most cases, and if you modify tile data, you usually want to keep the original data anyway

will have to stress this convention in the docs and guidelines, but it is the most straigforward, simplest way of keeping all stuff compatible

The downside is the missing support for async and await, which makes the implementation a little promise hell. Would be more readable otherwise.

I also added default timeout for QUnit of 60s, debugging is a drag if a test does not time out - you cannot rerun it in a separate window, since it shows rerun link only when failed.

@Aiosa What do you consider left to be done here? Are there open questions we should discuss?

Well, the docs, trying out how it performs/behaves (e.g. re-implementing the Filtering plugin) and possibly adding better cache handling/ordering using smarter file structures for cache expiration (instead of iterating the whole cache array every time) and see how that performs (the cache system already implements a heap so why not using it).

And also write tests. It will still take some time to finish, here is a small demo of the cache zombie feature.

I am facing a (probably final) design issue I would like to resolve.
The problem is that as of now, the new cache throws away references to the old data. This might be problemtic with user-interventions, and cache re-use. It all boils down to the tile-loaded event which is as of now inconsistent.
Suppose that we fetch na image. We use canvas so that tile cache data is rewritten with context2d reference. When a new tile has to be loaded for which we find an existing cache record (or revive zombie), we might find tile ready for rendering. Cool.
However, the tile-loaded event is fired with a different input type than it was when the data was loaded for the first time!
I think in OSD this is not an issue, but user might

arrive at unexpected type (not really issue functionally-wise, if API used correctly, no errors)
do unnecessary work/conversion

at worst, context2d data will be asked for as image, converted to canvas by canvas API, the the renderer will have to once more ask for canvas -> context2d conversion. This furthermore creates one tricky issue: drawer imposed data type requirements on a cache, thus the cache record type is context2d. If we finish with canvas type, we try to push this data to
a cache record of distinct type (if it already exists). As of now, such attempt will ignore the new data: it will add the tile to cache record and use existing data instead (it stores context2d!). Meaning all the work done was pointless. Note that this should be consistent behavior, since the cache key is equal, meaning the data already stored is the correct data.
This brings me to another related problem: we have two types: context2d and canvas, we might ping-pong these type conversions from one to other as I for example usually think about these two intechangeably, and better would be just having one type.
Extending on this: users might be tempted to do something like
...addHandler('tile-drawing', e => {
      e.tile.getData("image").then(...);
});

which would fire image <-> canvas conversions for each tile per frame. But that's probably just the cost of such flexibility. My point was that even e.tile.getData("canvas").then(...); would trigger conversions - which would be not so intuitive (given we use the canvas drawer).
Solutions?
To cache on the data type level seems to me like unecessary overhead.
Checking that no type conversion is done in tile-drawing seems like a good idea. But maybe it's desired behaviour.
Keeping always the very first data type cached explicitly, running the tile data initialization from the beginning, and somehow solve cache write collision differently (otherwise, the result of the whole execution is pointless).
Not raising the event when the cache was already initialized (my choice).

This overhaul does not introduce these issues - they've always been there. But being forced to do manual conversion made a developer relize the consequences. Having simple API might make people write super slow rendering (tile-drawing + conversion). Ideas?

I don't think I fully understand the scenario you're describing, but I do have a couple of quick impressions:

What's the advantage to having both context2d and canvas as different types? It doesn't seem necessary to me.
Even if we still support tile-drawing events, nothing should be allowed to delay them. I don't see any problem with people doing asynchronous conversion in tile-drawing events; the result just won't be ready until some future draw. As always, the OSD philosophy is "you draw with the data you have, not the data you wish you had".

As for the bigger question, would it be solved by allowing the system to save intermediate steps? I agree that it probably shouldn't by default, but maybe that could be an option? Maybe by default you always go back to the original when modifying?

The advantage would be users being able to use various types - handiness. And it might as well introduce these re-conversion issues. I don't know which one is better. In the end, the same code gets called anyway (either automatically or manually), the difference is that conversion process is asynchronous.

you draw with the data you have, not the data you wish you had

Yes, after thinking about it, I think the ebst option is to change the tile API a bit to not to change the tile cache immediatelly, but only when user explicitly tells it to. E.g. tile.getData() will return a copy of the data (or reference if no conversion happens), and changes the tile cache type once when user explicitly asks it to (~save).
This will help since renderer will happily draw old data untill new data arrives. We might as well either force users to return drawer-compytible type (or ensure this automatically) so that when user finishes, the conversion to something drawer-compatible happens ASAP to not to hinder the animation.
Caching immediate steps on data-type level would be a bit harder to implement. For now, I am trying to design support for the most basic advanced scenario: tile will get cache + original cache key (these equal by default), and first change to the cache data will ensure the original data is preserved. Then, if a tile is loaded to the system and cache exists (zombie or re-use), depending on cache it uses it can

cache from cacheKey: does not call tile-loaded since we found cache data
cache from originalCacheKey: call event if these keys differ - meaning it stores some first version of data and can re-execute the pipeline

Which kind of works, except that new tiles (as of now, zombies erase this info explicitly but don't have to) might not know the changed cacheKey so that they would always re-execute the pipeline, which might be desired behavior (you do the initialization for all tiles, we cache only the original data). And then

change cacheKey = "__mod__" + cacheKey  so that modified cache key is deterministic [current behavior]

new tiles will know which key to access to check for modified data presence
downside: might interfere with custom code, but IMHO this is such a generic thing everyone would want to do that we can just recommend no to touch these and create different custom cache objects for additional usecases

change cacheKey = <random> so that modified cache is never shared and tile-loaded event can be executed

otherwise, uses would re-execute their pipeline with originalCacheKey data, modify it
attemtp to add cache of a deterministic changed key "__mod__" + cacheKey to the viewer cache
collision: the cache already exists and holds data:

the result of the new pipeline overwrites the data for all tiles sharing the object, requires different approach than what discussed above
the result of the new pipeline is discarded [current behavior]

When users decide not to keep the old data (now the default option) and overwrite the cache itself (one per tile), then it makes no sense to re-execute tile-loaded over shared cache, because the cache state will differ with each repeated call.
This seems to me like workflow that works.

I'm still only partially following; this cache stuff is confusing to me! Anyway, what you've described sounds sensible.
Regarding the cache key naming, is that something that would be a convention and the developer would have to know what to do, or is it something that we enforce somehow? If it's a convention, what are the failure modes when they don't do it properly? How do we educate them to do it right?

The advantage would be users being able to use various types - handiness. And it might as well introduce these re-conversion issues. I don't know which one is better. In the end, the same code gets called anyway (either automatically or manually), the difference is that conversion process is asynchronous.

We're talking about context2d vs. canvas, right? It's the same data type, just different parts. We should just use context2d and if people need the canvas they can use context2d.canvas. Unless we're talking about the possibility of WebGL canvasses?

These cache naming things would be convention, but convention that is handled internally, they have two cache key props they can use. When they don't properly use it, the only thing that happens is that some tile might receive different cache object than they would expect it to and vice versa. Purely from the system point of view, as if they did not properly provide 'properly unique cache key getter'. So we educate them by using the cache API through the tile instance only. The only problem is that they could accidentaly use the same cache key as we automatically derive - there is a very low chance, and we as well might use some URL-unfriendly characters to ensure this does not happen.
Thanks for the last point! Indeed, canvas type is abiguous as it's context defines its behavior. Than it's clear.

Okay, sounds reasonable!

Great that you're updating the filtering plugin! This will be good to have, but also it's a good test case.
When you say it takes a little longer at startup, how much longer are we talking about?
It does seem like when a plugin asks for the version of the data prior to their changes, it should include the changes from the previous plugins. That said, how do we determine what the plugin order is? And of course it's not just plugins... It could be the app itself. It does seem like some way to "stack" effects in a stable manner would be good.

Yeah, I think we should assume we're getting rid of (deprecating for now) tile-drawing.
Regarding the cache, here's a thought experiment: What if the only data you can access is the result of the previous plugin? Only the first plugin can ever get access to the original data. Would that work or are there problems with that? I suppose someone needs to be able to get access to the current data, for the sake of drawing (or perhaps data analysis), but not modification.
Would it be worth having an actual explicit data pipeline structure? We load the data, then plugin A does its thing, then plugin B, then it's ready for drawing. If plugin B's settings change, it gets to re-render based on the cached output from plugin A. If plugin A's settings change, it re-renders based on the original data and then its output goes to plugin B to re-render. Of course that implies we store each step. Perhaps it's better to invalidate the entire chain regardless of which plugin needs a re-render?
I imagine for a lot of scenarios we'll only have a single plugin, but it's worth nailing down what happens with multiple. Apologies if I'm repeating things you've already said... Just trying to make sure I understand the issues.

Yeah, I think we should assume we're getting rid of (deprecating for now) tile-drawing.

I did not want to deprecate this event. For the canvas drawer, it is still relevant. Since HTML drawer does not support it, I thought that having drawer-dependent events makes the most sense, simply docummenting which events are supported by which drawers. With canvas drawer plugins like OpenSeadragonFiltering will not work since they rely on it...

Would it be worth having an actual explicit data pipeline structure? ... Of course that implies we store each step. Perhaps it's better to invalidate the entire chain regardless of which plugin needs a re-render?

I tried having implicit pipeline structure with each plugin driving the behavior. By promoting selected APIs I thought we could achieve both simple usage, but at the same time also we could provide devs with freedom to do whatever they want. Yes, if we could let only the first plugin access the original data, and then let subsequent plugins consume the data as they chain, that would work well. Then, we don't need to store each step, but we have to trigger the re-validation bottom up. The problem here is that users have too much freedom with the current event-oriented plugin design: this execution pipeline must have explicit execution order, e.g. we must know when it starts and when it finishes

you don't know when the pipeline execution starts
-> assume at the beginning of each tile-* event
-> what if users access tile at arbitrary time, e.g. by item.tilesDrawn property (I do as well, although for read-only purposes)
you can assume that the pipeline execution ends when the tile is being drawn and the tile status is ready, though it might introduce other problems
you need async events (which is not that problematic, we can make tile-drawing asnyc too)

If you want to have explicit pipeline, then you have to define nodes, what they consume, and what they output. This is already implemented in the type conversion process. If plugins registered via type-convertors for the data processing, then there is predefined execution order and it already works. Or not. Sort of:

each plugin would have the ability to output only one data item (limitation)
plugins cannot control at what time they execute (limitation)
plugins would have to respect mutually and explicitly existing types to allow chaining
plugins would likely not be executed anyway (the system finds shortest path to the desired drawer type which means ignoring longer paths that consist of using plugins)

So the type conversion system is not built for this usecase.
I thought keeping up with the existing event system would be better, just to work out the issues about the implicit pipeline I mentioned above. IMHO a solution would be to create this chaining pipeline automatically around events that relate to tiles, and allow users to trigger events outside the drawing process. What we could do is to enforce the event-based execution, by restricting users from write access to tile outside the event, and having the event act as the pipeline interface, which ensures plugins execute on outputs of previous plugins. But if we enforce the write access on cache, we break deprecation feature compatibility. And also, the event-tile-based pipeline makes much less sense with WebGL, since the paradigm is fundamentally different there. Modular WebGL will allow you to create interface-level elements you chain at the compilation time, and the rendering is done via compiled shader on GPU. You can also trigger re-compilation, but on on a tile level. Modifying data on CPU is much less desired and although we should support it, we should also discourage it.

Man, this stuff is complicated! I keep coming back to we should define which scenarios we want to support. We don't need to solve every possible permutation of image caching.
Regarding tile-drawing, I'm still unclear on what advantage it has over doing the manipulation on tile-loaded and again whenever settings change. I suppose if you're doing some sort of animation and you want to make sure the tile gets updated every single frame, this could be nice. Is that really a scenario we are expecting?
BTW, I tracked down the PR that introduced tile-drawing... Some similar conversation there: https://github.com/openseadragon/openseadragon/pull/348
At any rate I continue to feel strongly about that any events that fire during the drawing process should not be blocking, which means no async processing can happen during tile-drawing before the tile is actually drawn to the screen. If someone needs to update the tile every frame, they need to make sure they have all the data they need in the right format already.

Actually, if the event was async we know updates have finished so that we can collect requests for update and say re-render  (if any requests found)

@iangilman I take it back, ``tile-drawingshould be deprecated. It is supported only in a single drawer as of now, and there istile-drawnevent I did not notice, so firing two events per tile drawing seems like already too much. Since cache access is async, havingtile-drawinghandler async makes no sense (otherwise it gets fired _after_tile-drawnif made async). I guess the purpose of the event was to have access to thecontext2D` property of the tile. Since data is managed now automatically in an uniform way, there seems no reason to keep this event around anymore.

Interesting! The lazy loading is an important angle... No point in updating tiles that aren't even in the viewport at the moment. That said, they could eventually move into the viewport, and we wouldn't want them to be drawn with the old data at that point, but we also don't want to wait for them to update before drawing.
I can see the appeal of tying into the tile-drawing or tile-drawn event, but it still doesn't seem to me as clean as a purpose-built API could be.
It seems to me the ideal API for that sort of thing would be a "re-do the tiles" thing the dev can trigger that goes through and updates all of the tiles that are in view and renders a new frame, then continues to update all of the other tiles in the cache in the background.

I don't know precisely how the details of this would work out in the new system, but it seems to me that if something changes (like a filter setting) that requires reprocessing of tile data, the following should happen:

existing tiles in the cache with old data need to be invalidated and NOT used for drawing
as the tiles are processed asynchronously, they should fire a new tile-loaded event and become available for drawing

Deciding when each individual tile should be reprocessed could be done in a number of ways.

The problem is that you don't want to remove the tile from the system nor fire tile-loaded event

users might have custom caches, they need the freedom to decide when to free them, we should not force tile re-entering the life cycle

having separate tile-loaded and tile-[something] that differentiate the tile state (existing data VS fresh new tile) allows devs finer control over the data flow since they know whether the tile has been already 'initialized'

re-inserting tiles creates unecessary overhead on the cache which needs to modify its records too often, this could be avoided if we know we will keep the tile, but then it just adds more complexity to the implementation

I like what @iangilman proposed. Events are still desirable, since they allow simple execution. We could:

deprecate tile-drawing since there is no need for the event, or even better

move tile-drawn before the tile is being drawn and make it essentially tile-drawing which is not asnyc
users still might want to access the tile before it's being drawn, and the data to draw is accessible immediatelly if the data type fits, so synchronous updates are still possible

explicitly provide mechanism for tile updates which should be preferred

called manually by the app/user
registers an event & provides execution pipeline that manages the data chaining (as discussed above)
somehow selects which tiles are to be processed in a priority (item.tilesDrawn), gradually makes sure updates apply on each tile sooner or later (item._tileCache._tilesLoaded)
can either (or both, a new configuration parameter)

prevent the drawer from accessing the tile data until finished by setting tile.loading=true in the process
-> this can modify directly the data as the tile is stalled
can allow the drawer render using old data and re-rendering once finished (current implementation)
-> this accesses the cache by copy-only, to ensure we do not modify underlying data for rendering

Thanks for the fresh view, I will try to comment on thing that are different in the current implementation or things I view differently, the rest I leave out conforms to the current design. I tried to achieve the target design changes to improve the usability of the system, but keep it as similar to the original implementation where possible.

Previous conversations between @Aiosa and @iangilman have raised some objectives for how the system should ideally work during dynamic data manipulation:

I want to add that the async execution is mainly due to conversions that cannot be performed synchronously. While it has the advantage of not stilling the animation event loops, since all work is done on the main thread, CPU-hungry transforms still affect the framerate. Also, as of now async cache makes initial loading slower, which is noticeable when tiles are browser-cached (network masks the latency, I need to test that a bit more but from the refactoring of the Filtering plugin it seemed like so).
The main motivation was that OSD does not have clear data control flow (and I wanted users to be able to provide any data type to the system they want, and be compatible with any drawers or plugins out of the box; at most providing one edge for the data type conversion graph; default scenarios might not be optimally performant, but it should be flexible enough to allow further optimizations). And mainly, most advanced plugins and features simply ditched the cache system and saved data persistently on tiles.

Unload old data from the cache (not necessarily from memory) when the cache is full

My current design of cache is kind of unique_ptr style, meaning if you gave it something that has explicitly defined destructor, it is called when you gave the cache a reason to do so (e.g., free a texture), but since CPU heap is GC'ed, it just works like a normal reference. To avoid destruction, you must steal the underlying data (which is as of now not API-possible, we could add support).

Should cached data be modified in order to change what is drawn to the screen?

Not in my opinion.
Multiple Tiles from one or more TiledImages may depend on the same underlying cached data, and not all of them may want the data to be changed (or changed in the same way).

As of now, I do this to optimize the maximize the cache re-use, and in my opinion, if two tiles render off the same cache object, then the developer's job is to ensure these tiles have different cache key. And yes, state change is propagated. cache knows which tiles it belongs to.
Truth is, current API does not promote this scenario so you need to actually create the cache with data manually and change tile.cacheKey value, which is intended to be the slightly more advanced use of cache API. We could do better here.

Suppose you have plugin modyfying the cache. Then, if two tiledImages share a tile and different filters are applied to the tiles, that came from the same original, data, you must instead of
tile.getData(type); //now creates copy if not called with second arg FALSE
tile.setData(data, theDataType); // keeps old data and creates new cache if not done, else overwrites the main cache data, also disable-able with a third arg=false

do this
tile.getData(type);
tile.cacheKey = newKey;
tile.setCache(newKey, data, theDataType); //creates new cache if key unique (else **ignores** the data and just receives cache reference)

If we were to create always a new cache, then it is impossible to have two tiles sharing caches (except the original data), now a programmer can do both (but they need to know this which is a spot for bugs). Moreover, the default data pipeline scenario keeps at most 2 caches per tile (no matter how many plugins you use), which drastically reduces the cache strain (it is still implemented in array traversal with default size of 200 caches, which gets easily filled even with a single tiled image, if you were to increase the cache multiple fold a tile (even in e.g. mirroring!), you would need to start off with cache size default at least 2k I think. This might not be friendly for all mobile devices, and each cache access does O(n) search :(

Could type conversion could be allowed without considering it to be mutated, since it could be converted back?
This could still potentially be problematic if the conversions are async (see below)

I would say yes if we want to minimize cache load. The current design ensures colliding async accesses always work, and the default scenario is access (=default copy) by async, set synchronously, so it's a hot swap.

Tile objects would reference a DrawableData object; drawers would request data synchronously from this object.
DrawableData keeps a reference count of Tile and Cache references; when this drops to zero, fire a data-unloaded event to allow cleanup by drawers etc.

Can you give an example? I am not sure I fully understand the idea. Also, how does tile reference multiple DrawableData? Or just a single data item gets wrapped, others are in cache? So we still also have CacheRecord so we can put data to the cache? Since default access by tile API is copy-on-get, swap on write, it seems to be also 'immutable' (unless dev wants to modify it which is possible, and we avoid creating new cache objects by default, we just keep the original data and the data to be drawn).

Accessing a different type that has a cheap synchronous conversion could be done on-the-fly (and cached or not), and returned immediately.

Although it is possible to access the underying data cache.data if cache.loaded=true, I did not add support for sync conversions since it might be too complex for use, we could add API for explicit sync convert, but that would mean convertor.learn and other must specify whether the method is asnyc or not, which seemed to me like unncessary complication, and I went for uniform API.

Access requests that requires async conversion would return null and begin the async process, with results cached:

Locally within the DrawableObject, or
As a new DrawableObject added to the cache, with the type accessor of the original object redirected to the new object,

Previously cached converted types would be returned immediately

Generally, I think you would get bloated with keys and lower the cache reuse to minimum. My thinking is that a plugin engagement is a linear pipeline, which needs the original data (by default stored in separate cache) and needs to store the default output (mid-results are relevant only for subsequent plugins, and can be accessed directly if type equals). Again, I think keeping midresults gives too much needless strain on the cache (suppose plugins access data to modify these, then pipeline mid-results are relevant only during the single execution, future ones must re-render, stuff likely changed).

Depending on use case, Tile objects that referenced the original DrawableData could be updated at the beginning or end of this process

Updating a Tile at the beginning of the process would stop old (bad) data from being inappropriately shown in the meantime.

Desired. But hard to do nicely since you need to essentially block drawing and process all relevant tiles to update at once (or, delay the data swap to a single call once ready). This is were sync events and execution rocked, but made any callback/async funcitonality real pain (viz Filtering Plugin that had to re-insert tiled image to avoid artifacts). Also, stilled the animation frames.

Updating a Tile at the end of the process would allow old (good) data to be shown in the interim, which might be desirable.

Creates artifacts. Current approach (hot swap), simpler to do. I would allow only as explicitly enabled feature.

Nice way of thinking, but such a cache-heavy design would IMHO be better for a subsequent PR. I have even now problems with the async call penalization (execution is deferred) which is probably a reason for a tile updates slowdown, so I need to fix this first (or at least mask it). I have hopes for the multi-best-tile PR to improve this, but I need to merge relevant PRs first to be able to test it, since they change drawer design & tile loading.

The main motivation was that OSD does not have clear data control flow (and I wanted users to be able to provide any data type to the system they want, and be compatible with any drawers or plugins out of the box; at most providing one edge for the data type conversion graph; default scenarios might not be optimally performant, but it should be flexible enough to allow further optimizations).

Better defining the data pipeline is definitely a good thing!

I would say yes if we want to minimize cache load. The current design ensures colliding async accesses always work, and the default scenario is access (=default copy) by async, set synchronously, so it's a hot swap.

Tile objects would reference a DrawableData object; drawers would request data synchronously from this object.
DrawableData keeps a reference count of Tile and Cache references; when this drops to zero, fire a data-unloaded event to allow cleanup by drawers etc.

Can you give an example? I am not sure I fully understand the idea.

I'll try to explain the idea a bit more clearly.

TiledImage creates an array of Tiles that should be used for the current frame.
In order to be included, these Tiles must have data immediately (synchronously) available that can be used for drawing, because all of them need to be drawn in the same render operation.
In the scheme I described above, the DrawableData class would encapsulate the data.
Tile.drawableData would provide access.

How would this work with navigation or other frame update triggers?

When a Tile comes into the viewport, two things can happen:

If DrawableData exists in the cache, the Tile grabs a reference to it (Tile.drawableData = cache.get('key'))
If DrawableData doesn't exist in the cache, the Tile gets added to the data loading (and processing) pipeline.

As long as a Tile stays in the viewport, it keeps its reference to the data
Tiles that exit the viewport would have Tile.drawableData = null, releasing their reference to the data

If the cache keeps a reference, it won't be garbage collected and can be reused when the Tile comes back into the viewport

How about updating the drawable data if the data processing pipeline changes, like a different filter setting?

If you don't want to keep drawing the previous data during processing, set Tile.drawableData = null so the old tiles aren't drawn, and they'll be added to the loading and processing pipeline.

This uses the same priority scheme for drawing as plain images, where low-res tiles that provide full coverage are drawn first, with higher-res tiles added as they become available
The data loader can use a cached version of the web request, if available, or re-fetch if needed
Once the data is loaded and processed, it can be added to the cache and/or directly to a Tile within a Promise

If you want to keep showing the old data while the new process is ongoing, don't set Tile.drawableData to be null, just start your async process and swap the reference once done

For example, this could be desirable if you were applying a sharpening filter. It would just get sharper one tile at a time but would be better than the whole image going back to low resolution.

Also, how does tile reference multiple DrawableData?
It doesn't - just a single DrawableData (ready to be drawn) or null (not ready yet).

So we still also have CacheRecord so we can put data to the cache?
Yes, we still need to cache things, so CacheRecord would be useful - this is how the data would be saved from garbage collection so it could be reused.

The descrition fits the current CacheRecord behavior. If you want synchronous access, you can do tile.getCache("key").data which is either loaded or undefined/null. It also has loaded property that informs you whether synchronous access is possible.
The record has two access data ways: direct conversion od the data (transformTo) and copy (getData) that allow you to say in async context: give me the data in desired type either in the cache (transform) or as a copy (getData). The former will make drawers ignore the tile since cache.loaded=false, the other is meant to swap-in-place the cache (overwrite()), so that drawers never encounter  cache.loaded=false for the given tile (these methods are async, but never run in parallel). And all ensure any intermixing of these calls access a valid cache state. So it IMHO fulfills your description, just the API is different. The current design makes all data managements and workings the same via uniform API, with ability for tile to define any amount of caches to reference, the selected cache to draw is defined by tile.cacheKey which ofc can be changed.
Now, from the DrawableData I have a feeling that everything that happens to the CacheRecord would be desirable in both cases: I want to be able to call transform on drawable if I am a drawer or another system entity that verifies the data is compatible with the current drawer used.. I also want to have the ability to transform records since they keep the data which I might need in different format to use. Similar with copy access, there are use-cases in both scenarios. So thinking about it, I think something like
//tile.js
    get drawableData() {
        const cache = this.getCache(this.cacheKey);
        if (!cache || !cache.loaded) return null;
        return cache;  //or cache.data if we want the data reference
    }

more or less implements the described behavior. Although we would maybe want to prevent devs from directly changing the drawableData and rather forcing hot-swap it (which is done hy copy-access and assign data write), we could as well want the devs giving the freedom of doing both (e.g. work with the drawn cache directly = not available for use in the process, or, hot swap, which is promoted by nicer name getData, vs transformTo).
My current understanding is therefore just the small difference in the above code example. There actually is one thing that is different is that transformTo sets loaded=true after finish, so then the tile thinks it's ready for drawing, whereas user code might need to perform further modifications. There it helps a bit to have explicit tile.drawableData pointer since users must explicitly assign it. But this would not help between two plugins anyway (two async handlers, between them the tile.drawableData handler is set by both). Therefore, we need an external break anyway, which would

ensure the tile cache state is set to ready or loading=false when a data processing pipeline finishes, and possibly
delay this state change when multiple tiles finish, so that the change is immediate

So far, I prefer more uniform API (function names might not be the best ones) and OSD's support for the data pipeline in automated way..

The descrition almost fits the current CacheRecord behavior.

Yes, I think there are lots of similarities here, which is good.

If you want synchronous access, you can do tile.getCache("key").data which is either loaded or undefined/null.

I'm glad there's synchronous access because I think it is mandatory that drawers use synchronous access. I can't think of a situation where it makes sense for a drawer to ask for asynchronous access for the purpose of drawing the current frame.
My philosophical objection to the API you're creating here with getCache('key') is that this design tightly couples the cache with the tile and the data manipulations. I think this is undesirable. To me, Cache should deal with, well, caching things (managing which data stay in memory, which data to unload if needed, etc). Exposing the cache through a Tile API doesn't make sense to me - the tile should have data or not, regardless of whether it came from the cache or currently exists in the cache. I am in support of the manipulations you describe - I just think it would make way more sense to have it be decoupled from the cache itself.

My philosophical objection to the API you're creating here with getCache('key') is that this design tightly couples the cache with the tile and the data manipulations. I think this is undesirable. To me, Cache should deal with, well, caching things (managing which data stay in memory, which data to unload if needed, etc). Exposing the cache through a Tile API doesn't make sense to me - the tile should have data or not, regardless of whether it came from the cache or currently exists in the cache.

Yes, but:

you need this functionality of dealing with data toghether with the data
you need to store this object in a cache, so it can get auto-freed

So I still feel like if I renamed CacheRecord to DrawableData, it would suddenly conform to your description. Why not, let's do it. The actual cache functionality is not implemented in CacheRecord, but TileCache class. The record itself just provides API for data operations (+ ability to get all tiles that share the data, which you might argue is a cache-related functionality).

I guess something I don't grasp yet is why data to be drawn must exist in the cache

Because there are also tiles that are not in the viewport. The TileCache measures its load based on cache count. So you could end up in a scenario where you have 0 caches loaded, and 1000 tiles loaded simultaneously and growing, since the cache thinks it's empty. Also, the cache cannot free CacheRecord, it can free only Tiles. The reason is that with cache you don't know if it is needed in the system. With tiles, you do (last access time). And since tiles share multiple caches, unloading tile does not have to destroy the cache object if shared.
Of course it can be implemented so that you have one cache item (or drawable data or whatever you call it) explicitly pulled out from the cache and work things out, but it adds implementation complexity. What if you need to share this data between tiles? How do you know it is referenced by multiple tiles if it is not in cache, does not have tile references, but is a member of Tile class? You don't. So there is no way of sharing this data. Meaning in a simple scenario where loaded data=drawn data, there is 0 cache reuse. Zero. Of course this could be somehow implemented but... even more complexity.
So TileCache is the cache implementation. CacheRecord is the data wrapper that supports asnyc/sync access and so it happens it can also be stored in a cache (and is managed by it if so). And by having all the data element behave the same, you can have M tiles share N caches (and vice versa), have a granular control over how much cache items are load, and control cache re-use by cache keys.
As you've pointed out, having more cached items required for viewport drawing than the cache storage limit can be an issue, we should try to detect this and increase the cache limit if so. This needs to be done in all scenarios, since if you don't include 'data to be drawn' to the cache limit, you end up with no caching at all since the system store all tiles and their data, not just the ones visible. And if it's desired behavior, just set cache limit to Infinity.

So I still feel like if I renamed CacheRecord to DrawableData, it would suddenly conform to your description. Why not, let's do it.

This may seem like a small change but I think it makes the system more understandable.

you need to store this object in a cache, so it can get auto-freed

A Cache shouldn't be needed for this. If it is, there's an issue with the design.
Unbounded memory growth during image navigation is obviously undesirable. Let's imagine how a system with no cache could avoid this. It would just require that when a Tile exits the viewport, it releases its reference to the data. If nothing else (like other Tile objects) holds a reference, the data is freed (i.e. can be garbage collected).
Memory growth would not be an issue in this scenario, because the only data held in memory would be that which is actively used to draw the viewport. The viewer performance would not be great, though, because you'd have to re-download data every time you return to a region you've already viewed. This is where the cache should come into play - to maintain a reference to recently-used data so it isn't immediately freed, and can be reused when navigation takes you back to that part of the image.
And what should be cached? The data. I know it's currently called TileCache but actually it doesn't make sense to cache the Tile objects, because those are entirely managed by the TiledImage that owns them. And, as you point out, data can be shared by multiple Tiles, and this doesn't mean it needs to be duplicated in the cache.
So, what I'm saying is that DrawableData can be referenced by two places: Tiles in the viewport, and/or the Cache. When a Tile leaves the viewport, it releases its reference. When it re-enters the viewport, it re-references it. When the Cache is full it releases a reference to an old DrawableData. When nothing is referencing the data, it can be garbage collected.
To me, that's not adding complexity - it seems like it is instead simplifying it.

I also want to say that the system I'm describing is not how it has been working previously either. But we have a chance to make it a really good design now during this major overhaul.

To some extent I agree, if complexity is entirely under the hood then yes it does only matter to developers. But since plugin developers are expected to be interacting with it, to me that introduces risks of bugs (or the need for lots of support) if the system is really complicated or just not intuitive.
I hear your concerns with my proposal/design considerations. I suspect we're disagreeing mostly because words aren't working very well to communicate these concepts clearly. I think I'll fork your branch and modify it so we can compare; it'll be a way to test performance too. I know you've been updating the filtering plugin too. It might be nice to incorporate that into a test page during development at least. Thoughts?

The new commits add

custom CSS for the static page server (I was always pissed by how dicrectories blend with files, it uses hack to find directories -- if the size tag is empty, it considers the item a directory)
concept of explicit pipeline with new event (needs polishing, and discussion, and tests, also with console logs for debugging for now)
the filtering plugin demo
auto reploading of tiles at new item arrival (needs polishing and decision if we want this)

Hmm the tile preload broke tests that rely on single tile per frame fetch, so I will fix that once we decide if we want to support it.

Okay, now we have first attempt at WebGL support implemented I am going to merge the changes.

Upon merging the code i found someone added a new event image-unloaded (https://github.com/openseadragon/openseadragon/blob/f7d86ac244826416d3090fbef86968f17fc40fe8/src/tilecache.js#L269) which I am going to merciressly remove since it goes againts all the effort we try to do here and is not part of any release yet.
It can be substituted by tile-unloaded event that receives a new argument destroyed which tells you whether the data is being kept in the memory or not. Concerning the iOS memory leaks, canvas2D will receive destructor implementation.

Ah I see this method and other one were introduced by the webgl PR for texture management. Then it's no big deal to remove them since our data types now have destructors.

Have you finished merging with the new master? If so, I will fork your
branch to start testing and potentially contributing to this PR.
On Sun, Feb 4, 2024 at 5:27 AM Aiosa @.***> wrote:

Ah I see this event and other one were introduced by the webgl PR for
texture management. Then it's no big deal to remove them since our data
types now have destructors.
—
Reply to this email directly, view it on GitHub
https://github.com/openseadragon/openseadragon/pull/2407#issuecomment-1925686457,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7LYHMJX5SOPBUZGHEZBCLYR5PALAVCNFSM6AAAAAA4P5324KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRVGY4DMNBVG4
.
You are receiving this because you were mentioned.Message ID:
@.***>

Not yet you would see a frew commits :)

I will not finish today unfortunatelly, but I have most stuff done - just 5 tests to fix.

The code should be merged. There are some minor issues and tests still do not fully pass - sometimes there are few weird errors, and it seems CI got stuck too. Also there are quite some TODOs which need to be resolved (particularly with the new async behavior to finetune animation), but from my point of view this is almost ready. @pearcetm could you verify this works on your end?
Btw: I tried to also start cleaning up code where I made more changes (use class, replace var with const and let). One of these cleanups should be also moving files to some hierarchy as the sources file count is increasing. I decided to leave this once this PR is almost ready to prevent merge issues from any other PRs that might come - there are quite a few changes. The proposed structure is:
utils/
  - matrix3.js
  - proiorityqueue.js
  - spring.js
  - strings.js
  - point.js
  - rectangle.js
  - profiler.js
  - placement.js /more like enum
  - displayrectangle.js
  - fulscreen.js
sources/
  ...*source.js
drawers/
  ...*drawer.js

... the rest

Reorganizing the files sounds good, and that looks like a good organization. We should do that in a separate PR after this, to avoid confusion in this one.

@Aiosa I see you added a filtering plugin demo page. Awesome! It seems currently broken, to me at least. Is that what you're seeing too?

For me it works both for the drawers. The only issue is that

if you apply filter, you have to move viewport, the redraw / refresh does not properly propagate
the async application of filters makes renderer blink black frames since sometimes the data to render is not ready

I did not have time to fix this yet, but I was happy it is already operable - regardless of what drawer you use.

I have a question: how (and what purpose) is that of imageFormatSupported and related? E.g. discussed here: https://github.com/openseadragon/openseadragon/issues/2453 - the support for image formats is directly given by renderers as of now (what canvas / webgl can eat), and in the future will be given by what format type convertors are programmed. To me it seems like outdated feature, yet I struggle to come up with a good replacement design. Remove completely?
I mean even now with OSDv4 I render zip archives thanks to the 'advanced data pipeline'. This PR makes it possible to render virtually anything. Thoughts?

It looks like imageFormatSupported dates back to at least 2012. I'm not sure exactly why it was added.
The thing I like about it is the idea that it helps with cross-browser compatibility. The formats that we say we support are the ones we know are supported on all of the browsers OSD is expected to run on. For instance, it's not uncommon for people to want to load .tiff files directly into OSD, but only Safari supports them directly (at least according to https://developer.mozilla.org/en-US/docs/Web/Media/Formats/Image_types). This means if they tested only on Safari (for some kooky reason) they would end up thinking that it works.
With the cache overhaul, it seems like maybe this feature is even more important, since we are now going to be supporting a lot more possible formats. I imagine it should tell the developer whether the format they are using is either natively supported by browsers or there is some sort of converter installed for it.

Okay, I managed to get some free time and fix a few things. I still have to measure the performance but otherwise it should work. There is no ability to say 'we support format XY' with the convertor class, so developers need to

program the type conversion
add the supported format declaration

To me this looks confusing. Format != data type looks to me like window != viewport coordinates, people will confuse it all the time. I would like to unify this somehow; in the end this function is used in just two sources (dzi and iiif), only the second one really uses it, dzi just throws an error. The rest of the sources just ignore it. Moreover format declaration cannot be fully trusted anyway. IMHO it would be better to just check that given data can be converted, and if the conversion fails just print an error that lists several reasons why this could be the case. If you test only on Safari you still won't see any errors anyway whether we check formats or not. So the format support could be just the IIIF class thing...

@Aiosa I haven't had time to dive into this extensively or do much testing, but I hope to do so over the next couple of weeks. In your mind is the filtering demo the best place to start? Have you been able to address the issues you noted above?

if you apply filter, you have to move viewport, the redraw / refresh does not properly propagate
the async application of filters makes renderer blink black frames since sometimes the data to render is not ready

I managed to mostly fix these issues, however, there are another issues
I tried to implement sharing of cache between multiple viewers. I did not succeed. Not only you sometimes cannot afford to share the cache completely (WebGL cannot share loaded texture between contexts) which I was able to solve more or less, but the overal tile fetching strategy is that tile record is created once it has data downloaded, which is too late: already two jobs per tile were requested at that point. So I realized it maybe might be much better idea to work with TiledImage positioning within one OSD instance. Harder to adjust plugins (e.g. annotation) to this separation though.
Concerning the new cache API, there are still easy to misuse scenarios, for example my implementation of the filtering plugin was doing this
        if (processors.length === 0) {
            //restore the original data
            const context = await tile.getOriginalData('context2d', false);
            tile.setData(context, 'context2d');
            tile._filterIncrement = self.filterIncrement;
            return;
        }

        const contextCopy = await tile.getOriginalData('context2d', true);
        const currentIncrement = self.filterIncrement;
        for (let i = 0; i < processors.length; i++) {
            if (self.filterIncrement !== currentIncrement) {
                break;
            }
            await processors[i](contextCopy);
        }

and I did not realize that my implementation of data restoration was in fact making the data item shared between two caches, and when one cache was freed so was the other -> black screen. But this happens only when you add some filters, then remove all of them and then try to again add new filters. But forcing users to copy on read will have its cost too.
Then there is another issue when the user calls tile invalidation too often (e.g. range input drag), my implementation of the plugin sometimes does not update all tiles to the latest state.
It will still take quite some work to get this right.

Okay I thought it is fixed but the webgl drawer still sometimes shows black screen on filter adjustments, I already ten times thought that it works already, then I clean up console log stuff and it stops working. I tried to put request redraw everywhere I could think of, still sometimes it does not properly redraw viewport... I will see

This is gnarly stuff! Thank you for working through all of it and keeping us updated ❤

@iangilman I would like your opinion on imageFormatSupported. I can see two ways:

moving it to IIF tile source which is the only implementation seriously using it

just implement a warnings on conversion failure listing possible causes

properly integrating it withint he tile source base class

this is very hard to imagine for me RN, how do we check formats in a generic way? we no longer have to download just images, it can be vector, byte streams / archives, text...

To me it seems like it tries to solve the single issue where image type cannot be supported due to formats, and also to add ability for IIF to specify a desired image format. The former can just check the image conversion routine and alert on error.
convertor.learn("url", "image",  (tile, url) => new $.Promise((resolve, reject) => {
    const img = new Image();
    img.onerror = img.onabort = reject.bind(null, 'Check image format bla bla bla etc');
    img.onload = () => resolve(img);
    img.src = url;
}), ...);

The latter would better be in the IFFF class anyway, since it is a specific protocol feature.

Yeah, it may be time to let go of imageFormatSupported, or just push it down into the components that really need it.
That said, with the new data conversion infrastructure, aren't we expanding our knowledge of what is supported? For instance, if we're loading vectors or bytestreams, either we have modules installed that can convert those into pixels to be drawn to the screen or we don't, right? So it's not like the existence of those new formats makes it harder for us to know what can be loaded, right?
Anyway, as far as I can tell, the only advantage imageFormatSupported gives us is helping to inform the developer that they're using an image format that only some browsers support. Maybe it's fine to leave that to them.
When things fail on the current browser, though, providing meaningful error messages would be good!

I am sorry I did not progress much here, but lately I was very busy. I will try to return to this in a few weeks when all the stuff calms down a bit :)

@Aiosa Thank you for the update!

Merge for the recent changes. I am still unable to get stuff smoothly working. I replaced OpenSeadragon.Promise with sync proxy and the issues remained.... I have some local testing changes that did not work and so I came to realize the issue is with the approach rather the implementation:
Since I was doing an async pipeline I somehow assumed that most calls should just 'work' and execute as soon as possible with the only limit to keep the cache state consistent. But cache consitency != screen consistency. Instead, we should restrict execution flow as much as possible to allow only meaningful routes:

if a cache is being used, restrict any access to it until finished

this is a bit tricky since we need a good mechanism that will enable users really do the stuff they need

do not store two, but three caches: original data, render target (main cache), and working cache ('swap buffer')

we might want to hide the working cache within the main cache since it must have the very same tile set connected, tricky to keep this consistent otherwise

collect all changes and when ready, swap caches to allow seamless changes

what about the tiles that arrive during a processing? keep them from being rendered until processing changes to not to render 'new' data on the 'old' canvas - we are waiting for all tiles to finish the update to the new state
have 'swap queue' to allow users to queue up custom changes or restrict any custom usage except for the defined routes

And we can both support async & sync mechanisms just by simply swithing between window.Promise and the promise stub. This is pretty neat. Any comments to the above?

@Aiosa I'm glad you're still working on this. Apologies that I haven't found time to dig into it in depth lately myself, it is very complicated!

But cache consitency != screen consistency.

Totally agree - and screen consistency is critical for making the viewing experience good.

Any comments to the above?

I will need to look at the code further to understand, I think. One thing that would help me think about this is if there are some concrete examples of scenarios that you're trying to support. Sorry if those have been described in detail already and I missed them. Do any of these scenarios exist in demo pages for testing at the moment?

Neither I had time lately :/
The scenarios that I wand to support is arbitrary chaining of plugins that attempt to modify tile data.
I want to have two things available:

managed route: you can call invalidate on world/tiled image, and use methods on tile that get the current data, modify it somehow and save it back; this route keeps one cache for rendering, one with original data, and one with the curently modified content, with effort to revert back to two or single cache (if the original data is rendered); this route allows ordering of plugins because od event handler priority, and this event should also somehow avoid tiles that share the cache (if position-independent processing is used e.g.sobel) or allow sharing only original data (e.g. computing vignette)

custom route: have the above implemented via series of Apis so that advanced user can do the above manually with customziations

ofc part of the system is automated data type conversion so that you don't care what the cache data type is, you just ask for the type you need (which already works - webgl renderer just defines how to convert image data to texture type and the system handles everything automatically, so that even filtering plugin works without worrying how to move data between CPU,.GPU, or whether and whem to free the textures), although there are rendering artifacts as described above

The scenarios that I wand to support is arbitrary chaining of plugins that attempt to modify tile data.

If this high-level concept could be put into a concrete example or two (with code ideally in a demo) I think it would be very helpful when evaluating the system you've come up with.

Yeah, the filtering plugin demo is a demo exactly for this purpose, it lets you select a drawer of a choice, and without changing a line you can use both to modify tile data using c2d pipeline. It does not have plugin chaning, but since the tile data is modified in an awaiting handler, this should be pretty straightforward (once I have the filtering plugin working).

The new direction sounds good to me. I agree we want to support chaining plugins, and we want to avoid drawing tiles that are partway through being updated. I think supporting both synchronous and asynchronous is good. And keeping things to a limited set of routes seems sanest.

I will try to work on this more in the following weeks, but I also might be loaded with other resposibilities, I will not abandon this project, but I have currently too much on my plate :/

@Aiosa I totally understand. I appreciate whatever work you can put into it when you can.
Perhaps we should be considering releasing OSD 5.0.0 ahead of this, though? We've been getting some good refinements on the WebGL code, and I think bug reports have been slowing down, so a wider release is probably a good next step on that front. What do you think?

The only thing on the release I don't like is the addition of the two new events that are not necessary with this PR, otherwise it looks OK.

The only thing on the current code I don't like is the addition of the two new events that are not necessary with this PR, otherwise it looks OK.

@Aiosa which are the events you don't like? If I'm remembering older conversations correctly, I think they might be needed for backwards compatibility (at least until the cache overhaul is ready). Is the problem that they'll eventually be obsolete, so it would be better to not make them part of a release so we don't have to worry about them moving forward (after cache overhaul)?

I am talking about image-unloaded and tile-ready events, they were introduced because of missing features which are part of this overhaul. So I was wishing for a clean replacement without deprecation. You don't need  tile-ready since a drawer registers formats it can use and the system will convert to the cheapst one automatically. You don't need image-unloaded since each cache object has a destructor based on its data type.

I am talking about image-unloaded and tile-ready events, they were introduced because of missing features which are part of this overhaul.

Even with the cache overhaul I actually like having these events because they let other components of the system react to the state of the data without being too tightly coupled.

You don't need image-unloaded since each cache object has a destructor based on its data type.

The webgl drawer needs to know when to release the data from the graphics card. In essence, the data is cached as a texture on the GPU in addition to being cached in memory. While this could be handled in the cache object's destructor I don't think that is good design. The GPU should be managed by the drawer. This event provides the loose coupling that allows management of the GPU "cache" so I think we should keep it.

You don't need tile-ready since a drawer registers formats it can use and the system will convert to the cheapst one automatically.

This one is less clear to me - perhaps it isn't needed.... but still, the webgl drawer needs a way to know when the data is ready to be converted to a texture and uploaded to the GPU. So I don't think it is bad to have it.

So I was wishing for a clean replacement without deprecation.

If we don't want to have to worry about deprecating them if they become unneeded, we can mark them as private so they aren't part of the public API. Then they can be refactored out at any time.

The webgl drawer needs to know when to release the data from the graphics card. In essence, the data is cached as a texture on the GPU in addition to being cached in memory. While this could be handled in the cache object's destructor I don't think that is good design. The GPU should be managed by the drawer. This event provides the loose coupling that allows management of the GPU "cache" so I think we should keep it.

Either you have the data in the cache, then there is a corresponding texture on the GPU, or not. It is the same whether you do it via the event (which reacts to the cache destruction), or via the data transformation pipeline (which reacts to the fact that the cache was either transformed or destroyed). IMHO this PR approach is much more stable. What if there is a plugin, let's say the OpenSeadragonFiltering, that wants to adjust the data and then draw it again? It will modify the data on the cache object (in memory), but the drawer will not recognize the data replacement (on the GPU). The overhaul detects change and replaces the old texture, freeing it, with the new one. Automatically.

@Aiosa It sounds like that would work, though depending on the implementation it would potentially couple the drawer implementation more tightly to the cache than I think it should be. I guess the drawer could be responsible for defining a cache object implementation for this data type that adheres to the cache API or something like that.
In the mean time let's just keep the events private so we aren't promising future compatibility for these events.

It does not couple anything. The drawer registers new data format (e.g. texture2D) and its destructor. That's it.
https://github.com/RationAI/openseadragon/blob/0a035afc2ddfc0cf54fc4655e83b921c62a04a43/src/webgldrawer.js#L914

Making those events private sounds good. @pearcetm can you make a PR for that?

Making those events private sounds good. @pearcetm can you make a PR for that?

Sure - just created one.

@pearcetm @Aiosa @iangilman
Question...as I've never worked with the tile cache code I'm not sure what to expect, but I'm doing some XHR debugging so I'm monitoring network requests. As far as I can tell, I'm NEVER getting any hits from the tile cache. Here's my modified basic.html code that creates the viewer...what the heck am I missing?
    var duomo = {
        Image: {
            xmlns: "http://schemas.microsoft.com/deepzoom/2008",
            Url: "//openseadragon.github.io/example-images/duomo/duomo_files/",
            Format: "jpg",
            Overlap: "2",
            TileSize: "256",
            Size: {
                Width:  "13920",
                Height: "10200"
            }
        }
    };

    var viewer = OpenSeadragon({
        // debugMode: true,
        id: "contentDiv",
        prefixUrl: "../../build/openseadragon/images/",
        //tileSources: "../data/testpattern.dzi",
        tileSources: duomo,
        //showNavigator: true,
        //loadTilesWithAjax: true,
        drawer: 'canvas',
        maxImageCacheCount: 4000
    });

The _tileCache.getImageRecord() call is what gets a cached image, right? I never get an image record returned from that function.
Code is current master

@msalsbery It is pretty convoluted...
The network requests are done by the ImageLoader:
https://github.com/openseadragon/openseadragon/blob/980deca107df92205e14bea684372ffe2dc41500/src/tiledimage.js#L1950-L1975
In the chain of calls initiated by the callback to the image loader, the tile is added to the cache.
Data can be gotten from the cache in multiple ways. I think this is the one that is being used by the current drawing pipeline - tile.getCanvasContext() which in turn calls cacheImageRecord.getRenderedContext():
https://github.com/openseadragon/openseadragon/blob/980deca107df92205e14bea684372ffe2dc41500/src/tile.js#L327-L334
This ends up calling a function which isn't actually defined in TileCache - instead, it is in TileSource, but is bound to the cache record:
https://github.com/openseadragon/openseadragon/blob/980deca107df92205e14bea684372ffe2dc41500/src/tilesource.js#L890-L911

The _tileCache.getImageRecord() call is what gets a cached image, right? I never get an image record returned from that function.

I guess getImageRecord() is one option for getting the data, but perhaps it is never actually used at the moment?

@pearcetm  Ok cool thanks.
I need to dig deeper, but it sure looks like I'm getting full network calls for every tile when I zoom in and out repeatedly. Is that to be expected the way I defined my tile source? Is the retrieved context from the cache just the URL so it needs to be reloaded anyway?

Is the retrieved context from the cache just the URL so it needs to be reloaded anyway?

No, it is actually a canvas with the data to draw.

it sure looks like I'm getting full network calls for every tile when I zoom in and out repeatedly

When I set breakpoints in the code I see the cached data being used. However, the default setting is only a cache size of 200 tiles, so depending on how many levels you're zooming through and your viewport size, you may be blowing through the cache really fast and essentially replacing the entire cache every time you zoom in and out. You may want to try upping the cache size in your options.

When I set breakpoints in the code I see the cached data being used. However, the default setting is only a cache size of 200 tiles, so depending on how many levels you're zooming through and your viewport size, you may be blowing through the cache really fast and essentially replacing the entire cache every time you zoom in and out. You may want to try upping the cache size in your options.

It's 4000 in my code posted above lol but I've tried 500, 1000, 2000, etc.
I'm digging deeper, thanks for the help!

Haha sorry I didn’t see your config above!
On Wed, Jul 17, 2024 at 11:17 PM Mark Salsbery @.***>
wrote:

When I set breakpoints in the code I see the cached data being used.
However, the default setting is only a cache size of 200 tiles, so
depending on how many levels you're zooming through and your viewport size,
you may be blowing through the cache really fast and essentially replacing
the entire cache every time you zoom in and out. You may want to try upping
the cache size in your options.
It's 4000 in my code posted above lol but I've tried 500, 1000, 2000, etc.
I'm digging deeper, thanks for the help!
—
Reply to this email directly, view it on GitHub
https://github.com/openseadragon/openseadragon/pull/2407#issuecomment-2235230302,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7LYHPUNIAHNS73K7SPVKLZM4XTLAVCNFSM6AAAAAA4P5324KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZVGIZTAMZQGI
.
You are receiving this because you were mentioned.Message ID:
@.***>

Cache does work, it is where the data for rendering is stored, otherwise each frame you would download the data again. Question is how it behaves wrt. cache size param, if you are suspicious of a bug, good to inspect it. Though please, take this to issues, this is offtopic on this PR.

I have merged latest master, and try to move this a bit today, tests might be broken I used Web UI to merge the changes so not sure how the tests will fare 😃

Looks like the tests passed!

I managed to move the implementation a bit further. Now, each tile has by default up to three caches:

rendering cache
working cache
original data cache

No postprocessing applied, rendering cache === original cache, working cache is null
Postprocessing in progress: rendering cache is either equal or different from original (first or subsequent postprocessing respecitvely), working cache is being updated in-place
Postprocessing finished: synchronous swap working cache --> main cache is performed if working cache exists, else main cache gets reset to the original cache if someone requested tile reset.

Of course people can do custom stuff with cache, this is just the default flow, but should be already powerful enough, supports plugin chaining and arbitrary drawer interaction.
We can trigger update routine (on user action or within a plugin):
viewer.world.invalidateItems();

A code that could a plugin implement looks like this:
tile.restore();
if (donWantAnyProcessing) {
    return; // will fall back to single-cache tile
}

//get arbitrary supported format
const contextCopy = await tile.getData('context2d');
...do something ...
//store arbitrary supported format
await tile.setData(contextCopy, 'context2d');

Pretty simple. Can also by synchronous if no asynchronous execution happens in the event handlers and openseadragon is configured as so.
Things that are currently missing is that the execution is not guarded, I will have to restrict modification of relevant data objects when an update routine is in progress. Also, WebGL renderer still has a few black screen keyframes when texture gets swapped, not sure this can be avoided anyway since we have to go and replace a few dozen of textures with new ones. This might be a cost for post-processing on CPU (tested with the OSD filtering plugin via Caman). Without the plugin, it seems to be working smoothly in both drawers.

That sounds great! Seems like a nice clean API.
When you say black screen keyframes, are you saying that swapping in new tile image data causes a flicker on the screen? We'll definitely want to address that... It must be possible to fix somehow!

Still did not push any code here because tests do not pass, but managed to get rid of the black screen blinking, it now WORKS. There are still some bugs, navigator needs to fire events too, sometimes deleted cache object gets into the processing pipeline, and also cache shared by multiple tiles might misbehave - need to test these.

Now, before we finish processing, we always ensure working cache is converted to drawer-compatible item first. Then, working cache is swapped with main cache, main cache gets destroyed.

The progress you've made recently sounds really great @Aiosa!
Maybe I'm misunderstanding things here, but are you saying that all data in the cache is converted before anything starts changing in the visible drawer output? On one hand, this could be a good experience. But especially if conversion is happening actively during navigation this also may have the potential to feel unresponsive (in one way or another).
If I've misunderstood what you mean, feel free to disregard the following:
Here's an idea - and maybe this wouldn't work well because I haven't thought through all the implications, but I feel like piggybacking on the existing loading behavior might be helpful. What I mean is that when a processing pipeline change happens, we could prioritize tiles for conversion the same way that they would be prioritized for loading: lowest-res first, then tiling in at higher resolution as tiles become available. Thus, the switch could happen as soon as enough low-res tiles to cover the viewport have been converted (re-loading original low-res tiles if needed), and it would work seamlessly even if new tiles become available due to navigation. I feel like this could provide a responsive feel similar to how the viewer already works. Thoughts?

I was thinking a lot about how to streamline cache-wide update. We will have to documment this to avoid issues, but so far my current approach works well. Will have to test it with larger cache sizes though.
There are two events that support cache updates, both are async. tile-needs-update is fired on each tile in the cache. tile-loaded is fired on newly arriving tiles. The former is performed in up to 4 streams (2x2):

_lastDrawn tiles are updated for each tiled image
whole cache gets updated (skipping those that are already processed)

After each, target viewport is re-drawn. The second step is potentially costly, but happens after viewport update. I was thinking about any other possible approach, but I think trying to delay/split cache update will only cause potential rendering issues (old data, black frames...). A good thing is that you can sort of expect tile-needs-update only when there is no navigation, it means something changes in the way the viewer is rendering, meaning user usually clicked some button at that point, did not navigate. On the other hand, tile-loaded ensures that newly arriving tiles are always up-to-date as much as possible, meaning when navigating, only little work is going on.
Touching the tile data outside these events is simply undefined behavior - if a programmer does it right, it might work, but likely they will get rendering artifacts. As I said, right now it feels pretty responsive. Question is how it will fare with cache size 10k...

Managed to fix a few things:

internal cache must be a map of caches, each drawer must have their own internal cache (internals are used e.g. by webgl to keep track of their tetures, so that the original data stays loaded in memory, internal cache gets wiped when cache ceases to be a main cache for a tile), this was necessary since there are two webgl contexts - one for navigator, one for the main canvas
cache must not free tiles that are in the update process, synce async execution will arrive at cases where we try to manipulate data, but another manipulation step triggered cache unload

might do unnecessary work, since cration of many working caches triggers many unload events, we could artificially lower the cache size threshold, free tiles and then update the cached tiles
but, first is called update on _lastDrawn, meaning some freeing up already happens since we first trigger only for a subset
and working cache replaces main cache which gets erased, therefore the size bump is only temporary and we would penalize the cache limit too much I think

the last bug I was able to find is that when I trigger update event too often, individual updates get mixed together and screen might contain tiles from different processing results, but any reasonable update event immediatelly fixes this...

https://github.com/user-attachments/assets/60d55ff9-1b31-49d0-88fd-4b9ced7cb9af

This demo is already using the same code (filtering plugin) for any drawer OSD might support, has support for async and automatic data type conversion. This is the WHOLE plugin logics (compare to the original)
 function tileUpdateHandler(event) {
            const tile = event.tile;
            const incrementCount = tile._filterIncrement;
            if (incrementCount === self.filterIncrement) {
                //we _know_ we have up-to-date data to render
                return;
            }
            //go async otherwise
            return applyFilters(tile, event.tiledImage);
        }

        async function applyFilters(tile, tiledImage) {
            const processors = getFiltersProcessors(self, tiledImage);

            tile.restore();
            if (processors.length === 0) {
                tile._filterIncrement = self.filterIncrement;
                return;
            }

            const contextCopy = await tile.getData('context2d');
            const currentIncrement = self.filterIncrement;
            for (let i = 0; i < processors.length; i++) {
                if (self.filterIncrement !== currentIncrement) {
                    break;
                }
                await processors[i](contextCopy);
            }

            tile._filterIncrement = self.filterIncrement;
            await tile.setData(contextCopy, 'context2d');
        }

@Aiosa That's looking fantastic! The logic looks nice and clean as well. I like your update prioritization as well too. Great to see this coming along!

The problem is that we need to catch potential bugs - I need to fix old cache tests that test different behavior, I need to add new test that uses the whole viewer and checks which caches are being created. Also, it currently does not work with cache sharing, I need to fix interaction of tiles that share data. Maybe some additional locking will be necessary...

It certainly is a complex task! 😅

Managed to get also shared tiles working. This was a bit daunting as I had to ensure all interaction works. Now, only one tile (first come first served) is processed. If two tiles share (original) cache key, I assume they are equal and perform all cache swaps (this was tricky :D ) and updates on these as well. Furthermore, all such tiles are locked from deletion until this process finishes.
Two changes: now each method that is involved in this lifecycle is called 'invalidate', e.g. invalidateItems, requestInvalidate, event tile-invalidated. Also, tile restoration is no longer called by a plugin, event is invoked with this option requestInvalidate: function (restoreTiles = true) {... instead, so that plugins are chainable (if every plugin wanted restoration, only last change would be reflected).
I fixed part of (my) cache tests, others I commented out to be able to share the latest code. They test different behavior so they do not work. I also did not test behavior with zombies. Maybe we can remove them, since the use-case for re-insertion of tiled images is no longer needed, as it is possible to update data also as async handler. But I doubt people will abandon this way quickly.
I also added some package.json scripts as some commans are pretty standard accross libs (npm run build, npm run dev..)
There might be still some bugs, I would appreciate if you can test it.

So the newest code of the filtering plugins looks even like this:
        function tileUpdateHandler(event) {
            return applyFilters(event.tile, event.tiledImage);
        }

        async function applyFilters(tile, tiledImage) {
            const processors = getFiltersProcessors(self, tiledImage);

            if (processors.length === 0) {
                return;
            }

            const contextCopy = await tile.getData('context2d');
            const currentIncrement = self.filterIncrement;
            for (let i = 0; i < processors.length; i++) {
                if (self.filterIncrement !== currentIncrement) {
                    break;
                }
                await processors[i](contextCopy);
            }

            tile._filterIncrement = self.filterIncrement;
            await tile.setData(contextCopy, 'context2d');
        }

Sorry missclicked. I also added newer even simpler plugin code.

I did not comment on many todos, they are there to flag potential interesting part of the code. I want to use them when debugging to behavior to remind me of possible places where I might to re-consider some design choices. As the design changes, the considerations change too. I want to delete them once we decide it is ok to roll out.

I have just tested old WebGL plugin (https://github.com/thejohnhoffer/viaWebGL) atop this PR cache system + canvas drawer (webgl does not work since it does not fire 'tile-drawing' event). It works. I have tested also old OSD filtering plugin. If I don't use caman filter from the available demo (hacky attempt to support async caman functions), it also works. No changes. So far it looks much more compatible than I thought it would be.

Here some memory measurements: It seems like the impact is minimal. The graphs are not aligned well but if you look at the stabilized part at the end, the memory usage seems to be similar...

Great that the plugins are still working! And good to hear regarding the memory usage. Things are coming along here!

What's your thought with the old filters in the demo folder? Is that just a demo or are we intended to maintain them as updated versions of those plugins?

It is there for others to try out. I can remove it later, or we can keep it for some time within the list of demos as a demonstration of a compatibility use. Or we can move them to the v5 -> v6 migration guide page or whatever we might want to document there.

I am more and more satisfied with the current design. Implemented new functions, all nicely works together. Rewritten HTMLDrawer to also rely on the cache api (IDK why it was not done yet, but I had there a TODO 😄), deprecated dirty props on the Tile instance, even HTMLDrawer code become more tidy (tile node copy / initialization is no longer in the draw method).

Where do you think we're at here? What's left to do?

My new 'avoid tile loaded event' fixed this issue on the theoreticall level, but now I have 13 tests to fix at practical level 👍
So some final bugfixing needs to be done. And docs.

Hmm. Hmmm.
My local run:
...
.OK
>> 303 tests completed in 42911ms, with 0 failed, 0 skipped, and 0 todo.

Done.

In those extreme cases, does working with a deleted tile cause problems or is it just wasted work?

In those extreme cases, does working with a deleted tile cause problems or is it just wasted work?

It's wasted work, possibly errors in the console. The filtering plugin calls async Caman processing, and inside the canvas errors out since it tries to work with zero sized canvas, which is caused by a destructor being called on the canvas, however, before giving the data to caman it is still valid, so something like replacement of the current working cache with a new one could be happening, while it is stil being processed.

Okay, most things are fixed. But I tested the new code with filtering plugin + webgl, and it again has artifacts when I start doing more crazy clicking around, so I will have to look into it again :/

hi, thanks for all the works
can I ask how should I use filtering plugin with these changes? should I change anything manually in osd or plugin code? or is there a updated version of both to work?

@sinamcr7 the filtering plugin updated version is available in demos. you can just run this version dev setup, and visit http://localhost:8000/test/demo/filtering-plugin/ or build this version, and grab the plugin code from the repo demo folder (or http://localhost:8000/test/demo/filtering-plugin/plugin.js).

@iangilman @sinamcr7 also note that with the plugin, fast change in updates (dragging a slider or adding too many filters at once) breaks the code for now, since:

new update is allowed to fire while old updates are running
old updates might be in the process of rendering (Caman)
new update effectively deletes the old data, which in case of context2D calls destructor on the data and sets canvas size=0, which in turn errors out on the invalidation pipeline and crashes

So this is a usecase we either have to implement on the plugin level, or guard this somehow on the viewer level.

It'd be great if we could guard for it, but if it's something with a clear API that the user can watch for, maybe that's reasonable. Give them a way to check if the tile has gone away?

It is not a tile that goes away, it is the underlying cache object. We could implement this somehow that working cache can be added multiple times on a tile, and we somehow propagate information whether the async execution is sitll valid. Then we ensure this old working cache gets freed only after the event finishes and that it does not affect the tile anymore...

This 'final touch' actually gave me more work than I thought it would. Tests are not updated yet, first I want to discuss whether this is the way to go.

All and all, this code seems to be quite visually stable, though there is no priority processing (all the tile cache just gets updated at once), which might be pretty slow. tile-drawing was in this sense much nicer, but it did not support async execution after all. When we say 'ok', I will go fix tests (there are test suites on methods that no longer exist), and we are ready to merge I guess.

Honestly I would welcome if we could wait with some features like the smoothing once this is merged. I had to spend another hour fixing this. Though part of the fix was new feature for drawers, they can now change their internal type they require, call invalidation routine, and the data gets updated to the new type. But this works (flawlessly) only when the viewer requires a format that plugins do not give directly (therefore the internal cache is not a reference). No tested with the case when the target drawn data is also the one to modify - in that case there could be black frames before the data updates. But so far there is no scenario that would trigger this.
The new implementation fixes tests, (+ two bugs), and jsdoc.
I also had an idea how to split processing of data: we could process immediatelly the viewport + some top level tiles (cutoff?) and delay updates of the rest into batches. A tradeoff between waiting and visible resolution. But maybe we could make this in another PR as not to complicate this even more.
So... docs and done?

https://github.com/openseadragon/site-build/pull/289

Thank you for writing up all the new documentation! I haven't had a chance to look through it yet but I look forward to it.
And yes, let's see about wrapping up this patch and getting it merged. It would be good to start a list of follow-up tasks somewhere... perhaps in an issue?
Also, I still think it would be good if you could write up the changelog entries, calling out not only the main new feature here but incidental things and the various breaking changes and deprecations.
Thank you for all of the work you're putting into this! It's an epic patch!

Also, I still think it would be good if you could write up the changelog entries, calling out not only the main new feature here but incidental things and the various breaking changes and deprecations.

I tried to write them in the documentation. The new entries include migration documents.

@Aiosa @iangilman I've found a little time to review this again. First of all @Aiosa this is cool and a huge amount of work, and kudos to keep pushing along with revamping this system to add excellent new functionality. That said, I'd like to request a bit of a pause and further discussion before merging, based on some things I've noticed while reviewing.

I've flagged some specific questions/comments/concerns as code reviews above. Beyond those, I have a few general thoughts at this point. These are not necessarily critiques of the current approach; some are exactly what the current version of the code in this PR does, while others I think suggest a bit different approach.

I think it goes without saying that we want to support flexibility in processing data, and having a clean and well-documented external API is the most important thing. Some internal complexity is going to be unavoidable. But for the sake of maintainability and further development we should try to keep the internal complexity as minimal and understandable as possible.
Conceptually, I think the main job of the cache should be to keep data around so it doesn't have to be re-fetched and/or re-processed, whenever possible (limited by the configurable cache size limit, of course). The converter functionality is excellent. I think it should be used alongside the cache but not tightly coupled to the cache itself.
Cache objects should know when they are being referenced by an active Tile and should not be unloaded during this time. Outside of the context of an active Tile the cache should hang onto data until it is forced out by newer data. If for some reason purging the cache is desired there can be an API for that, but it should be uncommon that it would be useful.
Network loading is essentially always going to be the most expensive thing; therefore, for a given (active) tile URL we should always keep the original around.
4.a) The ImageLoader should look to the TileCache using the original URL before making a new network request, and if found, should treat it as if it were just fetched again which could start the processing pipeline if needed. I would make the network request and caching part of that pipeline rather than making Tile or TiledImage keep track of this part.
Once an image is loaded from the network, there are really two distinct types of changes that can happen to the data for a given tile: changing the actual pixels values (like filtering/blurring/color shift), and changing the format.
5.a) Changing the pixel data is a tool for developers, and it makes total sense to cache the resulting pixels to be drawn. The processed pixel data that is ready to be drawn should be cached using a key that is specific to the processing that has been done. A given cache key should always return the same data! If something about the pipeline changes, the cache keys should change too.
-5.a.1) One of the things about the current design that makes me most uncomfortable is that there is not a one-to-one mapping between cache keys and underlying data (i.e. a given key can return different results). I think this is where a lot of internal complexity comes in, and introduces opportunities for future brittleness.
5b) In contrast to changing pixel data, changing formats is likely to be mostly used by Drawer implementations. It makes sense to at least provide an option to attach the altered format onto the cached pixel data so it doesn't have to be converted more often than necessary.
I think it makes more sense for a Drawer to use the converters (as needed) at the time of drawing, caching the result by attaching to the cached data object. However - are there async converters? It definitely makes sense from an image processing perspective, but do we really need that level of flexibility for converters? Async conversions complicates the ability to let drawers handle the conversion process, because it messes with things like calculating tile coverage.

Ok, it's getting late, and this is already a wall of text. I would really appreciate the chance to discuss more. I promise I'm actually excited about this, and I don't mean to come across as too negative by being a little worried by the complexity.
I hopefully also will have a chance soon to fork this PR and play around a bit with some refactoring ideas which I hope would at a minimum help clarify some ideas, even if we don't go with any of them.

Continue won’t fix it. Just don’t do the next line. The rest of the loop
still needs to happen. The way it was before this PR was correct, you can
just exclude the empty else statement with the commented out log message.
On Tue, Nov 26, 2024 at 8:06 AM Aiosa @.***> wrote:

@.**** commented on this pull request.
In src/webgldrawer.js
https://github.com/openseadragon/openseadragon/pull/2407#discussion_r1858505993
:

                   const textureInfo = this.getDataToDraw(tile);

                   if (!textureInfo) {

                       return;
                   }

                   this._getTileData(tile, tiledImage, textureInfo, overallMatrix, indexInDrawArray, texturePositionArray, textureDataArray, matrixArray, opacityArray);

Aha, there should be 'continue'. My bad, thanks for noticing.
—
Reply to this email directly, view it on GitHub
https://github.com/openseadragon/openseadragon/pull/2407#discussion_r1858505993,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC7LYHIKITF7QHGIO6YUM3L2CRW3XAVCNFSM6AAAAAA4P5324KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINRRGQ4DCNBXGY
.
You are receiving this because you were mentioned.Message ID:
@.***>

I did not mean 'just' continue, but to contiue the routine. I returned the code to the previous state, but I don't like the
	let indexInDrawArray = tileIndex % maxTextures;
    let numTilesToDraw =  indexInDrawArray + 1;

calls and then doing nothig, is this really desirable?

Also one note: I would strive for merging this as soon as possible, and fixing further design goals in subsequent PRs. It is harder and harder to manage this, so I would focus primarily on fixing bugs we manage to identify.

I tried to write them in the documentation. The new entries include migration documents.

Yes, that's awesome! We still need them and changelog form as well. Normally I write the changelogs, but in this case it seems like it would be good for you to write the first draft since you have a much better handle on what all has changed.

Thank you @pearcetm for your code review! It's good to have these discussions.
@Aiosa I hear you on the need to get this merged soon. Specifically, minimizing the number of other merges before this one.
We will want to release 5.0.1 before this is merged, to get those changes out. After this PR is merged, we'll want a good amount of bake time before releasing it.
The one thing we're anticipating to merge before 5.0.1 is https://github.com/openseadragon/openseadragon/pull/2614, or something like it. Hopefully that won't be too disruptive.
I think one thing that would be good as we prep for merging is to start a collection of post-merge plans. That way we don't have to feel like we need to get everything perfect before the merge, and we're on the same page about how to move forward.

I pulled this branch and started to poke around at it, but I'm afraid there's just too much complexity here for me to propose any changes that address my design concerns without breaking the whole thing again.
I guess I am fine with merging this soon, once the parts we already know will be buggy are fixed up. (I've flagged those above where ever I spotted things I think would cause problems).
Reviewing the code was giving me flashbacks to the process of disentangling the tile drawing pipeline while building the WebGLDrawer. There was drawing-related code in like 5 different files. Here it feels like there is caching-related code in many places too (TiledImage -> Tile -> Cache -> Drawer -> Cache again etc to figure out what the state of the system is). Maybe I'll be able to take a crack at simplifying/refactoring some of this once the PR is merged.

Normally I write the changelogs, but in this case it seems like it would be good for you to write the first draft

Will do.

Here it feels like there is caching-related code in many places too (TiledImage -> Tile -> Cache -> Drawer -> Cache

There might be usages of cache system on other places, but I don't think the cache is managed anywhere but at the tile + cache logics. This could be further cleared up. On the other hand, this PR cleans quite a lot of code. It removes all the drawing properties from a tile reference, and splits the logic. It clearly separates parts of the system that draw, that modify data (plugins) and that download data (TileSources). TiledImage could be freed from some of the tile initialization logics, that's true, but I think this PR actually brings cleaner design. Yes, there are complex parts in the cache, but these carry the complex logics of asynchronous code interacting with synchronous drawing routine.

I think one thing that would be good as we prep for merging is to start a collection of post-merge plans.

Solve priority processing. We can process only lastDrawn + cutoff tiles immediatelly. We can set other tiles state back to 'loading' and process them later.
Solve conversion cost evaluation. RN it is just a number developer adds to the conversion function. We could instead do a performance conversion test and assign numbers from this test (gray data / noise data processed by the conversion, by different sizes, and compute score)
Solve asynchronous VS synchronous conversions. It might be useful to do async type conversions on the invalidation pipeline, and potentially let drawer convert its data (probably even by a choice) either in the invalidation process (if the conversion is costly) or when the viewer starts drawing (in a synchronous way, e.g. webgl uploading these textures). The only thing I am afraid of is that this could bring unavoidable anination lags, since there would come a frame where no tiles are ready to be drawn.
Maybe try to move around some of the logics to different spaces

This implementation still tries to minimize the structure changes, so if some processing was done at TiledImage, it is still there.

I think the things I was concerned would cause new bugs and/or reversions of previously fixed bugs have been addressed. So I guess we can merge it, and address anything that comes up in subsequent patches.
When does the new API become "official" and require deprecations if changed in the future? At the next major release? I.e., how long do we have to make potential adjustments before public-facing APIs are locked in?

I would say a new major version. Though we could also release it on this version, since the PR also brings back some compatibility options for the Webgl drawer (that does not work with most plugins that modify the tile data). Needless to say the plugins have to change too, so it is not literally a compatibility.

I would say the new API isn't official until we do a release, and yes, it'll definitely be a major version (6.0.0). I figure we'll give it a few months of bake time after this PR is merged, just like we did with the WebGL Drawer.

Needless to say the plugins have to change too, so it is not literally a (backwards) compatibility.

I don't see any breaking changes in the changelog... I guess those still need to be added? The current changelog gives the impression that all of the existing plugins will work without any modification.

I don't see any breaking changes in the changelog... I guess those still need to be added? The current changelog gives the impression that all of the existing plugins will work without any modification.

This is a good point. Even though the old filtering plugin and other related things don't work in the WebGLDrawer, they can still be used with CanvasDrawer so there was still backwards compatibility. If these will be broken with the new cache system and can't even be used with CanvasDrawer, that is definitely a new breaking change.

I don't see any breaking changes in the changelog... I guess those still need to be added? The current changelog gives the impression that all of the existing plugins will work without any modification.

Strictly speaking, I've tested two plugins that touch tile data, and both still work. If they stick with Context2D renderer. They only get tons of deprecation warnings. I am not 100% sure they work without issues, because they rely on synchronous updates (which should work since the only conversion that happens is when tile is downloaded), so in theory there is no breaking behavior. And there is also supportsAsync option which can remove async execution to ensure everything is updated synchronously (+ some convertors won't work and throw error since they check async is enabled).

Great to know! That's actually a really impressive feat... congratulations on pulling it off, @Aiosa!
Of course we'll keep an eye on things after the merge and see if there's anything else that needs documenting or fixing.
My plan is to release 5.0.1 on Monday and merge this after that. It's exciting to have gotten to this point!

@Aiosa Do you want to add an acknowledgement for BBMRI-ERIC to the README?

BTW, it looks like this patch adds 38 TODOs to the code. Are these things to work on once it's merged?
Also, I ran the tests a few times with and without this patch. Looks like this patch adds about 10 seconds to the test run, which is totally reasonable, considering all the new tests!

I will double-check. But I doubt I added 38 todos, there were plenty of todos in the code alteady.

Well, on master there are 39 hits if I search for "todo" (non-case sensitive, not just whole words) and on RationAI-cache-overhaul-reviewed there are 77. Turns out there are 32 in demo-bundle.js, though, so it's really just 6 new real ones. 😄

Okay, 5.0.1 is released! Shall we merge this?
There's a little merge conflict... I assume it's the changelog; can you fix that @Aiosa? Also, add a note about BBMRI-ERIC in the README?
Anything else before we merge it?

I Will double check todos. And try one more old plugin demo. Also, should I remove the old plugin demos before merging?

I Will double check todos. And try one more old plugin demo.

Great!

Also, should I remove the old plugin demos before merging?

It would probably be nice to remove the cutter, if you don't think they are needed.

Tried custom plugin, since I could not find one that tests tile-drawing + context2D property. Seems to be also ok.
Noticed that rendered is not deprecated in the event, is it desiable?
get rendered () {
    $.console.warn("[tile-drawing] rendered property is deprecated. Use invalidation routine instead.");
    return rendered;
}

On one hand, they get the data snychronously to modify before being drawn with canvas drawer, but.. is it deisrable? it potentially intervenes with the invalidation routine (e.g. it could modify the tile original data). On the other hand, we can see it as a feature of canvas drawer, with possible impacts on the system that have to be just well documented...

Few todos cleaned, does the 'sponsors' part look OK? Should I also add acknowledgement to Standford?

Though I was not sure where to put the logo, so I put it in the test/data/assets folder

Noticed that rendered is not deprecated in the event, is it desiable?

I guess it's not too big of a deal since it's just a special situation for the canvas renderer. On the other hand, we do want to be encouraging people to be using the new pipeline. Is there any downside to using the new pipeline instead of rendered? Is there any reason to use tile-drawing without using rendered?

Is there any reason to use tile-drawing without using rendered?

Probably no, but one can argue that if they stick to Canvas renderer & do not use any plugins, they can directly modify the data in a synchronous way, in the fashion it was being done before. Since the default renderer is WebGL and tile-loaded event will scream at you if you try to modify the retrieved data, I think the system already pushes users to go with the invalidation routine anyway. So RN I would say keep it, but warn users that using this has consequences - they lose interoperability with different drawers and plugins.

So RN I would say keep it, but warn users that using this has consequences - they lose interoperability with different drawers and plugins.

Sounds good. We can always deprecate it later if we want to.

@Aiosa Thanks for fixing up the sponsor bit. Looks like we're ready to merge! Do you agree?

Yes!

@Aiosa Congrats on getting this massive PR all the way here!

Thanks! There will be issues no doubt, I hope people will start testing it thoroughly once it is merged.

I've started a discussion for people to try the new system out: https://github.com/openseadragon/openseadragon/discussions/2637
@Aiosa @pearcetm Anything to add to it?