Pixel perfect static images

[julioromano]

1. On devices less than xhdpi request a 1x image from MapTiler (such devices are generally old, slower and with little memory so avoiding to get the 2x image only to have to shrink it later could help).
2. Coerce too big width/height combos within the API limits keeping the aspect ratio (this will allow requests on big horizontal displays to succeed).
3. Don't crash when given weird width/height combos (i.e. zero or negative).
4. Introduce interfaces to hide this whole logic and make it easier for forks to implement their own.

Related to:

• [Story] View expanded map element-meta#1678

[github-actions]

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link:

[codecov]

Codecov Report

Patch coverage: 94.64% and project coverage change: +0.06 🎉

Comparison is base (88c66dd) 56.61% compared to head (c3b82f5) 56.67%.

❗ Current head c3b82f5 differs from pull request most recent head 18b14c9. Consider uploading reports for the commit 18b14c9 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #933      +/-   ##
===========================================
+ Coverage    56.61%   56.67%   +0.06%     
===========================================
  Files          969      973       +4     
  Lines        24667    24704      +37     
  Branches      5009     5017       +8     
===========================================
+ Hits         13966    14002      +36     
- Misses        8483     8484       +1     
  Partials      2218     2218              

Impacted Files	Coverage Δ
...id/features/location/impl/show/ShowLocationView.kt	54.21% <ø> (ø)
...location/api/internal/TileServerStyleUriBuilder.kt	66.66% <66.66%> (ø)
...cation/api/internal/MapTilerStaticMapUrlBuilder.kt	96.55% <96.55%> (ø)
...ent/android/features/location/api/StaticMapView.kt	50.00% <100.00%> (+1.02%)	⬆️
...d/features/location/api/internal/MapTilerConfig.kt	100.00% <100.00%> (ø)
.../api/internal/MapTilerTileServerStyleUriBuilder.kt	100.00% <100.00%> (ø)
...tures/location/api/internal/StaticMapUrlBuilder.kt	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[csmith]

I don't think we want to do this.

Changing the size of the map we request affects the information density returned in the image. On an xxxhdpi display that would mean maptiler gives us a map suitable for display on something the size of a small computer monitor, and we display it in the space of a few postage stamps. It would be crisp, but it achieves that by rendering more area around the location we care about, effectively zooming out.

Sizing it to the physical pixel dimensions and then requesting the @2x version also means we end up with an image twice as large as needed in both dimensions. e.g. if we're filling a 300px x 150px area we'd now get a 600px x 300px image and that would have to be scaled down by the platform to actually display.

iOS has the same behaviour as the old code: they pass the size in points to MapTiler, which will be 2x or 3x smaller than the number of pixels depending on the device. (Presumably, on 3x devices the map isn't "pixel perfect" either)

[julioromano]

Sizing it to the physical pixel dimensions and then requesting the @2x version also means we end up with an image twice as large as needed in both dimensions. e.g. if we're filling a 300px x 150px area on an xdpi device we'd now get a 600px x 300px image and that would have to be scaled down by the platform to actually display.

That's not what this code does: Requested dimension is halved for @2x images thereby yielding the same pixel size static image but with twice the resolution (i.e. zoomed in).

[csmith]

That's not what this code does: Requested dimension is halved for @2x images thereby yielding the same pixel size static image but with twice the resolution (i.e. zoomed in).

Ah, I missed that. As it happens, that's exactly what I did when I first implemented it (halving the pixel count and requesting @2x), and I had to switch to the current implementation to get maps that looked OK/consistent.

[julioromano]

To give you an idea here's how the 3 possible choices look: first is current, second is 2x halved (as proposed in this PR), third is 2x without halving (zooms out a lot as you said).

[csmith]

You can see the zoom out in the second image as well, and that zoom will change based on the density of the device, which we don't want. If you find a device with a density double your current one, it'll look like the bottom image, right?

[julioromano]

If you find a device with a density double your current one, it'll look like the bottom image, right?

Yes, and on the same device the current implementation will look twice as more blurred.
Anyways ~1000dpi devices are not a thing nowadays. They currently top out at ~560dpi. So it's not a thing we should worry about soon.

I don't think there's a right choice here: both choices have pros and cons.
Current) Pixel sharp. Text and map will shrink effectively zooming out as the device dpi increases further from @2x (i.e. ~320dpi).
Proposed) Blurred. Text and map will be of constant size regardless of the device. Blur will increase as the device dpi increases further from @2x (i.e. ~320dpi).

So either we keep the sharpness constant at the expense of a decreased content size.
OR
We keep the content size constant at the expense of a decreased sharpness (i.e. blur)

[csmith]

IMO if the lack of crispness is an issue we should look at alternatives to maptiler that can provide images in the right density rather than just 1x and 2x (or at least provide controls for the map area separate to the dimensions of the static map). [But obviously that's probably out of scope for this initial implementation 😓 ]

[bmarty]

Just putting again the screenshots from previous comment but in a table so that we can compare easily

current	2xHalved	2xNotHalved

[julioromano]

Thanks! The table helps to evaluate and compare the various content sizes, but to evaluate sharpness the images should be looked at 100% size (or even better by running the code on a screen of a real device).

[bmarty]

I have no particular remark on the code, but maybe what can be strange is that the event will be rendered differently between 2 devices. Also I think user can still open the map fullscreen by clicking on it and zoom as they want, so I think it's fine.

[bmarty]

FTR, attribution was managed manually on Element Android since the rendering was declared too small, and on the new screenshots of this PR, they are smaller again (and truncated due to the round corner). More info in element-hq/element-android#6247. I am not sure if it was in your radar, so now you know!

[csmith]

Just to illustrate the pathological case, this is what a Sony Xperia Z5 Premium would look like:

Current (using DPI and 2x)

New (using pixels/2 and 2x)

And this is what it looks like in eleweb:

[julioromano]

IMO if the lack of crispness is an issue we should look at alternatives to maptiler that can provide images in the right density rather than just 1x and 2x

@csmith You hit the nail on the head. To properly (i.e. with a correct content size) rasterize a vector image (such as a map) we need 2 things:

1. width/height dimensions in pixels.
2. screen density (in DPI or as a scaling factor).

We can easily get this information from the Android OS but MapTiler's static map API doesn't take it as an input. It effectively only allows 1 and 2 as scaling factors.

I've changed the PR to basically keep the existing behavior but with a few tweaks:

1. On devices less than xhdpi request a 1x image from MapTiler (such devices are generally old, slower and with little memory so avoiding to get the 2x image only to have to shrink it later could help).
2. Coerce too big width/height combos within the API limits keeping the aspect ratio (this will allow requests on big horizontal displays to succeed).
3. Don't crash when given weird width/height combos (i.e. zero or negative).
4. Introduce interfaces to hide this whole logic and make it easier for forks to implement their own.

[csmith]

LGTM, thanks!

I added a small doc in docs/maps.md about passing in properties and forking etc, I guess that probably needs a slight update now

[julioromano]

Done!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication