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

Make request for ImageSource cancelable #1802

Merged
merged 3 commits into from
Nov 30, 2022
Merged

Make request for ImageSource cancelable #1802

merged 3 commits into from
Nov 30, 2022

Conversation

blinkzz
Copy link
Contributor

@blinkzz blinkzz commented Nov 2, 2022

Added missing cancelation feature for ImageSource, this way any pending request for image will be canceled if updateImage is used, or if source is being removed
This closes #1798

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2022

Can you also take a look at this fix while you are touching the image source:
mapbox/mapbox-gl-js@50adf1c
This is a fix made in version 1.13.2 which I believe still has the "right" license.

@blinkzz
Copy link
Contributor Author

blinkzz commented Nov 2, 2022

@HarelM
Isn't it already backported?

function arrayBufferToImage(data: ArrayBuffer, callback: (err?: Error | null, image?: HTMLImageElement | null) => void) {
const img: HTMLImageElement = new Image();
img.onload = () => {
callback(null, img);
URL.revokeObjectURL(img.src);
// prevent image dataURI memory leak in Safari;
// but don't free the image immediately because it might be uploaded in the next frame
// https://github.com/mapbox/mapbox-gl-js/issues/10226
img.onload = null;
window.requestAnimationFrame(() => { img.src = transparentPngUrl; });
};
img.onerror = () => callback(new Error('Could not load image. Please make sure to use a supported image type such as PNG or JPEG. Note that SVGs are not supported.'));
const blob: Blob = new Blob([new Uint8Array(data)], {type: 'image/png'});
img.src = data.byteLength ? URL.createObjectURL(blob) : transparentPngUrl;
}

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

Bundle size report:

Size Change: +23 B
Total Size Before: 206 kB
Total Size After: 206 kB

