Implement removeSprite

[ljbade]

No description provided.

[ljbade]

@tobrun I think to implement this, we will need reference counting in Sprite class to know when last marker with this icon is removed from the map

[ljbade]

I have implemented this my using a SparseIntArray to hold the ref count for each sprite ID (which is now stored as an int in Sprite).

Remaining is to test then make a PR.

[ljbade]

@tobrun We are going to need to add a stress test similar to the bulk marker test for adding and removing a large number of sprites to ensure we correctly delete the sprites when they reach a zero ref count.

So basically a lot of markers each with a large number of different sprites with varying amounts of duplicate icons.

[ljbade]

Hmm Core GL's removeAll method seems to crash.

[ljbade]

Stack trace:

********** Crash dump: **********
Build fingerprint: 'samsung/zerofltedv/zeroflte:5.1.1/LMY47X/G920IDVU3DOJ6:user/release-keys'
pid: 29915, tid: 31161, name: Map Thread  >>> com.mapbox.mapboxgl.testapp <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0xc
Stack frame #00 pc 002337ec  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (mbgl::SpriteAtlas::copy(mbgl::SpriteAtlas::Holder const&, bool)+48): Routine std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__is_long() const at /home/leith/mb/mapbox-gl-native/mason_packages/linux-x86_64/android-ndk/arm-9-r10e/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../include/c++/4.9/string:1712
Stack frame #01 pc 0023436c  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (mbgl::SpriteAtlas::updateDirty()+472): Routine mbgl::SpriteAtlas::updateDirty() at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/geometry/sprite_atlas.cpp:185
Stack frame #02 pc 001e0728  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (mbgl::MapContext::setSprite(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::shared_ptr<mbgl::SpriteImage const>)+124): Routine mbgl::MapContext::setSprite(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::shared_ptr<mbgl::SpriteImage const>) at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/map/map_context.cpp:312
Stack frame #03 pc 001e7e04  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (_ZN4mbgl4util7RunLoop7InvokerIZNS0_6ThreadINS_10MapContextEE4bindIMS4_FvRKNSt3__112basic_stringIcNS7_11char_traitsIcEENS7_9allocatorIcEEEENS7_10shared_ptrIKNS_11SpriteImageEEEEEEDaT_EUlDpOT_E_NS7_5tupleIJSD_SJ_EEEE6invokeIJLj0ELj1EEEEvNS7_16integer_sequenceIjJXspT_EEEE+104): Routine operator()<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::shared_ptr<const mbgl::SpriteImage> > at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/util/thread.hpp:72
Stack frame #04 pc 001e7d4c  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (_ZN4mbgl4util7RunLoop7InvokerIZNS0_6ThreadINS_10MapContextEE4bindIMS4_FvRKNSt3__112basic_stringIcNS7_11char_traitsIcEENS7_9allocatorIcEEEENS7_10shared_ptrIKNS_11SpriteImageEEEEEEDaT_EUlDpOT_E_NS7_5tupleIJSD_SJ_EEEEclEv+44): Routine _ZN4mbgl4util7RunLoop7InvokerIZNS0_6ThreadINS_10MapContextEE4bindIMS4_FvRKNSt3__112basic_stringIcNS7_11char_traitsIcEENS7_9allocatorIcEEEENS7_10shared_ptrIKNS_11SpriteImageEEEEEEDaT_EUlDpOT_E_NS7_5tupleIJSD_SJ_EEEEclEv at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/util/run_loop.hpp:113
Stack frame #05 pc 001a4758  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (mbgl::util::RunLoop::process()+352): Routine mbgl::util::RunLoop::process() at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/util/run_loop.cpp:27
Stack frame #06 pc 003f2444  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so: Routine uv__async_event at /home/travis/build/mapbox/mason/mason_packages/.build/libuv-1.7.5/src/unix/async.c:92
Stack frame #07 pc 003f2670  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so: Routine uv__async_io at /home/travis/build/mapbox/mason/mason_packages/.build/libuv-1.7.5/src/unix/async.c:137
Stack frame #08 pc 003fe33c  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so: Routine uv__io_poll at /home/travis/build/mapbox/mason/mason_packages/.build/libuv-1.7.5/src/unix/linux-core.c:345
Stack frame #09 pc 003f2bd8  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (uv_run+428): Routine uv_run at /home/travis/build/mapbox/mason/mason_packages/.build/libuv-1.7.5/src/unix/core.c:341
Stack frame #10 pc 001eca08  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (_ZN4mbgl4util6ThreadINS_10MapContextEE3runINSt3__15tupleIJRNS_4ViewERNS_10FileSourceERNS_7MapDataEEEEJLj0ELj1ELj2EEEEvNS0_13ThreadContextEOT_NS5_16integer_sequenceIjJXspT0_EEEE+144): Routine uv::loop::run() at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/util/uv_detail.hpp:70
Stack frame #11 pc 001ec904  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (_ZZN4mbgl4util6ThreadINS_10MapContextEEC1IJRNS_4ViewERNS_10FileSourceERNS_7MapDataEEEERKNS0_13ThreadContextEDpOT_ENKUlvE_clEv+220): Routine operator() at /home/leith/mb/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/util/thread.hpp:104
Stack frame #12 pc 001ec7d8  /data/app/com.mapbox.mapboxgl.testapp-1/lib/arm/libmapbox-gl.so (_ZNSt3__114__thread_proxyINS_5tupleIJZN4mbgl4util6ThreadINS2_10MapContextEEC1IJRNS2_4ViewERNS2_10FileSourceERNS2_7MapDataEEEERKNS3_13ThreadContextEDpOT_EUlvE_EEEEEPvSM_+88): Routine _ZNSt3__18__invokeIZN4mbgl4util6ThreadINS1_10MapContextEEC1IJRNS1_4ViewERNS1_10FileSourceERNS1_7MapDataEEEERKNS2_13ThreadContextEDpOT_EUlvE_JEEEDTclclsr3std3__1E7forwardIT_Efp_Espclsr3std3__1E7forwardIT0_Efp0_EEEOSK_DpOSL_ at /home/leith/mb/mapbox-gl-native/mason_packages/linux-x86_64/android-ndk/arm-9-r10e/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../include/c++/4.9/__functional_base:413
Stack frame #13 pc 000174e7  /system/lib/libc.so (__pthread_start(void*)+30)
Stack frame #14 pc 00015503  /system/lib/libc.so (__start_thread+6)

