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

Separate annotation sprite atlas from the style, proposed fix for #1488 #3049

Merged
merged 3 commits into from
Dec 1, 2015
Merged

Separate annotation sprite atlas from the style, proposed fix for #1488 #3049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5888684
[core] Move MapData storage to MapContext
jfirebaugh Nov 20, 2015
e1091db
[core] Use a separate atlas and store for annotation sprites
adam-mapbox Nov 17, 2015
622f669
[glfw] No longer necessary to wait for style load to add default marker
jfirebaugh Nov 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/mbgl/map/map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ class Map : private util::noncopyable {
private:
View& view;
const std::unique_ptr<Transform> transform;
const std::unique_ptr<MapData> data;
const std::unique_ptr<util::Thread<MapContext>> context;
MapData* data;

enum class RenderState {
never,
Expand Down
2 changes: 0 additions & 2 deletions include/mbgl/platform/default/glfw_view.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ class GLFWView : public mbgl::View {
std::array<uint16_t, 2> getFramebufferSize() const override;

void initialize(mbgl::Map *map) override;
void notifyMapChange(mbgl::MapChange) override;
void activate() override;
void deactivate() override;
void notify() override;
Expand Down Expand Up @@ -62,7 +61,6 @@ class GLFWView : public mbgl::View {
std::vector<std::string> spriteIDs;

private:
bool initializedDefaultMarker = false;
bool fullscreen = false;
const bool benchmark = false;
bool tracking = false;
Expand Down
8 changes: 1 addition & 7 deletions platform/default/glfw_view.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,7 @@ GLFWView::~GLFWView() {

void GLFWView::initialize(mbgl::Map *map_) {
View::initialize(map_);
}

void GLFWView::notifyMapChange(mbgl::MapChange change) {
if (change == mbgl::MapChange::MapChangeDidFinishLoadingMap && !initializedDefaultMarker) {
initializedDefaultMarker = true;
map->setSprite("default_marker", makeSpriteImage(22, 22, 1));
}
map->setSprite("default_marker", makeSpriteImage(22, 22, 1));
}

void GLFWView::onKey(GLFWwindow *window, int key, int /*scancode*/, int action, int mods) {
Expand Down
17 changes: 16 additions & 1 deletion src/mbgl/annotation/annotation_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ namespace mbgl {
const std::string AnnotationManager::SourceID = "com.mapbox.annotations";
const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points";

AnnotationManager::AnnotationManager() = default;
AnnotationManager::AnnotationManager(float pixelRatio)
: spriteStore(pixelRatio),
spriteAtlas(512, 512, pixelRatio, spriteStore) {
}

AnnotationManager::~AnnotationManager() = default;

AnnotationIDs
Expand Down Expand Up @@ -122,6 +126,7 @@ void AnnotationManager::updateStyle(Style& style) {
layer->sourceLayer = PointLayerID;
layer->layout.icon.image = std::string("{sprite}");
layer->layout.icon.allowOverlap = true;
layer->spriteAtlas = &spriteAtlas;

style.addLayer(std::move(layer));
}
Expand Down Expand Up @@ -152,4 +157,14 @@ void AnnotationManager::removeTileMonitor(AnnotationTileMonitor& monitor) {
monitors.erase(&monitor);
}

void AnnotationManager::setSprite(const std::string& name, std::shared_ptr<const SpriteImage> sprite) {
spriteStore.setSprite(name, sprite);
spriteAtlas.updateDirty();
}

double AnnotationManager::getTopOffsetPixelsForAnnotationSymbol(const std::string& name) {
auto sprite = spriteStore.getSprite(name);
return sprite ? -sprite->height / 2 : 0;
}

}
11 changes: 10 additions & 1 deletion src/mbgl/annotation/annotation_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#include <mbgl/annotation/annotation.hpp>
#include <mbgl/annotation/point_annotation_impl.hpp>
#include <mbgl/annotation/shape_annotation_impl.hpp>
#include <mbgl/sprite/sprite_store.hpp>
#include <mbgl/sprite/sprite_atlas.hpp>
#include <mbgl/util/geo.hpp>
#include <mbgl/util/noncopyable.hpp>

Expand All @@ -21,7 +23,7 @@ class Style;

class AnnotationManager : private util::noncopyable {
public:
AnnotationManager();
AnnotationManager(float pixelRatio);
~AnnotationManager();

AnnotationIDs addPointAnnotations(const std::vector<PointAnnotation>&, const uint8_t maxZoom);
Expand All @@ -31,6 +33,10 @@ class AnnotationManager : private util::noncopyable {
AnnotationIDs getPointAnnotationsInBounds(const LatLngBounds&) const;
LatLngBounds getBoundsForAnnotations(const AnnotationIDs&) const;

void setSprite(const std::string& name, std::shared_ptr<const SpriteImage>);
double getTopOffsetPixelsForAnnotationSymbol(const std::string& name);
SpriteAtlas& getSpriteAtlas() { return spriteAtlas; }

void updateStyle(Style&);

void addTileMonitor(AnnotationTileMonitor&);
Expand All @@ -48,6 +54,9 @@ class AnnotationManager : private util::noncopyable {
ShapeAnnotationImpl::Map shapeAnnotations;
std::vector<std::string> obsoleteShapeAnnotationLayers;
std::set<AnnotationTileMonitor*> monitors;

SpriteStore spriteStore;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be easier to have these objects living the MapContext object? They would survive a style change anyway and would have a more clear scope. The crash I'm seeing on Linux is related to destruction order, not threading issues.

We are calling glObjectStore.performCleanup() on MapContext::cleanup() just before calling ~MapContext() that will ultimately call the ~MapData() -> ~AnnotationManager() -> ~SpriteAtlas() that tries to dispose the textures but they never get cleaned up.

This patch fixes the crash on your branch: c15e1e6

Copy link
Contributor

Choose a reason for hiding this comment

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

They are specific to annotations, so I'd rather have them owned by AnnotationManager.

Your analysis of the issue sounds correct; however I would rather fix it by using std::unique_ptr<AnnotationManager> in MapData and resetting that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your analysis of the issue sounds correct; however I would rather fix it by using std::unique_ptr in MapData and resetting that.

That also should work.

SpriteAtlas spriteAtlas;
};

}
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/layer/symbol_layer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ std::unique_ptr<StyleLayer> SymbolLayer::clone() const {
result->copy(*this);
result->layout = layout;
result->paint = paint;
result->spriteAtlas = spriteAtlas;
return std::move(result);
}

Expand Down Expand Up @@ -178,7 +179,7 @@ std::unique_ptr<Bucket> SymbolLayer::createBucket(StyleBucketParameters& paramet
// needed by this tile.
if (!parameters.partialParse) {
bucket->addFeatures(parameters.tileUID,
parameters.spriteAtlas,
*spriteAtlas,
parameters.glyphAtlas,
parameters.glyphStore,
parameters.collisionTile);
Expand Down
4 changes: 4 additions & 0 deletions src/mbgl/layer/symbol_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace mbgl {

class SpriteAtlas;

class SymbolLayoutProperties {
public:
LayoutProperty<PlacementType> placement = PlacementType::Point;
Expand Down Expand Up @@ -94,6 +96,8 @@ class SymbolLayer : public StyleLayer {

SymbolLayoutProperties layout;
SymbolPaintProperties paint;

SpriteAtlas* spriteAtlas;
};

}
Expand Down
6 changes: 4 additions & 2 deletions src/mbgl/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ namespace mbgl {
Map::Map(View& view_, FileSource& fileSource, MapMode mapMode, GLContextMode contextMode, ConstrainMode constrainMode)
: view(view_),
transform(std::make_unique<Transform>(view, constrainMode)),
data(std::make_unique<MapData>(mapMode, contextMode, view.getPixelRatio())),
context(std::make_unique<util::Thread<MapContext>>(util::ThreadContext{"Map", util::ThreadType::Map, util::ThreadPriority::Regular}, view, fileSource, *data))
context(std::make_unique<util::Thread<MapContext>>(
util::ThreadContext{"Map", util::ThreadType::Map, util::ThreadPriority::Regular},
view, fileSource, mapMode, contextMode, view.getPixelRatio())),
data(&context->invokeSync<MapData&>(&MapContext::getData))
{
view.initialize(this);
update(Update::Dimensions);
Expand Down
25 changes: 8 additions & 17 deletions src/mbgl/map/map_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@

namespace mbgl {

MapContext::MapContext(View& view_, FileSource& fileSource, MapData& data_)
MapContext::MapContext(View& view_, FileSource& fileSource, MapMode mode_, GLContextMode contextMode_, const float pixelRatio_)
: view(view_),
data(data_),
dataPtr(std::make_unique<MapData>(mode_, contextMode_, pixelRatio_)),
data(*dataPtr),
asyncUpdate([this] { update(); }),
asyncInvalidate([&view_] { view_.invalidate(); }),
texturePool(std::make_unique<TexturePool>()) {
Expand Down Expand Up @@ -57,6 +58,7 @@ void MapContext::cleanup() {
style.reset();
painter.reset();
texturePool.reset();
dataPtr.reset();

glObjectStore.performCleanup();

Expand Down Expand Up @@ -241,7 +243,7 @@ bool MapContext::renderSync(const TransformState& state, const FrameData& frame)
glObjectStore.performCleanup();

if (!painter) painter = std::make_unique<Painter>(data, transformState);
painter->render(*style, frame);
painter->render(*style, frame, data.getAnnotationManager()->getSpriteAtlas());

if (data.mode == MapMode::Still) {
callback(nullptr, std::move(view.readStillImage()));
Expand All @@ -267,12 +269,7 @@ bool MapContext::isLoaded() const {

double MapContext::getTopOffsetPixelsForAnnotationSymbol(const std::string& symbol) {
assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
auto sprite = style->spriteStore->getSprite(symbol);
if (sprite) {
return -sprite->height / 2;
} else {
return 0;
}
return data.getAnnotationManager()->getTopOffsetPixelsForAnnotationSymbol(symbol);
}

void MapContext::setSourceTileCacheSize(size_t size) {
Expand All @@ -297,14 +294,8 @@ void MapContext::onLowMemory() {
}

void MapContext::setSprite(const std::string& name, std::shared_ptr<const SpriteImage> sprite) {
if (!style) {
Log::Info(Event::Sprite, "Ignoring sprite without stylesheet");
return;
}

style->spriteStore->setSprite(name, sprite);

style->spriteAtlas->updateDirty();
assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));
data.getAnnotationManager()->setSprite(name, sprite);
}

void MapContext::onTileDataChanged() {
Expand Down
6 changes: 5 additions & 1 deletion src/mbgl/map/map_context.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <mbgl/map/update.hpp>
#include <mbgl/map/transform_state.hpp>
#include <mbgl/map/map.hpp>
#include <mbgl/map/map_data.hpp>
#include <mbgl/style/style.hpp>
#include <mbgl/util/async_task.hpp>
#include <mbgl/util/gl_object_store.hpp>
Expand All @@ -27,9 +28,11 @@ struct FrameData {

class MapContext : public Style::Observer {
public:
MapContext(View&, FileSource&, MapData&);
MapContext(View&, FileSource&, MapMode, GLContextMode, const float pixelRatio);
~MapContext();

MapData& getData() { return data; }

void pause();

void triggerUpdate(const TransformState&, Update = Update::Nothing);
Expand Down Expand Up @@ -69,6 +72,7 @@ class MapContext : public Style::Observer {
void loadStyleJSON(const std::string& json, const std::string& base);

View& view;
std::unique_ptr<MapData> dataPtr;
MapData& data;

util::GLObjectStore glObjectStore;
Expand Down
1 change: 1 addition & 0 deletions src/mbgl/map/map_data.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class MapData {
: mode(mode_)
, contextMode(contextMode_)
, pixelRatio(pixelRatio_)
, annotationManager(pixelRatio)
, animationTime(Duration::zero())
, defaultFadeDuration(mode_ == MapMode::Continuous ? std::chrono::milliseconds(300) : Duration::zero())
, defaultTransitionDuration(Duration::zero())
Expand Down
4 changes: 3 additions & 1 deletion src/mbgl/map/tile_worker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ TileParseResult TileWorker::parsePendingLayers() {
if (layer.type == StyleLayerType::Symbol) {
auto symbolBucket = dynamic_cast<SymbolBucket*>(bucket.get());
if (!symbolBucket->needsDependencies(*style.glyphStore, *style.spriteStore)) {
symbolBucket->addFeatures(reinterpret_cast<uintptr_t>(this), *style.spriteAtlas,
const SymbolLayer* symbolLayer = dynamic_cast<const SymbolLayer*>(&layer);
symbolBucket->addFeatures(reinterpret_cast<uintptr_t>(this),
*symbolLayer->spriteAtlas,
*style.glyphAtlas, *style.glyphStore, *collisionTile);
insertBucket(layer.bucketName(), std::move(bucket));
pending.erase(it++);
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/renderer/painter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ void Painter::prepareTile(const Tile& tile) {
config.stencilFunc = { GL_EQUAL, ref, mask };
}

void Painter::render(const Style& style, const FrameData& frame_) {
void Painter::render(const Style& style, const FrameData& frame_, SpriteAtlas& annotationSpriteAtlas) {
frame = frame_;

glyphAtlas = style.glyphAtlas.get();
Expand Down Expand Up @@ -158,6 +158,7 @@ void Painter::render(const Style& style, const FrameData& frame_) {
spriteAtlas->upload();
lineAtlas->upload();
glyphAtlas->upload();
annotationSpriteAtlas.upload();

for (const auto& item : order) {
if (item.bucket && item.bucket->needsUpload()) {
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/renderer/painter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ class Painter : private util::noncopyable {
void changeMatrix();

void render(const Style& style,
const FrameData& frame);
const FrameData& frame,
SpriteAtlas& annotationSpriteAtlas);

// Renders debug information for a tile.
void renderTileDebug(const Tile& tile);
Expand Down
5 changes: 3 additions & 2 deletions src/mbgl/renderer/painter_symbol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ void Painter::renderSymbol(SymbolBucket& bucket, const SymbolLayer& layer, const
const float fontSize = properties.icon.size;
const float fontScale = fontSize / 1.0f;

spriteAtlas->bind(state.isChanging() || layout.placement == PlacementType::Line
SpriteAtlas* activeSpriteAtlas = layer.spriteAtlas;
activeSpriteAtlas->bind(state.isChanging() || layout.placement == PlacementType::Line
|| angleOffset != 0 || fontScale != 1 || sdf || state.getPitch() != 0);

if (sdf) {
Expand All @@ -202,7 +203,7 @@ void Painter::renderSymbol(SymbolBucket& bucket, const SymbolLayer& layer, const
layout.icon,
properties.icon,
1.0f,
{{ float(spriteAtlas->getWidth()) / 4.0f, float(spriteAtlas->getHeight()) / 4.0f }},
{{ float(activeSpriteAtlas->getWidth()) / 4.0f, float(activeSpriteAtlas->getHeight()) / 4.0f }},
*sdfIconShader,
&SymbolBucket::drawIcons);
} else {
Expand Down
11 changes: 9 additions & 2 deletions src/mbgl/sprite/sprite_atlas.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ SpriteAtlas::SpriteAtlas(dimension width_, dimension height_, float pixelRatio_,
pixelRatio(pixelRatio_),
store(store_),
bin(width_, height_),
data(std::make_unique<uint32_t[]>(pixelWidth * pixelHeight)),
dirty(true) {
std::fill(data.get(), data.get() + pixelWidth * pixelHeight, 0);
}

Rect<SpriteAtlas::dimension> SpriteAtlas::allocateImage(const size_t pixel_width, const size_t pixel_height) {
Expand Down Expand Up @@ -103,6 +101,11 @@ SpriteAtlasPosition SpriteAtlas::getPosition(const std::string& name, bool repea
}

void SpriteAtlas::copy(const Holder& holder, const bool wrap) {
if (!data) {
data = std::make_unique<uint32_t[]>(pixelWidth * pixelHeight);
std::fill(data.get(), data.get() + pixelWidth * pixelHeight, 0);
}

const uint32_t *srcData = reinterpret_cast<const uint32_t *>(holder.texture->data.data());
if (!srcData) return;
const vec2<uint32_t> srcSize { holder.texture->pixelWidth, holder.texture->pixelHeight };
Expand Down Expand Up @@ -189,6 +192,10 @@ void SpriteAtlas::updateDirty() {
}

void SpriteAtlas::bind(bool linear) {
if (!data) {
return; // Empty atlas
}

if (!texture) {
MBGL_CHECK_ERROR(glGenTextures(1, &texture));
MBGL_CHECK_ERROR(glBindTexture(GL_TEXTURE_2D, texture));
Expand Down
4 changes: 3 additions & 1 deletion src/mbgl/sprite/sprite_atlas.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ class SpriteAtlas : public util::noncopyable {
inline dimension getTextureWidth() const { return pixelWidth; }
inline dimension getTextureHeight() const { return pixelHeight; }
inline float getPixelRatio() const { return pixelRatio; }

// Only for use in tests.
inline const uint32_t* getData() const { return data.get(); }

private:
Expand All @@ -90,7 +92,7 @@ class SpriteAtlas : public util::noncopyable {
BinPack<dimension> bin;
std::map<Key, Holder> images;
std::set<std::string> uninitialized;
const std::unique_ptr<uint32_t[]> data;
std::unique_ptr<uint32_t[]> data;
std::atomic<bool> dirty;
bool fullUploadRequired = true;
GLuint texture = 0;
Expand Down
7 changes: 7 additions & 0 deletions src/mbgl/style/style.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <mbgl/map/map_data.hpp>
#include <mbgl/map/source.hpp>
#include <mbgl/map/transform_state.hpp>
#include <mbgl/layer/symbol_layer.hpp>
#include <mbgl/sprite/sprite_store.hpp>
#include <mbgl/sprite/sprite_atlas.hpp>
#include <mbgl/style/style_layer.hpp>
Expand Down Expand Up @@ -88,6 +89,12 @@ StyleLayer* Style::getLayer(const std::string& id) const {
}

void Style::addLayer(util::ptr<StyleLayer> layer) {
if (SymbolLayer* symbolLayer = dynamic_cast<SymbolLayer*>(layer.get())) {
if (!symbolLayer->spriteAtlas) {
symbolLayer->spriteAtlas = spriteAtlas.get();
}
}

layers.emplace_back(std::move(layer));
}

Expand Down
Loading