Output file Before After Change
maplibre-gl.js 197 kB 197 kB +23 B
maplibre-gl.css 9.09 kB 9.09 kB 0 B
ℹ️ View Details
Source file Before After Change
src/source/image_source.ts 1.11 kB 1.14 kB +31 B
src/render/program/terrain_program.ts 695 B 707 B +12 B
src/render/program/heatmap_program.ts 558 B 566 B +8 B
src/render/program/fill_extrusion_program.ts 685 B 690 B +5 B
src/render/program/circle_program.ts 454 B 459 B +5 B
src/render/draw_background.ts 572 B 577 B +5 B
src/data/pos_attributes.ts 80 B 84 B +4 B
src/render/program/line_program.ts 1.16 kB 1.16 kB +4 B
src/render/draw_line.ts 1.05 kB 1.05 kB +4 B
src/util/web_worker.ts 39 B 42 B +3 B
src/shaders/fill.vertex.glsl.g.ts 171 B 174 B +3 B
src/shaders/hillshade.vertex.glsl.g.ts 136 B 139 B +3 B
src/render/program/fill_program.ts 570 B 573 B +3 B
src/ui/handler/tap_recognizer.ts 532 B 535 B +3 B
src/ui/control/geolocate_control.ts 2.24 kB 2.24 kB +3 B
src/source/source.ts 351 B 353 B +2 B
src/util/global_worker_pool.ts 319 B 321 B +2 B
src/shaders/background.fragment.glsl.g.ts 137 B 139 B +2 B
src/shaders/circle.vertex.glsl.g.ts 557 B 559 B +2 B
src/shaders/heatmap_texture.vertex.glsl.g.ts 143 B 145 B +2 B
src/shaders/collision_circle.fragment.glsl.g.ts 258 B 260 B +2 B
src/shaders/fill_outline_pattern.fragment.glsl.g.ts 418 B 420 B +2 B
src/shaders/fill_pattern.vertex.glsl.g.ts 387 B 389 B +2 B
src/shaders/fill_extrusion_pattern.fragment.glsl.g.ts 415 B 417 B +2 B
src/shaders/hillshade_prepare.vertex.glsl.g.ts 190 B 192 B +2 B
src/shaders/line_pattern.vertex.glsl.g.ts 787 B 789 B +2 B
src/shaders/symbol_icon.vertex.glsl.g.ts 947 B 949 B +2 B
src/shaders/symbol_text_and_icon.vertex.glsl.g.ts 1.01 kB 1.01 kB +2 B
src/shaders/terrain_coords.fragment.glsl.g.ts 168 B 170 B +2 B
src/render/program/symbol_program.ts 1.29 kB 1.29 kB +2 B
src/ui/hash.ts 935 B 937 B +2 B
src/data/pos3d_attributes.ts 82 B 84 B +2 B
src/ui/control/scale_control.ts 733 B 735 B +2 B
src/ui/control/terrain_control.ts 418 B 420 B +2 B
src/render/texture.ts 679 B 680 B +1 B
src/style/load_glyph_range.ts 222 B 223 B +1 B
src/style/light.ts 559 B 560 B +1 B
src/source/vector_tile_source.ts 1.1 kB 1.1 kB +1 B
src/util/offscreen_canvas_supported.ts 154 B 155 B +1 B
src/data/raster_bounds_attributes.ts 93 B 94 B +1 B
src/symbol/grid_index.ts 1.35 kB 1.35 kB +1 B
src/source/pixels_to_tile_units.ts 103 B 104 B +1 B
src/shaders/_prelude.vertex.glsl.g.ts 956 B 957 B +1 B
src/shaders/background_pattern.vertex.glsl.g.ts 224 B 225 B +1 B
src/shaders/clipping_mask.fragment.glsl.g.ts 59 B 60 B +1 B
src/shaders/heatmap.fragment.glsl.g.ts 266 B 267 B +1 B
src/shaders/collision_box.vertex.glsl.g.ts 315 B 316 B +1 B
src/shaders/debug.fragment.glsl.g.ts 147 B 148 B +1 B
src/shaders/debug.vertex.glsl.g.ts 161 B 162 B +1 B
src/shaders/fill_extrusion.vertex.glsl.g.ts 622 B 623 B +1 B
src/shaders/hillshade_prepare.fragment.glsl.g.ts 500 B 501 B +1 B
src/shaders/line.fragment.glsl.g.ts 324 B 325 B +1 B
src/shaders/line_gradient.fragment.glsl.g.ts 350 B 351 B +1 B
src/shaders/line_sdf.vertex.glsl.g.ts 808 B 809 B +1 B
src/gl/vertex_buffer.ts 608 B 609 B +1 B
src/gl/stencil_mode.ts 150 B 151 B +1 B
src/render/draw_symbol.ts 2.61 kB 2.61 kB +1 B
src/render/draw_fill_extrusion.ts 795 B 796 B +1 B
src/render/draw_hillshade.ts 1.15 kB 1.15 kB +1 B
src/render/draw_custom.ts 337 B 338 B +1 B
src/util/primitives.ts 728 B 729 B +1 B
src/ui/handler/keyboard.ts 569 B 570 B +1 B
src/ui/handler/scroll_zoom.ts 1.28 kB 1.28 kB +1 B
src/util/debug.ts 159 B 160 B +1 B
src/source/terrain_source_cache.ts 879 B 880 B +1 B
src/ui/marker.ts 2.87 kB 2.87 kB +1 B
src/ui/control/fullscreen_control.ts 783 B 784 B +1 B
src/style/style_image.ts 126 B 125 B -1 B
src/render/glyph_manager.ts 947 B 946 B -1 B
src/render/line_atlas.ts 984 B 983 B -1 B
src/source/raster_dem_tile_source.ts 971 B 970 B -1 B
src/source/canvas_source.ts 1.02 kB 1.02 kB -1 B
src/source/query_features.ts 1.22 kB 1.21 kB -1 B
src/source/source_cache.ts 3.99 kB 3.99 kB -1 B
src/util/worker_pool.ts 419 B 418 B -1 B
src/style-spec/diff.ts 1.54 kB 1.54 kB -1 B
src/symbol/path_interpolator.ts 311 B 310 B -1 B
src/symbol/collision_index.ts 1.64 kB 1.64 kB -1 B
src/style/pauseable_placement.ts 598 B 597 B -1 B
src/symbol/cross_tile_symbol_index.ts 1.37 kB 1.37 kB -1 B
src/style/style.ts 6.72 kB 6.72 kB -1 B
src/shaders/line_sdf.fragment.glsl.g.ts 460 B 459 B -1 B
src/render/vertex_array_object.ts 581 B 580 B -1 B
src/render/program/debug_program.ts 192 B 191 B -1 B
src/gl/value.ts 1.09 kB 1.09 kB -1 B
src/gl/color_mode.ts 174 B 173 B -1 B
src/gl/context.ts 1.28 kB 1.28 kB -1 B
src/render/draw_terrain.ts 1.75 kB 1.75 kB -1 B
src/geo/edge_insets.ts 431 B 430 B -1 B
node_modules/gl-matrix/esm/mat2.js 204 B 203 B -1 B
src/util/throttle.ts 145 B 144 B -1 B
src/ui/handler/touch_pan.ts 541 B 540 B -1 B
src/ui/handler/tap_drag_zoom.ts 482 B 481 B -1 B
src/ui/handler/shim/drag_pan.ts 214 B 213 B -1 B
src/ui/handler/shim/touch_zoom_rotate.ts 280 B 279 B -1 B
src/ui/handler_manager.ts 2.46 kB 2.46 kB -1 B
src/ui/camera.ts 3.41 kB 3.4 kB -1 B
src/render/terrain.ts 2.08 kB 2.08 kB -1 B
src/ui/map.ts 7.16 kB 7.16 kB -1 B
node_modules/@mapbox/tiny-sdf/index.js 1.1 kB 1.1 kB -2 B
src/shaders/_prelude.fragment.glsl.g.ts 121 B 119 B -2 B
src/shaders/circle.fragment.glsl.g.ts 410 B 408 B -2 B
src/shaders/heatmap_texture.fragment.glsl.g.ts 207 B 205 B -2 B
src/shaders/collision_box.fragment.glsl.g.ts 148 B 146 B -2 B
src/shaders/collision_circle.vertex.glsl.g.ts 547 B 545 B -2 B
src/shaders/fill.fragment.glsl.g.ts 178 B 176 B -2 B
src/shaders/fill_outline.vertex.glsl.g.ts 218 B 216 B -2 B
src/shaders/fill_outline_pattern.vertex.glsl.g.ts 415 B 413 B -2 B
src/shaders/fill_pattern.fragment.glsl.g.ts 396 B 394 B -2 B
src/shaders/fill_extrusion.fragment.glsl.g.ts 119 B 117 B -2 B
src/shaders/hillshade.fragment.glsl.g.ts 555 B 553 B -2 B
src/shaders/line.vertex.glsl.g.ts 709 B 707 B -2 B
src/shaders/line_pattern.fragment.glsl.g.ts 707 B 705 B -2 B
src/shaders/terrain.fragment.glsl.g.ts 112 B 110 B -2 B
src/render/program/clipping_mask_program.ts 106 B 104 B -2 B
src/gl/depth_mode.ts 135 B 133 B -2 B
src/render/draw_collision_debug.ts 1.14 kB 1.14 kB -2 B
src/ui/default_locale.ts 313 B 311 B -2 B
src/ui/control/navigation_control.ts 1.58 kB 1.58 kB -2 B
src/shaders/background.vertex.glsl.g.ts 106 B 103 B -3 B
src/shaders/clipping_mask.vertex.glsl.g.ts 106 B 103 B -3 B
src/shaders/heatmap.vertex.glsl.g.ts 369 B 366 B -3 B
src/shaders/fill_extrusion_pattern.vertex.glsl.g.ts 829 B 826 B -3 B
src/shaders/line_gradient.vertex.glsl.g.ts 739 B 736 B -3 B
src/render/program/collision_program.ts 724 B 721 B -3 B
src/render/draw_fill.ts 974 B 971 B -3 B
src/shaders/terrain.vertex.glsl.g.ts 222 B 218 B -4 B
src/shaders/shaders.ts 1.49 kB 1.48 kB -4 B
src/render/program/hillshade_program.ts 882 B 878 B -4 B
src/index.ts 883 B 878 B -5 B
src/render/draw_debug.ts 1.37 kB 1.36 kB -8 B
src/render/program/program_uniforms.ts 960 B 931 B -29 B

