HARP-12700: Disposal of POI resources by reference counting. #1946

atomicsulfate · 2020-11-03T17:38:28Z

Share material and texture across pois with same image even if they have different render order.
Do reference counting on resources to dispose of them when they're not used any longer.

- Share material and texture across pois with same image even if they have different render order. - Do reference counting on resources to dispose of them when they're not used any longer.

atomicsulfate · 2020-11-04T14:13:18Z

@here/harp-mapview/lib/Tile.ts

+        this.textElementGroups.forEach((element: TextElement) => {
+            element.dispose();
+        });


Call to TextElement.dispose() on Tile disposal to ensure that it releases it's poi buffer (if any).

atomicsulfate · 2020-11-04T14:14:41Z

@here/harp-mapview/lib/text/TextElement.ts

+    dispose() {
+        const poiBuffer = this.poiInfo?.buffer;
+        if (poiBuffer) {
+            poiBuffer.decreaseRefCount();
+        }
+    }


Decrease ref counting on TextElement disposal to ensure the poi buffer and associated resources are released when ref count reaches 0.

atomicsulfate · 2020-11-04T14:16:21Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

        const imageWidth = imageItem.imageData.width;
        const imageHeight = imageItem.imageData.height;
        const paddedSize = MipMapGenerator.getPaddedSize(imageWidth, imageHeight);
-        const trilinearFiltering = PoiRenderBufferBatch.trilinear && imageItem.mipMaps;
+        const trilinearFiltering = PoiBatch.trilinear && imageItem.mipMaps;
        const paddedImageWidth = trilinearFiltering ? paddedSize.width : imageWidth;
        const paddedImageHeight = trilinearFiltering ? paddedSize.height : imageHeight;

-        const iconWidth = imageTexture.width !== undefined ? imageTexture.width : imageWidth;
-        const iconHeight = imageTexture.height !== undefined ? imageTexture.height : imageHeight;
+        const iconWidth = imageTexture?.width !== undefined ? imageTexture.width : imageWidth;
+        const iconHeight = imageTexture?.height !== undefined ? imageTexture.height : imageHeight;

-        let minS = 0;
-        let maxS = 1;
-        let minT = 0;
-        let maxT = 1;
+        const width = imageTexture?.width !== undefined ? imageTexture.width : imageWidth;
+        const height = imageTexture?.height !== undefined ? imageTexture.height : imageHeight;
+        const xOffset = imageTexture?.xOffset !== undefined ? imageTexture.xOffset : 0;
+        const yOffset = imageTexture?.yOffset !== undefined ? imageTexture.yOffset : 0;
+
+        const minS = xOffset / paddedImageWidth;
+        const maxS = (xOffset + width) / paddedImageWidth;
+        const minT = yOffset / paddedImageHeight;
+        const maxT = (yOffset + height) / paddedImageHeight;


no actual change, just removing unused code and simplifying.

atomicsulfate · 2020-11-04T14:17:39Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

+        if (imageItem.loadingPromise) {
+            // already being loaded, will be rendered once available
+            return;
+        }
+
+        const result = imageCache.loadImage(imageItem);
+        assert(result instanceof Promise);
+        const loadPromise = result as Promise<ImageItem | undefined>;
+        loadPromise
+            .then(loadedImageItem => {
+                if (loadedImageItem === undefined) {
+                    logger.error(`preparePoi: Failed to load imageItem: '${imageItem.url}`);
+                    return;
+                }
+                this.setupPoiInfo(poiInfo, loadedImageItem, env, imageTexture);
+            })
+            .catch(error => {
+                logger.error(`preparePoi: Failed to load imageItem: '${imageItem.url}`, error);
+                poiInfo.isValid = false;
+            });


Just rearranging the code to reduce nesting level.

atomicsulfate · 2020-11-04T14:23:39Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

        } else {
+            // No image texture found. Either this is a user image or it's missing.
+            const image = this.mapView.userImageCache.findImageByName(imageTextureName);
+
+            if (!image) {
+                // Warn about a missing texture, but only once.
+                if (PoiRenderer.m_missingTextureName.get(imageTextureName) === undefined) {
+                    PoiRenderer.m_missingTextureName.set(imageTextureName, true);
+                    logger.error(
+                        `preparePoi: No imageTexture with name '${imageTextureName}' found`
+                    );
+                }
+                poiInfo.isValid = false;
+                return;


Now the user images don't need to be registered both in MapView's userImageCache and PoiManager. This only makes sense internally for theme atlases.

atomicsulfate · 2020-11-04T14:24:44Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

-    boxBuffer: BoxBuffer | undefined;
-
-    private m_material?: THREE.Material | THREE.Material[];
+export class PoiBuffer {


This is the class where we do reference counting and propagate disposal of any resources when ref count reaches 0.

atomicsulfate · 2020-11-04T14:29:10Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

+ * There is a `PoiBatch` for every icon in a texture atlas, since the size of the icon in the atlas
+ * as well as the texture coordinates are specified in the `PoiBatch`.
+ */
+class PoiBatch {


This is the old PoiRenderBufferBatch, but I've moved in all PoiBuffers with same image but different render order, which now share same material and texture (before we didn't share them for different render orders of the same image).

atomicsulfate · 2020-11-04T14:31:45Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

+    pickBoxes(
+        screenPosition: THREE.Vector2,
+        pickCallback: (pickData: any | undefined) => void,
+        imageData?: ImageBitmap | ImageData
+    ) {
+        for (const poiBuffer of this.m_poiBuffers.values()) {
+            poiBuffer.buffer.pickBoxes(screenPosition, pickCallback, imageData);
+        }
+    }
+
+    /**
+     * Update the info with the memory footprint caused by objects owned by the `PoiBatch`.
+     *
+     * @param info - The info object to increment with the values from this `PoiBatch`.
+     */
+    updateMemoryUsage(info: MemoryUsage) {
+        if (this.imageItem.imageData !== undefined) {
+            const imageBytes = this.imageItem.imageData.width * this.imageItem.imageData.height * 4;
+            info.heapSize += imageBytes;
+            info.gpuSize += imageBytes;
+        }
+        for (const poiBuffer of this.m_poiBuffers.values()) {
+            poiBuffer.buffer.updateMemoryUsage(info);
+        }
+    }


Nothing new here, just moved.

atomicsulfate · 2020-11-04T14:32:34Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

+        this.m_poiBuffers.clear();
+        this.m_material.map.dispose();
+        this.m_material.dispose();
+        this.m_onDispose();


Here we dispose of material and texture, which we never diposed of before.

atomicsulfate · 2020-11-04T14:33:26Z

@here/harp-mapview/lib/poi/PoiRenderer.ts

+
+        if (batch === undefined) {
+            batch = new PoiBatch(this.mapView, this.textCanvas, imageItem, () => {
+                this.deleteBatch(batchKey);


This callback deletes a batch when it's empty.

FraukeF · 2020-11-04T14:00:39Z

@here/harp-datasource-protocol/lib/TechniqueParams.ts

@@ -707,11 +707,13 @@ export interface MarkerTechniqueParams extends BaseTechniqueParams {
     */
    poiNameField?: string;
    /**
-     * Name of [[ImageTexture]] definition to use.
+     * Name of [[ImageTexture]] definition or image in {@link @here/harp-mapview#userImageCache}


I think the imageTextures from the theme go to some internal cache in MapViewThemeManager, not to MapView.userImageCache

I meant, it's the name of either a theme's image or a user image in userImageCache. I'll rewrite the documentation to make it clearer.

@here/harp-mapview/lib/poi/PoiRenderer.ts

codecov · 2020-11-05T09:11:51Z

Codecov Report

Merging #1946 into master will increase coverage by 0.52%.
The diff coverage is 46.61%.

@@            Coverage Diff             @@
##           master    #1946      +/-   ##
==========================================
+ Coverage   65.25%   65.78%   +0.52%     
==========================================
  Files         293      293              
  Lines       26329    26327       -2     
  Branches     5951     5936      -15     
==========================================
+ Hits        17181    17319     +138     
+ Misses       9148     9008     -140

Impacted Files	Coverage Δ
...re/harp-datasource-protocol/lib/TechniqueParams.ts	`72.13% <ø> (ø)`
@here/harp-mapview/lib/MapView.ts	`73.48% <ø> (ø)`
@here/harp-mapview/lib/poi/BoxBuffer.ts	`29.21% <41.05%> (+23.04%)`	⬆️
@here/harp-mapview/lib/poi/PoiRenderer.ts	`44.14% <48.32%> (+26.06%)`	⬆️
@here/harp-mapview/lib/text/TextElement.ts	`86.41% <80.00%> (-0.60%)`	⬇️
@here/harp-mapview/lib/Tile.ts	`83.23% <100.00%> (+0.09%)`	⬆️
@here/harp-mapview/lib/text/FontCatalogLoader.ts	`85.71% <0.00%> (-7.15%)`	⬇️
@here/harp-mapview/lib/MapViewFog.ts	`63.49% <0.00%> (-2.69%)`	⬇️
@here/harp-mapview/lib/MapViewEnvironment.ts	`35.55% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d230e1e...84ef311. Read the comment docs.

atomicsulfate force-pushed the HARP-12700_DynamicMarkers branch 2 times, most recently from bf51876 to 73c71e3 Compare November 4, 2020 12:12

atomicsulfate changed the title ~~HARP-12700: Refactor PoiRenderBuffer~~ HARP-12700: Disposal of POI resources by reference counting. Nov 4, 2020

HARP-12700: Disposal of POI resources by reference counting.

591b8df

- Share material and texture across pois with same image even if they have different render order. - Do reference counting on resources to dispose of them when they're not used any longer.

atomicsulfate force-pushed the HARP-12700_DynamicMarkers branch from 73c71e3 to 591b8df Compare November 4, 2020 12:18

atomicsulfate commented Nov 4, 2020

View reviewed changes

FraukeF reviewed Nov 4, 2020

View reviewed changes

HARP-12700: Address review comment and fix unit tests.

84ef311

atomicsulfate requested a review from FraukeF November 5, 2020 08:59

FraukeF approved these changes Nov 6, 2020

View reviewed changes

atomicsulfate merged commit e9e44a6 into master Nov 6, 2020

atomicsulfate deleted the HARP-12700_DynamicMarkers branch November 6, 2020 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HARP-12700: Disposal of POI resources by reference counting. #1946

HARP-12700: Disposal of POI resources by reference counting. #1946

atomicsulfate commented Nov 3, 2020 •

edited

Loading

atomicsulfate Nov 4, 2020 •

edited

Loading

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

atomicsulfate Nov 4, 2020

FraukeF Nov 4, 2020

atomicsulfate Nov 5, 2020

codecov bot commented Nov 5, 2020 •

edited

Loading

HARP-12700: Disposal of POI resources by reference counting. #1946

HARP-12700: Disposal of POI resources by reference counting. #1946

Conversation

atomicsulfate commented Nov 3, 2020 • edited Loading

atomicsulfate Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2020 • edited Loading

Codecov Report

atomicsulfate commented Nov 3, 2020 •

edited

Loading

atomicsulfate Nov 4, 2020 •

edited

Loading

codecov bot commented Nov 5, 2020 •

edited

Loading