Fix image source performance regression #11564
Conversation
ryanhamley
added
the
performance ⚡
There was a problem hiding this comment.
The PR looks good! I have a few questions before approving.
Could you confirm that #10685 is not reproducible after this PR is merged?
most newer browsers will create an ImageBitmap
Can we have a list of which ones would go through that code path to have an idea of the impact?
| this.image = browser.getImageData(image); | ||
| const {HTMLImageElement} = window; | ||
| if (image instanceof HTMLImageElement) { | ||
| this.image = browser.getImageData(image); |
There was a problem hiding this comment.
Not a blocker for this PR, but for this code path, should we investigate caching of canvas creation? We could reuse and retain a canvas once created until we need to use it for an image that has larger dimension. So that once the memory is there we would try to reuse it. For example:
- Create image of size 1024x1024, create canvas of size 1024x1024
- Create image of size 512x512, reuse canvas of size 1024x1024
- Create image of size 2048x2048, drop canvas of size 1024x1024, create new canvas of size 2048x2048
...
One case where this would work great is where all images are of the same size. At the moment, we're always re-requesting new canvas even when sizes are matching.
There was a problem hiding this comment.
I have an implementation of this that seems to work well and looks like it's fairly performant.
It's not reproducible
|
src/util/browser.js
Outdated
| canvas.width = width; | ||
| canvas.height = height; |
There was a problem hiding this comment.
Do you know why this commit didn't work out? 9d70bca
if (!canvas.width || canvas.width < width || !canvas.height || canvas.height < height) {
canvas.width = width;
canvas.height = height;
}
I'm concerned about these downsizing the canvas unnecessarily. Have you tried:
canvas.width = width > canvas.width ? width : canvas.width;
src/util/browser.js
Outdated
|
|
||
| canvas.width = width > canvas.width ? width : canvas.width; | ||
| canvas.height = height > canvas.height ? height : canvas.height; | ||
|
|
||
| context.drawImage(img, 0, 0, width, height); | ||
| return context.getImageData(-padding, -padding, width + 2 * padding, height + 2 * padding); |
There was a problem hiding this comment.
Following up from comment #11564 (comment), I think that would be the right way to resize:
| canvas.width = width > canvas.width ? width : canvas.width; | |
| canvas.height = height > canvas.height ? height : canvas.height; | |
| context.drawImage(img, 0, 0, width, height); | |
| return context.getImageData(-padding, -padding, width + 2 * padding, height + 2 * padding); | |
| if (width > canvas.width || height > canvas.height) { | |
| canvas.width = width; | |
| canvas.height = height; | |
| } | |
| context.clearRect(-padding, -padding, width + 2 * padding, height + 2 * padding); | |
| context.drawImage(img, 0, 0, width, height); | |
| return context.getImageData(-padding, -padding, width + 2 * padding, height + 2 * padding); |
The canvas might be dropped as soon as we resize it, to retain it we still need the condition but have a clear on the region where we'd like to use it. The above suggestion fixes that issue with proper caching.
|
Looks good to me pending the caching behavior fix mentioned in #11564 (comment). @sienki-jenki could you follow up to #11559 (comment) and confirm that this PR fixes the issue that you observed in your environment? |
Launch Checklist
texture.updaterather than always creating a new texture when the source's image is updatedonRemovethat destroys the texture to prevent a memory leak@mapbox/map-design-team@mapbox/static-apisif this PR includes style spec API or visual changes@mapbox/gl-nativeif this PR includes shader changes or needs a native portmapbox-gl-jschangelog:<changelog>Prevent performance regression when animating image sources</changelog>