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

Handle loading of empty raster tiles (204 No Content) #3428

Merged
merged 3 commits into from
Dec 3, 2023

Conversation

petrsloup
Copy link
Contributor

@petrsloup petrsloup commented Nov 30, 2023

This PR tries to solve the issue of multiple errors in case of loading raster tiles from server responding 204s with no content.

Related to #160 and #1586, but a different approach.

There are currently two ways of loading the image data (ArrayBuffer):

  • arrayBufferToImage ("legacy" way which uses Image under the hood)
  • the newer arrayBufferToImageBitmap (which requires createImageBitmap).

The legacy path has always been able to handle 204s correctly by load a transparent image instead:

img.src = data.byteLength ? URL.createObjectURL(blob) : transparentPngUrl;

There is no such check for the ImageBitmap version and it just fails with an error (it's not as easy to mock the transparent image here).

This PR adds a simple check so that empty ArrayBuffers (= 204 no content responses) always use the legacy path.

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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@HarelM
Copy link
Collaborator

HarelM commented Nov 30, 2023

Thanks for this PR! This is a very elegant solution.
Two questions:

  1. Can't arrayBufferToImageBitmap solve this in a more elegant way? creating a "empty bitmap image"?
  2. Can you add tests to this flow?

@HarelM HarelM mentioned this pull request Dec 1, 2023
4 tasks
@petrsloup
Copy link
Contributor Author

@HarelM I think the solution is now even cleaner -- both approaches are now able to handle empty buffers. (Turns out we can createImageBitmap with empty ImageData quite easily.)

I was looking into testing this flow, but since the whole createImageBitmap is not mocked inside the tests, it effectively does not matter if the buffer is empty or not, so this path is actually tested with the existing unit tests as well.

This test case actually uses an empty buffer:

test('getImage uses createImageBitmap when supported', async () => {
server.respondWith(request => request.respond(200, {'Content-Type': 'image/png',
'Cache-Control': 'cache',
'Expires': 'expires'}, ''));

But it happens to work, because the mocked createImageBitmap creates empty ImageBitmap without any content regardless of the buffer content:

stubAjaxGetImage(() => Promise.resolve(new ImageBitmap()));

@HarelM
Copy link
Collaborator

HarelM commented Dec 1, 2023

cc: @ibesora can you take a look at this?
The following PR also does some work in this area, please let me know how relevant it will be after this PR is merged.

  • Better raster 204 handling #2325
    If it is not relevant anymore, I'll close this other one.
    Overall, making the behavior of createImage and the other flow identical is a good solution from my perspective.
    THANKS!

Please add a changelog entry.

@petrsloup petrsloup changed the title Handle empty raster tiles Handle loading of empty raster tiles (204 No Content) Dec 1, 2023
@HarelM HarelM enabled auto-merge (squash) December 3, 2023 10:22
@HarelM HarelM merged commit 4ad19a6 into maplibre:main Dec 3, 2023
14 checks passed
@petrsloup petrsloup deleted the patch-1 branch December 4, 2023 15:55
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.

2 participants