[ljbade]

The problem:
Map::removeSprite calls setSprite(name, nullptr)

This makes it's way to Holder.texture (which is nullptr) in SpriteAtlas::copy(), that function does not expect a nullptr.

Searching for removeSprite no platorms (including iOS) use this function. The only external usage is in the SpriteStore tests which does not use a SpriteAtlas.

@jfirebaugh has Map::removeSprite() been used before/tested? At the very least there needs to be a test case for it - with an active SpriteAtlas.

[jfirebaugh]

@ljbade I think your analysis is correct -- it's never been used outside of tests.

[ljbade]

@jfirebaugh Do you know roughly what is needed to fix this? I sort of understand SpriteAtlas but not completely.

We don't need to erase the data from the texture do we? We can just leave it there and delete the sprite from the list?

[ljbade]

After chat with @jfirebaugh , going to punt on this until @kkaefer gets some eyes on this.

Ideally an app can use removeSprite to free up atlas texture taken up by un-used sprites.

Currently the removeSprite crashes. If we fix to just remove the name from the list in SpriteStore and SpriteAtlas this will not free up the now un-used bit of atlas texture.

For example, I can imagine an app that adds markers with sprites loaded from the Internet of user account avatars (think a find my friend sort of thing), it will keep creating new sprites, removing the old ones, but eventually you will hit a max texture size if you keep the app open for long enough.

[tobrun]

Not relevant anymore due to introduction of MarkerViews. A user can still use SpriteStore but if there is a need to dynamically update these a user should use MarkerViews instead.