Share and release marker bitmaps in mapsforge layer (rel. to cgeo#18404)

Every marker created a private ARGB_8888 copy of its source bitmap via
AndroidGraphicFactory.convertToBitmap(), although all caches of one type
share the same source bitmap from MapMarkerUtils' marker cache. Those
copies were never released either: MapsforgeV6ZLevelGroupLayer dropped
removed layers without calling Layer.onDestroy(), so Marker.onDestroy()
never decremented the bitmap reference count and Mapsforge's reusable
bitmap pool was never refilled.

Share one Mapsforge bitmap per source bitmap and hand out reference
counted references, and destroy layers when they are removed from the
group layer or when the group layer itself is destroyed.

Author: moving-bits

main/src/main/java/cgeo/geocaching/unifiedmap/geoitemlayer/MapsforgeV6GeoItemLayer.java

@@ -18,7 +18,9 @@
 import androidx.annotation.NonNull;
 
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.mapsforge.core.graphics.Paint;
 import org.mapsforge.core.graphics.Style;
@@ -41,6 +43,43 @@ public class MapsforgeV6GeoItemLayer implements IProviderGeoItemLayer<int[]> {
 
     private MapsforgeV6ZLevelGroupLayer groupLayer;
 
+    private final MarkerBitmapCache markerBitmaps = new MarkerBitmapCache();
+
+    /**
+     * Mapsforge needs its own copy of every marker bitmap, but many markers share the same source bitmap
+     * (all caches of one type look alike). Creating one copy per marker wastes a full ARGB_8888 bitmap each
+     * time, so copies are shared and released via Mapsforge's reference counting instead.
+     * <br>
+     * Reference counting contract: a freshly created bitmap has a count of 0, meaning "one owner" - which is
+     * this cache. Every marker handed the bitmap increments, {@link Marker#onDestroy()} decrements again, and
+     * the bitmap is only destroyed once the count drops below 0.
+     */
+    private static final class MarkerBitmapCache extends LinkedHashMap<Bitmap, org.mapsforge.core.graphics.Bitmap> {
+
+        private static final long serialVersionUID = 1L;
+        private static final int MAX_ENTRIES = 256;
+
+        MarkerBitmapCache() {
+            super(16, 0.75f, true);
+        }
+
+        @Override
+        protected boolean removeEldestEntry(final Map.Entry<Bitmap, org.mapsforge.core.graphics.Bitmap> eldest) {
+            if (size() <= MAX_ENTRIES) {
+                return false;
+            }
+            eldest.getValue().decrementRefCount(); // give up this cache's own reference
+            return true;
+        }
+
+        void release() {
+            for (org.mapsforge.core.graphics.Bitmap bitmap : values()) {
+                bitmap.decrementRefCount();
+            }
+            clear();
+        }
+    }
+
     public MapsforgeV6GeoItemLayer(final MapView mapView) {
         this.mapView = mapView;
         this.layerManager = mapView.getLayerManager();
@@ -76,6 +115,9 @@ public void destroy(final Collection<Pair<GeoPrimitive, int[]>> values) {
             }
             groupLayer.requestRedraw();
         }
+        // the markers created from these bitmaps have been destroyed above, so releasing our own
+        // reference now actually frees them
+        markerBitmaps.release();
         this.mapView = null;
     }
 
@@ -144,7 +186,7 @@ public String onMapChangeBatchEnd(final long processedCount) {
         return null;
     }
 
-    private static Marker createMarker(final Geopoint point, final GeoIcon icon) {
+    private Marker createMarker(final Geopoint point, final GeoIcon icon) {
         if (point == null || icon == null || icon.getBitmap() == null) {
             return null;
         }
@@ -155,13 +197,29 @@ private static Marker createMarker(final Geopoint point, final GeoIcon icon) {
 
         final Marker newMarker = new Marker(
                 latLong(point),
-                AndroidGraphicFactory.convertToBitmap(new BitmapDrawable(CgeoApplication.getInstance().getResources(), bitmap)),
+                getMarkerBitmap(bitmap),
                 (int) ((-icon.getXAnchor() + 0.5f) * bitmap.getWidth()),
                 (int) ((-icon.getYAnchor() + 0.5f) * bitmap.getHeight()));
         newMarker.setBillboard(!icon.isFlat());
         return newMarker;
     }
 
+    /**
+     * Gets the Mapsforge representation of the given bitmap, creating it on first use.
+     * The returned bitmap carries one additional reference for the marker it is about to be handed to.
+     */
+    private org.mapsforge.core.graphics.Bitmap getMarkerBitmap(final Bitmap bitmap) {
+        org.mapsforge.core.graphics.Bitmap markerBitmap = markerBitmaps.get(bitmap);
+        if (markerBitmap == null) {
+            // do NOT wrap the source bitmap directly: it is owned by MapMarkerUtils' marker cache and would
+            // be recycled by Mapsforge once the last marker using it is destroyed
+            markerBitmap = AndroidGraphicFactory.convertToBitmap(new BitmapDrawable(CgeoApplication.getInstance().getResources(), bitmap));
+            markerBitmaps.put(bitmap, markerBitmap);
+        }
+        markerBitmap.incrementRefCount();
+        return markerBitmap;
+    }
+
     private static Paint createPaint(@ColorInt final int color) {
         final Paint p = AndroidGraphicFactory.INSTANCE.createPaint();
         p.setColor(color);

main/src/main/java/cgeo/geocaching/unifiedmap/geoitemlayer/MapsforgeV6ZLevelGroupLayer.java

@@ -2,7 +2,9 @@
 
 import androidx.annotation.Nullable;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
@@ -62,6 +64,7 @@ public void remove(final boolean redraw, @Nullable final int ... context) {
         if (context == null || context.length < 2) {
             return;
         }
+        final List<Layer> removedLayers = new ArrayList<>(context.length);
         layerLock.lock();
         try {
             final int zLevel = context[0];
@@ -73,19 +76,52 @@ public void remove(final boolean redraw, @Nullable final int ... context) {
                 if (context[i] == 0) {
                     continue;
                 }
-                zLevelMap.remove(context[i]);
+                final Layer removed = zLevelMap.remove(context[i]);
+                if (removed != null) {
+                    removedLayers.add(removed);
+                }
             }
             if (zLevelMap.isEmpty()) {
                 this.layerItemMap.remove(zLevel);
             }
         } finally {
             layerLock.unlock();
         }
+        // free resources held by the removed layers (esp. bitmaps of markers). Do this outside of our
+        // own lock so we never call foreign code while holding it
+        destroyLayers(removedLayers);
         if (redraw) {
             requestRedraw();
         }
     }
 
+    /** removes all layers from this group and frees the resources held by them */
+    @Override
+    public void onDestroy() {
+        final List<Layer> removedLayers = new ArrayList<>();
+        layerLock.lock();
+        try {
+            for (Map<Integer, Layer> zLevelMap : this.layerItemMap.values()) {
+                removedLayers.addAll(zLevelMap.values());
+            }
+            this.layerItemMap.clear();
+        } finally {
+            layerLock.unlock();
+        }
+        destroyLayers(removedLayers);
+        super.onDestroy();
+    }
+
+    /**
+     * Mapsforge layers hold resources which are only released on onDestroy(); for a Marker this is the
+     * reference to its bitmap. Removing a layer without destroying it therefore leaks that bitmap.
+     */
+    private static void destroyLayers(final Iterable<Layer> layers) {
+        for (Layer layer : layers) {
+            layer.onDestroy();
+        }
+    }
+
     @Override
     public void draw(final BoundingBox boundingBox, final byte zoomLevel, final Canvas canvas, final Point topLeftPoint, final Rotation rotation) {
         layerLock.lock();