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

Hillshade and 3d terrain look wrong in iOS 17 private browsing mode #3110

Closed
msbarry opened this issue Sep 23, 2023 · 28 comments · Fixed by #3185
Closed

Hillshade and 3d terrain look wrong in iOS 17 private browsing mode #3110

msbarry opened this issue Sep 23, 2023 · 28 comments · Fixed by #3185
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed

Comments

@msbarry
Copy link
Contributor

msbarry commented Sep 23, 2023

I upgraded my iPhone 15 to iOS 17.0.1 and a few of the maplibre 3.3.1 examples using terrain look wrong in safari private browsing mode:

https://maplibre.org/maplibre-gl-js/docs/examples/3d-terrain/

https://maplibre.org/maplibre-gl-js/docs/examples/contour-lines/

And I occasionally get this warning at the top of the page:

The contour line problem is my issue over in the maplibre-contour plugin but hillshading and terrain are maplibre-gl-js.

I suspect that it's related to this line from safari 17 release notes:

Added noise to fingerprintable web APIs. (99360413)

@HarelM
Copy link
Member

HarelM commented Sep 23, 2023

I hate safari...
I have no clue where to start looking in order to solve this...
Nevertheless, this needs to be addressed.
Thanks for taking the time to report this!

@HarelM HarelM added bug Something isn't working PR is more than welcomed Extra attention is needed labels Sep 23, 2023
@msbarry
Copy link
Contributor Author

msbarry commented Sep 23, 2023

I'm pretty sure the codepath on modern browsers like safari is that it uses createImageBitmap from the raw image blob then does this to get the image pixels:

getImageData(imgBitmap: ImageBitmap): RGBAImage {
// Lazily initialize OffscreenCanvas
if (!this.offscreenCanvas || !this.offscreenCanvasContext) {
// Dem tiles are typically 256x256
this.offscreenCanvas = new OffscreenCanvas(imgBitmap.width, imgBitmap.height);
this.offscreenCanvasContext = this.offscreenCanvas.getContext('2d', {willReadFrequently: true});
}
this.offscreenCanvas.width = imgBitmap.width;
this.offscreenCanvas.height = imgBitmap.height;
this.offscreenCanvasContext.drawImage(imgBitmap, 0, 0, imgBitmap.width, imgBitmap.height);
// Insert an additional 1px padding around the image to allow backfilling for neighboring data.
const imgData = this.offscreenCanvasContext.getImageData(-1, -1, imgBitmap.width + 2, imgBitmap.height + 2);
this.offscreenCanvasContext.clearRect(0, 0, this.offscreenCanvas.width, this.offscreenCanvas.height);
return new RGBAImage({width: imgData.width, height: imgData.height}, imgData.data);
}

Which is the same thing that sketchy websites will use to fingerprint your browser since different elements will draw to canvas differently depending on your system fonts settings, etc.

A worst-case fallback would be decoding the image pixels using something like pngjs, but maybe there's a modern alternative for getting image pixels that doesn't require a canvas?

@HarelM
Copy link
Member

HarelM commented Sep 23, 2023

I think pngjs works in node environment and doesn't work in the browser the last time I checked...
When fetching an image the following code is used depending on the support of createImageBitmap:

export function arrayBufferToImage(data: ArrayBuffer, callback: (err?: Error | null, image?: HTMLImageElement | null) => void) {

And here:
https://github.com/maplibre/maplibre-gl-js/blob/5fa5b73c4a3a2e95b01c22305579ffe1d23472e9/src/util/util.ts#L484C3-L484C3

Maybe there's a need to update the code in the raster dem worker source, IDK...
It's different since it uses the offscreen canvas, maybe it should do it only if createImageBitmap is not supported, IDK...
I'm still not sure I understand where the bug is coming from or how to properly solve it...

@msbarry
Copy link
Contributor Author

msbarry commented Sep 24, 2023

Comparing safari remote debugger and chrome, I can reproduce the difference:

Safari private browsing mode remote debugger:

image

Chrome:

image

I can't isolate whether it's getImageData, or writing the image to canvas, but the entire end to end operation appears to have noise thrown in that wouldn't be perceptible in a visual image, but leads to hundreds of meters of difference with encoded elevations.

@jleedev
Copy link
Contributor

jleedev commented Sep 24, 2023

Firefox has a similar option under privacy.resistFingerprinting.

image-1

Somewhat different in that it seems to replace the image with completely random noise, and also is only enabled by default in the likes of Tor Browser.

There really ought to be a web api to fetch the pixels from a raster jpg/png/etc. separate from being able to observe the subtleties of how svg/canvas/webgl are rasterized.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 24, 2023

One issue is that the current mapbox/terrarium encodings encode elevation in a way that's not tied to visual perception. With a different encoding where the 2 are more correlated, imperceptible visual noise would only lead to imperceptible elevation noise, for example something like:

        32km    256m    1m      4mm
          |      |       |       |
  red bit 0  1  2  3  4  5  6  7  
 blue bit  0  1  2  3  4  5  6  7
green bit   0  1  2  3  4  5  6  7

so that +/- 2 on any channel only leads to a sub-1m elevation difference.

That wouldn't help the firefox/brave/tor browser use-case, but it would help safari and also potentially let us use lossy techniques to compress DEM images.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 25, 2023

I think the best outcome here is a browser API for getting pixels from an image in a way that can't be used to fingerprint a user. Is anyone aware of an upcoming standard for this?

In the short term, it looks like there are a few png decoders that work in the browser:

The biggest component to lib size is that they need to decompress data using inflate. There's a modern way to do that without any dependencies now: https://developer.mozilla.org/en-US/docs/Web/API/DecompressionStream so the only part left is decoding the png. This approach is probably slower than canvas approach so we'd probably want to prefer that when available. It seems like it would be tricky to tell if the canvas-based approach isn't working properly? Maybe something like decode a known image and if a checksum doesn't match what we expect then use png-decode approach?

Another thing that might help is DEM PNG's only use a subset of png functionality (ie. no alpha) so a simplified reader that doesn't handle all cases might be sufficient

@jleedev
Copy link
Contributor

jleedev commented Sep 25, 2023

If DecompressionStream isn't available then https://www.npmjs.com/package/fflate is a good alternative. Of course, the main use case for this iOS 17 only, so.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 25, 2023

If DecompressionStream isn't available then https://www.npmjs.com/package/fflate is a good alternative. Of course, the main use case for this iOS 17 only, so.

Does terrain/hillshading work at all in Firefox brave or tor browser? Seems like we should ideally try to find a solution that fixes those as well.

@birkskyum
Copy link
Member

macOS 14 (sonoma) was released... same issue on safari private desktop.

Screenshot 2023-09-26 at 22 57 08

@msbarry
Copy link
Contributor Author

msbarry commented Sep 27, 2023

Not sure if this is related but I'm also seeing a lot more webgl context lost events since upgrading to iOS 17 - in private browsing and regular safari.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 28, 2023

I put together a proof of concept decoding png in the browser forked from https://github.com/arian/pngjs using DecompressionStream - it appears to work, although it's slower than native canvas based approach. It's about 6kb raw/2.3K gzipped (all the code might not be needed for dem parsing though). Not sure it works in all cases, but it at least looks to be possible:

https://gist.github.com/msbarry/ca3f8193186b591560456d4cb3f45d19

@HarelM
Copy link
Member

HarelM commented Sep 28, 2023

Besides the above direction, do we have any alternative? other browser API we can use? some flags around avoiding the noise?
I'll be surprised if this affects only this library...

@msbarry
Copy link
Contributor Author

msbarry commented Sep 29, 2023

OK here we go... WebCodec is the upcoming standard, it has an ImageDecoder API that's meant for decoding images/videos/audio through javascript:

const response = await fetch('https://elevation-tiles-prod.s3.amazonaws.com/terrarium/0/0/0.png');
const imageDecoder = new ImageDecoder({ data: response.body, type: 'image/png' });
const image = await imageDecoder.decode();
const buffer = new Uint8Array(image.image.allocationSize());
image.image.copyTo(buffer);
console.log([
  buffer[(100 + 256 * 100) * 4],
  buffer[(100 + 256 * 100) * 4 + 1],
  buffer[(100 + 256 * 100) * 4 + 2],
  buffer[(100 + 256 * 100) * 4 + 3],
]);

Unfortunately it's not supported yet in safari, firefox, or iOS safari https://caniuse.com/mdn-api_imagedecoder

@msbarry
Copy link
Contributor Author

msbarry commented Sep 29, 2023

Another option is moving to another custom format for DEM that doesn't require decoding images. For example if you line up all of the elevation values in a tile then apply delta + zig-zag + varint + gzip encoding, the resulting tiles end up somewhere between png and webp sizing (webp is 20-40% smaller than png, this is closer to png for low-zoom tiles and closer to webp for higher zooms). There are probably smarter ways to do that - I was talking with @biodranik earlier this year and he had some ideas about more compact DEM encoding formats for Organic Maps.

So I think our paths forward are:

  safari firefox / tor brave downsides
bug safari team about this ⚠️
(maybe)
🚫 🚫  no guarantee they will address
noise-resistant png encoding ⚠️
(less bad)
🚫 🚫 people need to generate new tilesets, tiles will likely be larger
js png decoder slower, +6kb (2kb gzipped) bundle size, difficult to detect when needed
custom non-image encoding people need to generate new tilesets, not as compact as webp
ImageDecoder ⚠️
(eventually) 
 ⚠️
(eventually)
 ✅  not enough browser support yet

@HarelM
Copy link
Member

HarelM commented Sep 29, 2023

Seems like this is somewhat related to this discussion:
maplibre/maplibre-style-spec#326 (comment)

I generally agree the terrainRGB format can probably be improved in terms of compression/size.

But it will take a long time move things to a different format, I believe, due to tooling.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 29, 2023

Maybe if addProtocol were able to return the actual dem values instead of image bytes it would give people the flexibility to choose their own encoding based on those tradeoffs?

@HarelM
Copy link
Member

HarelM commented Sep 29, 2023

We can define another format/hook maybe to provide elevation at a given location, this will allow bypassing this issue ATM and maybe allow for other plugins/protocols to be added later for more flexibility.
This is a rough initial idea, I'm not sure about the exact API that is needed here.
addProtocol only allows "replacing the network" and feeding the existing formats the data they expect.
I need to see if there's a way to add a custom source (the API exists, I haven't seen anyone use it successfully though) and maybe solve this from this direction.
IDK...
In general, I think all options aren't great but allowing developers to choose which one they prefer might be the right direction, given the above limitations...

@msbarry
Copy link
Contributor Author

msbarry commented Sep 30, 2023

It looks like the custom source API sort of makes it possible to do this kind of thing:

class CustomRasterDEMTileSource extends maplibregl.RasterDEMTileSource {
  constructor(...args) {
    super(...args);
  }

  loadTile(tile, callback) {
    const url = tile.tileID.canonical.url(this.tiles, this.map.getPixelRatio(), this.scheme);
    const request = this.map._requestManager.transformRequest(url, 'Tile');
    // do work with request, and eventually set this.dem to new DEMData and callback(null)
  }
}
map.addSourceType('custom-dem', CustomRasterDEMTileSource, err => err && console.error(err));
map.addSource('dem2', {
  type: 'custom-dem',
  encoding: 'terrarium',
  tiles: ['https://elevation-tiles-prod.s3.amazonaws.com/terrarium/{z}/{x}/{y}.png']
  maxzoom: 13,
  tileSize: 256,
});
map.addLayer({
  id: 'hills2',
  type: 'hillshade',
  source: 'dem2',
  paint: {
    'hillshade-exaggeration': 0.9,
  },
});

The problems I can see are:

  • The implementation is tightly coupled to RasterDEMTileSource that you are subclassing
  • need to depend on the private map._requestManager API to transform the request using user-provided callback
  • in order to pass the request through any registered protocols (ie. pmtiles) you need to pass the request through ImageRequest.getImage (or at least the registered protocols) which are not exposed.
  • the result needs to end up in an instance of DEMData which is also not exposed

At the end of the day, DEMData still requires the data to be encoded in RGB pixels so it can be passed into the GPU to render terrain and hillshading, so if you use a different encoding, you'll still eventually need to pack it into rgb.

I'm not sure if it would be cleaner to address those issues to improve the custom source API, or to provide a different abstraction to users who want to intercept requests at a higher layer than just the network? I could see also wanting to support 258x258 or 260x260 DEMs that include neighbor data so you don't need to download neighboring tiles which is a bit more complex than just intercepting the request.

@HarelM
Copy link
Member

HarelM commented Sep 30, 2023

I don't know either honestly.
I think it'll make sense to expose DEMdata regardless of this issue, I've seen several cases where it makes sense.
You can probably compose instead of inherit, but there will still be high coupling.
Since this API already exists, it can solve this specific issue ATM and allow exploring different approaches I believe...

@msbarry
Copy link
Contributor Author

msbarry commented Sep 30, 2023

I also noticed that addSourceType lets you load code to run in maplibre's workers, which is exactly what I was looking for with the maplibre-contour plugin so this does look promising:

this.dispatcher.broadcast('loadWorkerSource', {
name,
url: SourceType.workerSourceURL
}, callback);

The weird thing is that you run map.addSourceType(type, class, callback) but it registers the source type globally so if you call on a different map it will fail because it's already registered. Seems like maybe it should an attribute of maplibre like addProtocol is today.

Also looking at how addProtocol gets invoked today, I wonder if we could just add an additional return type like RGBAImage or DEMData where if the result matches that, maplibre treats it as parsed rgb image data and skips the canvas decoding here:

const transfer = isImageBitmap(img) && offscreenCanvasSupported();
const rawImageData = transfer ? img : browser.getImageData(img, 1);

@HarelM
Copy link
Member

HarelM commented Sep 30, 2023

I think addProtocol can probably be improved like it was fine in the following PR:
#2126
This isn't exactly at addProtocol, but more of imageRequest class that supports more types of images, including HTMLImageElement.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 30, 2023

Actually while it looks like safari doesn't implement ImageDecoder they do implement VideoFrame which lets you do the same thing from an image element or fetch response:

img=new Image()
img.crossOrigin=true
img.src='https://elevation-tiles-prod.s3.amazonaws.com/terrarium/0/0/0.png'
vf=new VideoFrame(img, {timestamp:0})
// or vf = new VideoFrame(await createImageBitmap(await fetch(url).blob()), {timestamp: 0})
arr=new Uint8Array(vf.allocationSize())
vf.copyTo(arr)
// arr is the same in chrome/safari/ios 17 private safari
// they just have different pixel color ordering, see: https://w3c.github.io/webcodecs/#pixel-format

doesn't look like it works with firefox yet - from this page though it looks like firefox does plan to implement webcodec eventually.

@msbarry
Copy link
Contributor Author

msbarry commented Sep 30, 2023

Here's a very rough version that does seem to fix the issue on iOS 17 private safari: https://github.com/maplibre/maplibre-gl-js/compare/main...msbarry:maplibre-gl-js:ios-17-fix-maybe?expand=1

@msbarry
Copy link
Contributor Author

msbarry commented Oct 6, 2023

Here's the fix I'm putting in for maplibre-contour plugin: https://github.com/onthegomap/maplibre-contour/pull/85/files - are we OK applying a similar fix to maplibre?

@HarelM
Copy link
Member

HarelM commented Oct 6, 2023

I think so.
I also think the ability to read an image from arraybuffer and maybe use the DEMData class can be helpful to other libraries, I know I was copying the code to create some elevation service from RGBTerrain tiles in my website, and I needed the ability to both convert an image to pixels and convert these pixels to elevation data.
So I tend to think some stuff here can be exported for reusability in other applications/plugins...

@msbarry
Copy link
Contributor Author

msbarry commented Oct 7, 2023

OK, I put out a proposed fix in #3185, I was thinking get that in, then a separate review for exposing those other utilities?

@HarelM
Copy link
Member

HarelM commented Oct 7, 2023

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants