Fix Raster DEM decoding in safari private browsing mode

[msbarry]

Fix #3110 by conditionally using a webcodecs VideoFrame API to get pixels from terrain RGB images when the browser mangles images to mitigate fingerprinting. This makes hillshading work for me on iOS 17 with enhanced privacy protections.

I think we should still prefer OffscreenCanvas.getImageData when it works since the image can come back in many formats, and we only handle RGB/BGR here. There is a proposal to ask the browser to convert pixel formats for us: w3c/webcodecs#92 - maybe when that's implemented more broadly we can use this by default when available.

I added console.log timing the performance of this method, and it takes around 0.5-1ms for a 512px tile on desktop chrome (M1 macbook pro) and about 5-10ms on an iPhone 14.

terrain before	terrain after	contours before	contours after

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.

[codecov-commenter]

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (092c5de) 75.11% compared to head (7660598) 75.20%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3185      +/-   ##
==========================================
+ Coverage   75.11%   75.20%   +0.08%     
==========================================
  Files         240      241       +1     
  Lines       19171    19243      +72     
  Branches     4325     4337      +12     
==========================================
+ Hits        14401    14472      +71     
- Misses       4770     4771       +1     

Files	Coverage Δ
src/source/raster_dem_tile_worker_source.ts	95.45% <85.71%> (+31.16%)	⬆️
src/util/offscreen_canvas_distorted.ts	89.47% <89.47%> (ø)
src/util/util.ts	92.82% <88.00%> (-1.30%)	⬇️
src/source/raster_dem_tile_source.ts	49.43% <25.00%> (-3.07%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[birkskyum]

Will be interesting to see if this approach also fixes a longstanding issue with spikes in the terrain mesh that in some circumstances show up in the Brave browser. It is a quite extreme privacy browser, so the cause could even be related.

• Artifacts in WebGL mesh brave/brave-browser#25149

[HarelM]

I would consider it as a different PR...?
We need to measure the performance impact here I believe...

[msbarry]

I would consider it as a different PR...? We need to measure the performance impact here I believe...

Good point, we could limit scope to just raster dem tile source, and only when offscreen canvas is distorted?

-                const rawImageData = transfer ? img : browser.getImageData(img, 1);
+                const rawImageData = transfer ? img :
+                    isOffscreenCanvasDistorted() ? new RGBAImage({width: img.width + 2, height: img.height + 2}, await readImageUsingVideoFrame(img, -1, -1, img.width + 2, img.height + 2)) :
+                        browser.getImageData(img, 1);

[msbarry]

I applied that change in f12470c - I think it should have minimal performance implication, and only change behavior when the image gets mangled but let me know if you have any concerns.

[msbarry]

we can't support both safari 10 and 17 in the same version, it's too much...

Another datapoint here is that Apple's support for Safari version 10 ended August 2017 according to https://en.wikipedia.org/wiki/OS_X_Yosemite

[birkskyum]

We'll still be supporting Safari >= 10.1 , so it's actually only 10.0 that's unsupported.

[msbarry]

Ugh unfortunately it looks like that pushed this PR just over the 1024 byte increase limit from 770837 - should I bump it?

Looks like the reason is that by using readImageUsingVideoFrame from both the main thread and worker thread, it pulls the definition up into the shared chunk and the main and worker chunk refer to it by t.readImageUsingVideoFrame in the minified output. Previously when it was just defined in the worker chunk the minifier could shorten the function name. I wonder if there's a way for rollup to minify names shared across the chunks...?

[msbarry]

I wonder if there's a way for rollup to minify names shared across the chunks...?

Yep... there sure is:

--- a/rollup.config.ts
+++ b/rollup.config.ts
@@ -22,7 +22,8 @@ const config: RollupOptions[] = [{
         format: 'amd',
         sourcemap: 'inline',
         indent: false,
-        chunkFileNames: 'shared.js'
+        chunkFileNames: 'shared.js',
+        minifyInternalExports: production
     },
     onwarn: (message) => {
         console.error(message);

That brings this from a 1220 byte increase to a 9397 decrease (-1.7kb gzipped). Think we should pursue that?

[HarelM]

Yes, but not in this PR.
Let's have this merged, including size increase, release a version and have that in the next version so that we'll have a way to pinpoint issues that these two separate features can create...

[msbarry]

Sounds good, I bumped the quota and opened #3194 to track.

[HarelM]

I've added a few last comments, overall, this looks good. THANKS!!!

[HarelM]

If we are changing this method can we also change how we call it?
I don't like the imageLoaded.bind(this) part.
I also don't know if ImageRequest.getImage can handle a promise as a return type...?

[msbarry]

Sounds good, I changed it a bit to inline imageLoaded like image_source does it. I also noticed that expiry got broken-out into a 3rd argument so it couldn't have been working as-is but should be fixed now.

What I'd really like to do is change the entire function to use async/await without any callbacks, something like

try {
  tile.request = ImageRequest.getImage(request, this.map._refreshExpiredTiles);
  const image = await tile.request.image;
  const transfer = isImageBitmap(img) && offscreenCanvasSupported();
  const rawImageData = transfer ? img : await readImageNow(img);
  const params = { ... };
  if (!tile.actor || tile.state === 'expired') {
      tile.actor = this.dispatcher.getActor();
      const data = await tile.actor.send('loadDEMTile', params);
      if (data) {
          tile.dem = data;
          tile.needsHillshadePrepare = true;
          tile.needsTerrainPrepare = true;
          tile.state = 'loaded';
          callback(null);
      }
  }
} catch (err) {
  tile.state = 'errored';
  callback(err);
}

But that would require some larger refactoring to ImageRequest and the actor API. I implemented a similar API in maplibre-contour, except all promise-based, we might want to use some utilities to make promise-based code a little easier to write here, like CancelablePromise and a promise-based actor implementation that checks the remote method being called and its argument types when you do actor.send('method', args); but that's a larger project.

[HarelM]

Yes, I would like that too, there was a PR that started it, which has problems with the actor stuff, so your solution might be the missing part: #2121.
Cc: @smellyshovel

[HarelM]

P.S. if you would like to have a version released, please open a PR with version bump and I'll merge and carve out a release.
Thanks for all the hard work invested here in this!!

[msbarry]

Thanks @HarelM ! Do you think we should do a release that potentially drops support for a small percentage of browsers because of async/await? Or @birkskyum should we try to get the transpiler working before a release?

[HarelM]

I'm OK with releasing it as is, and if there's a requirement later on to address this, the person/company that needs this can push it forward like you did in this case.

[birkskyum]

Just to give a bit of context. Compiling it down to ES2016 adds just 1 kb to maplibre-gl.js, so it's 772 -> 773 kb, so that concern is not too big - I don't have any number on any performance changes related to async/await.

For anyone interested in setting up the down-compilation, the most feasible path seems to be having 2 tsconfig.json files, one for all tests and build scripts with >= es2017 target (due to usage of top-level await), and one for library source files with <= es2016 target.

I'm also in favor of releasing it. And in either case, it'll be great to see how await/async can simplify other areas of the library.

[msbarry]

OK thanks, I opened #3197 to do a patch version bump. I double-checked onthegomap traffic over the last week and it looks like only about 0.02% of users would fall into the safari/chrome/firefox version range that supports webgl but not async/await so on my end I'm fine with this without transpilation.

[birkskyum]

This fixed Brave support as well 👍