perf: Improve KML bitmap caching and fix icon scaling

[jeffdgr8]

Improve KML bitmap caching by caching BitmapDescriptors instead of Bitmaps. Every call to BitmapDescriptorFactory.fromBitmap() copies the Bitmap passed to it. So the current code ends up duplicating every Bitmap that is reused for multiple Markers or GroundOverlays for every usage.

Tested with this KML with 25 different marker icons and 25,000 random map points:
huge_kml.kml (remove .txt extension). Credit

On a Pixel 4 XL, the current code uses 0.9 GB RAM.

With these changes, only 294 MB RAM is used.

Note the caches use HashMaps and not LruCache, as was used previously. This is because only the BitmapDescriptors are cached indefinitely and the cache is only holding a reference to the same BitmapDescriptors used by the GoogleMap in its own view. So discarding any of these references wouldn't regain memory, as long as the BitmapDescriptors are still being used by the map view.

The original Bitmaps are only retained until all the images have been loaded, scaled, and cached as BitmapDescriptors.

Also fixes scaling for point marker icon images, taking into account screen density and scaling to a normalized minimum width of 32.

[arriolac]

Since this is now a breaking change to other clients, can you describe the steps to migrate to this new API? At first glance, this seems like an internal API that should've been scoped as protected but it would be good to have in case developers are using this function.

[jeffdgr8]

Yes, I don't believe anyone would actually be using the Renderer API directly. Many of these methods should be protected, as they're used only by the Renderer subclasses, KmlRenderer and GeoJsonRenderer. Many of the public methods can't be package-private or protected only because they're used both in Layer (same package) and one of the subclasses, which are in different packages.

Code that utilizes the KML and GeoJSON functionality of the library will mostly be interacting with the Layer subclasses, KmlLayer and GeoJsonLayer, which don't even expose their Renderer objects directly in order to access those public methods. Renderer could almost be an abstract class if the code was refactored some. There's quite a bit of code that's specific to KML and GeoJSON that could also be moved to one or the other subclasses.

I went ahead and reduced the access scope on all the Renderer methods that could be, to either package-private or protected. 084c0ca In this particular method's case, a Renderer subclass, which I don't expect to exist outside of KmlRenderer and GeoJsonRenderer, would follow the pattern in these subclasses and call getCachedMarkerImage() or getCachedGroundOverlayImage() instead of accessing a cache directly as was previously done through this getImagesCache() method.

[arriolac]

Agreed that scoping should be reduced in that class. Thanks for addressing that!

[arriolac]

Suggestion: thoughts on wrapping mMarkerImagesCache, mGroundOverlayImagesCache and mBitmapCache into a class? I think this would help with readability and simplify the API a bit.

[jeffdgr8]

Good call. Done: e9d8f24

[arriolac]

Noting that this is another breaking change. I do think this is a break API though.

[jeffdgr8]

As before, I don't believe anyone would actually be using the Renderer API directly. I've made this particular method protected now under this assumption. The previous ArrayList was always only adding unique icon URLs, but using a set makes this much more efficient, avoiding full list scanning for contains() each time one is added.

[arriolac]

The probability of missing the cache can be pretty high having Double as a key for Map. I suggest storing this as String instead and have up to a certain precision level (e.g. 4 decimal places?)

[jeffdgr8]

I had thought about this briefly and concluded that while a Double may not be able to perfectly represent all parsed decimal strings, 0.1 for example, that at least the parsed string double value will be equal for the scale value used to cache the image and scale value used to access the cached scaled image. In other words, a double representation of 0.1 will equal another double representation of 0.1, both obtained from the same parse routine. In fact, the same double mScale variable from a KmlStyle is often being used to both cache and retrieve the cached images.

However, while I made this conclusion based on the current implementation, I do see how a bug could possibly be introduced in the future, if style scales were introduced from another source somehow, possibly with a lower float precision.

I made this change to use the string formatted scale value: f9bd8ba

[arriolac]

I'd expect that getting a cached image would be pretty fast just from this method name. However, since this method will also perform image scaling, which is time consuming, it would be good to add a note that this is the case.

[jeffdgr8]

I added more details to the doc comment: f99a787