Use createImageBitmap when supported #650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Bundle size report: Size Change: -32 B
ℹ️ View Details
|
|
Is this the only place that uses the offscreen camvas check? Can you think why offscreen was checked here? |
|
No, the OffscreenCanvas check is also used in the raster_dem_tile_source but in this case the check is relevant since the feature is used in the worker. For the getImage function however, I don't see any reason to check the OffscreenCanvas support here and to be reassured mapbox-gl-js relaxed the check too in this commit. Regarding the ImageBitmap support check that you wrote, you may be referring to this commit. I did not find any other occurrence of createImageBitmap in the code. |
|
Thanks for the clarification, in that case my comments are the following:
I got confused with Thanks! |
clementgayvallet
force-pushed
the
image-bitmap-support
branch
from
80893b7
to
fc980f9
Compare
|
|
Thanks!! Can we add some tests to this behavior? |
|
I just tried but it wasn't very convincing. The problem is that it's browser dependent and during unit test the createImageBitmap is not defined. We can mock it with something like Do you have any idea on how to test this? |
|
You can override |
clementgayvallet
force-pushed
the
image-bitmap-support
branch
from
fc980f9
to
24732b0
Compare
|
Change log entry? Please also tic the relevant checkboxes in the pr template to make sure this is not copied etc... |
clementgayvallet
force-pushed
the
image-bitmap-support
branch
from
24732b0
to
a3d3b24
Compare
HarelM
approved these changes
Nov 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When converting a byte buffer to a CanvasImageSource, maplibre uses either an ImageBitmap if supported or an HTMLImageElement as fallback.
HTMLImageElement has many flaws, such as the transparent pixel hack to avoid memory leaks. Therefore the ImageBitmap seems more reliable and is the preferred image source.
In order to load the byte buffer to an ImageBitmap the library checks if the feature is available on the browser. Currently the function checks if the function createImageBitmap is defined and if the OffscreenCanvas feature is available.
However the OffscreenCanvas is not required to create an ImageBitmap and on browsers like Firefox where createImageBitmap is supported but not OffscreenCanvas (which is still experimental), the image source falls back on HTMLImageElement.
This PR simply checks for the availability of the createImageBitmap function instead of the unneeded OffscreenCanvas feature.
Launch Checklist
maplibre-gl-jschangelog:<changelog>getImage uses createImageBitmap when supported (#650)</changelog>.