@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2022

Yea, this was backported, my mistake.
There is another stuck PR in this repo which I'm not sure what to do with related to image source:
#1323
Would be nice if you can take a look at it and let me know if this can be integrated as well as part of this PR since it seems like it's in the same location...

@blinkzz
Copy link
Contributor Author

blinkzz commented Nov 2, 2022

@HarelM
It looks like solution for #1323 was influenced by fix introduced in mapbox@2.11.0-beta.1 (but we can assume that this is the only viable solution, since it also being used in other source types too)
But i think both this PR and mapbox solutions could be conditional based on if current browser is Safari (using isSafari from src/util/util), to prevent unnecessary image conversions for other browsers and therefore performance drop

@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2022

Sounds right I guess... We should make sure we don't copy form mapbox though...

@HarelM
Copy link
Collaborator

HarelM commented Nov 8, 2022

@blinkzz The code look good, thanks!
Let me know if you would like to push forward the other fix as well, maybe as part of a different PR.

@blinkzz
Copy link
Contributor Author

blinkzz commented Nov 8, 2022

@HarelM, will have a look on it for sure, but definetly as separate PR

@HarelM
Copy link
Collaborator

HarelM commented Nov 30, 2022

@blinkzz what's missing in order to merge this besides resolving conflicts?

@blinkzz
Copy link
Contributor Author

blinkzz commented Nov 30, 2022

@HarelM, this PR is for sure 100% complete, i had no time to check another PR we've discussed above, but it looks good to merge tho without any additional changes

@HarelM HarelM merged commit 018f6ec into maplibre:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImageSource's .updateImage() while loading
2 participants