Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core][android] Provide API to control eviction of cached images #14610

Merged
merged 6 commits into from
May 21, 2019

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented May 8, 2019

This PR introduces following changes:

  • Defines constant for ImageManager's cache size DEFAULT_ON_DEMAND_IMAGES_CACHE_SIZE that is equal to ~100 8kB sprites.
  • Adds bool onCanRemoveUnusedStyleImage(const std::string&) method to MapObserver interface that is invoked in two cases: OOM and when cumulative size of unused images in the cache goes over DEFAULT_ON_DEMAND_IMAGES_CACHE_SIZE limit.
  • Android implementation and unit tests

By default, map will remove *unused images if application returns true from onCanRemoveUnusedStyleImage method.

*Unused images: are images whose requesting tile is destroyed. Map will notify about unused images only if image was provided through onStyleImageMissing(id) => addImage(id).

// Android example
mapView.addOnCanRemoveUnusedStyleImageListener(id -> {
  return id != "keep_icon";
});

Fixes: #14547


TODO:

  • Darwin platform bindings

@alexshalamov
Copy link
Contributor Author

/cc @julianrex Would someone from your team have time to implement darwin platform bindings?

src/mbgl/renderer/image_manager.hpp Outdated Show resolved Hide resolved
src/mbgl/renderer/image_manager.hpp Outdated Show resolved Hide resolved
src/mbgl/renderer/image_manager.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/image_manager.cpp Show resolved Hide resolved
auto it = requestedImages.find(dependency.first);
if (it != requestedImages.end()) {
it->second.emplace(&requestor);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the above code not needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it moved to the checkMissingAndNotify, could you give some details on this?

src/mbgl/renderer/image_manager.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/image_manager.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/renderer_impl.cpp Outdated Show resolved Hide resolved
platform/android/src/native_map_view.cpp Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_removeUnusedStyleImages branch 2 times, most recently from 2e5c2f8 to d9b7936 Compare May 8, 2019 12:42
@julianrex
Copy link
Contributor

@alexshalamov yes, we can look at the Darwin side :)

@julianrex
Copy link
Contributor

@alexshalamov @tobrun a couple of questions:

  1. What's the policy for evicting images? Just all those that haven't been used recently? Is it an LRU policy?

  2. I wonder if this should ask platform code if it should evict images before doing so? An application may have good reason to keep an image around - is there a way to do this already?

@alexshalamov alexshalamov force-pushed the alexshalamov_removeUnusedStyleImages branch from d9b7936 to 40058ca Compare May 9, 2019 14:06
@alexshalamov
Copy link
Contributor Author

@julianrex

  1. What's the policy for evicting images? Just all those that haven't been used recently?

Whenever cumulative size of unused images that were added via onStyleImageMissing API goes over the limit, platform will notify MapObserver to remove unused images. By default, core will remove images from the style.

  1. I wonder if this should ask platform code if it should evict images before doing so? An application may have good reason to keep an image around - is there a way to do this already?

When core notifies SDK / application code and there is developer provided handler for the onRemoveUnusedStyleImages notification, core will not remove anything and delegate it completely to the application.

@julianrex
Copy link
Contributor

@alexshalamov can this land without the iOS parts? Or would you like a PR to this branch?

@alexshalamov
Copy link
Contributor Author

@julianrex not necessary to this branch, could be separate PR, I just want to make sure that the new interface is implementable for darwin platform.

src/mbgl/renderer/image_manager.cpp Outdated Show resolved Hide resolved
src/mbgl/map/map_impl.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Darwin side we have all what we need to build the bindings. I just have a question regarding the implementation.

@alexshalamov @julianrex we could pr the iOS bindings here or in a new pr, your call.

src/mbgl/map/map_impl.cpp Outdated Show resolved Hide resolved
@chloekraw
Copy link
Contributor

@alexshalamov quick point of clarification - when you say

Whenever cumulative size of unused images that were added via onStyleImageMissing API goes over the limit, platform will notify MapObserver to remove unused images.

that means all unused images are removed? so this has no direct relationship to least recently used, correct?

@alexshalamov alexshalamov force-pushed the alexshalamov_removeUnusedStyleImages branch 2 times, most recently from b80ae7c to 1e7f2b7 Compare May 10, 2019 11:04
@alexshalamov
Copy link
Contributor Author

@chloekraw

that means all unused images are removed? so this has no direct relationship to least recently used, correct?

All unused images that were added as a result of onStyleImageMissing notification. As for recently used, I don't think we have such concept at the moment.

@alexshalamov alexshalamov force-pushed the alexshalamov_removeUnusedStyleImages branch 2 times, most recently from 5d3c26f to 506c8d6 Compare May 16, 2019 02:07
@alexshalamov
Copy link
Contributor Author

@tobrun @fabian-guerra are you fine with the new observer method? I would like to merge PR this week.

@alexshalamov alexshalamov force-pushed the alexshalamov_removeUnusedStyleImages branch from 506c8d6 to cf96963 Compare May 21, 2019 13:11
@alexshalamov alexshalamov merged commit 00790dd into master May 21, 2019
@alexshalamov alexshalamov deleted the alexshalamov_removeUnusedStyleImages branch May 21, 2019 13:49
@chloekraw chloekraw added Core The cross-platform C++ core, aka mbgl Android Mapbox Maps SDK for Android labels Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Provide API to control eviction of cached images
7 participants