Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there any reason we can't drop the HTML drawer? #2473

Open
pearcetm opened this issue Feb 12, 2024 · 14 comments
Open

Is there any reason we can't drop the HTML drawer? #2473

pearcetm opened this issue Feb 12, 2024 · 14 comments
Labels

Comments

@pearcetm
Copy link
Contributor

Per https://caniuse.com/canvas we really don't support any browsers that don't support canvas - what's the point of keeping the HTML drawer at this point? Can we just ditch it, or at least deprecate it?

@iangilman
Copy link
Member

The HTML drawer continues to be useful for people as a fallback when they have issues with the canvas drawer. The thing that comes to mind is that mobile Safari has issues with large canvasses, so sometimes the HTML drawer is better there. I'd have to dig through the issues to see the last time something like that was discussed.

It is definitely a special case thing though... It seems reasonable that we could be moving toward deprecating it somehow. Perhaps at some point it could be spun off as a plugin if there's sufficient interest. Certainly it doesn't need to support the new fancy features (most of which it can't support anyways).

Are there ways in which it's getting in the way of continued development? Or just feeling like dead weight?

@pearcetm
Copy link
Contributor Author

Are there ways in which it's getting in the way of continued development? Or just feeling like dead weight?

I was experimenting with it a bit on the drawer comparison demo and noticed some funky things. For example, the Duomo image at some magnifications has the bottom row misplaced:

image

When I try to load the "Blue B" image, I get the following in the console, and nothing shows up in the viewer: htmldrawer.js:202 [Drawer._drawTileToHTML] attempting to draw tile 9/0_0 when it's not cached

And if I've already navigated around an image with the CanvasDrawer and then swap to the HtmlDrawer, tiles that have already been "touched" some way or another won't render in html because tile.getImage(); returns null. I suspect @Aiosa may be able to shed light on this.

I don't think I touched any of the relevant code around the HTML drawer pipeline that would explain this, but I suppose it is possible something inadvertent happened. Some of these things I've observed make me wonder if some of the other recent improvement (to the cache, etc) might have already broken some of the HTML drawer abilities.

@Aiosa
Copy link
Contributor

Aiosa commented Feb 13, 2024

When a canvas object is retrieved, the original image data is thrown away to avoid duplicate copies in memory. The Image getter on the older way of 'advanced data pipeline' based on three functions on the tile sources, does not account for this scenario, so the accessed image data is gone. You would have to convert canvas to image here:

return cacheObject._data; //the data itself by default is Image

@Aiosa
Copy link
Contributor

Aiosa commented Feb 13, 2024

Otherwise I also don't see a reason for HTML drawer...

@iangilman
Copy link
Member

Well, with version 5 we have officially dropped support for IE11 and have added the WebGL drawer. Maybe it's time to let go of the HTML drawer as well. If people need the HTML drawer, they can fall back to version 4.1.0. That said, I wouldn't want to get rid of it entirely this version, but instead deprecate it.

@pearcetm
Copy link
Contributor Author

I looked into some of the older issues. My impression is that the "problems with canvas" aren't really with canvas, but with super large canvases. Even on desktop browsers, there are limits to how big a canvas can be. The size of the full window is never a problem. However, when images with tens of thousands of pixels per dimension are used as "tiles" this leads to enormous canvases being created (to represent the data in a canvas, rather than an Image).

Since there is no such size restriction on Images, and Image is a perfectly valid and performant way to draw onto a canvas, maybe a better approach would be to allow tiles to stay as Image but still use the WebGLDrawer or CanvasDrawer.

@Aiosa
Copy link
Contributor

Aiosa commented Feb 13, 2024

That's how it is implemented btw within cache overhaul - conversion is done only when needed, and drawers define which formats they accept. So if you start with image data and the renderer either supports these or knows how to convert to something it can use, it is preferred over canvas conversion. E.g. WebGL will convert 'image' to 'texture2D'.

@iangilman
Copy link
Member

Sounds good :-)

@joedf
Copy link
Contributor

joedf commented Feb 15, 2024

I think js-canvas is pretty well established at this point. The software-rendering works reasonably well even on modern everyday laptops with no discrete graphics. I've never used or needed the html-drawn viewer. I don't see much reason in keeping this if it is not worth maintaining. That said, I think deprecating it for a bit instead of completely removing it right way makes sense.

@Aiosa
Copy link
Contributor

Aiosa commented Feb 15, 2024

That would be because canvas is processed on CPU.. I think :D

@joedf
Copy link
Contributor

joedf commented Feb 15, 2024

Yes, precisely. DOM modification is actually quite costly and thus canvas is generally recommended (w3c) for more complex animations and such.

@pearcetm
Copy link
Contributor Author

Sounds like we're in agreement that deprecating the HTML drawer is a reasonable next step. Whether we wait for @Aiosa's cache overhaul PR or not, I think the root of the performance problems that people have been using the HTML drawer to get around is the need for a potentially massive canvas to represent each individual tile for the CanvasDrawer (or the equivalent before the recent drawer refactoring). In order for the existing CanvasDrawer to take advantage of the new caching behavior we'll need to make the pipeline stop converting all fetched images to canvases. It'd be great to test performance of just leaving the data in Image format vs the current behavior of converting to canvas. If there's no performance hit we can just stop converting to canvas. If there is, we can pick a size above which we don't do this (perhaps as a config option). Once that's done there should be no actual need for the HTML drawer.

@pearcetm
Copy link
Contributor Author

In order for the existing CanvasDrawer to take advantage of the new caching behavior we'll need to make the pipeline stop converting all fetched images to canvases.

Thinking more about this, in order to not break existing APIs that depend on context2d being available on the tiles, we'd have to pair such a change (even in CanvasDrawer) with a synchronous conversion from Image to canvas when the data is accessed (the result could be cached, of course). Otherwise this wouldn't really be backwards compatible.

@Aiosa
Copy link
Contributor

Aiosa commented Feb 19, 2024

Since we wanted to push v5 (at least I thought so) also with cache overhaul that deprecates context2d and other hack props, all these issues should resolve by themselves... Deprecation itself could also happen in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants