[core] Fix symbol flickering during tile reload

[jfirebaugh]

Discard prior symbol buckets only when new symbol buckets became available, in order to eliminate flickering when tiles are refreshed. Plus followup refactor.

Fixes #7615

[mention-bot]

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @ansis, @incanus and @tmpsantos to be potential reviewers.

[kkaefer]

This also fixes a potential issue that could occur when after a relayout, a layer doesn't have any data anymore. In this case, it wouldn't produce a new bucket, so the old bucket would not be overwritten.

[kkaefer]

Could you please use branch names that are unambiguous? Using a digit-only branch name makes it harder to check out the branch locally.

[kkaefer]

Could we change this to std::shared_ptr<SymbolBucket>?

[jfirebaugh]

We could, however it would make GeometryTile::getBucket longer because it couldn't use layer.is<SymbolLayer>() ? symbolBuckets : nonSymbolBuckets. Not sure if it's worth it.

[kkaefer]

I tested this with a database that has prior cache data in my testing area (Berlin), and ran the following SQL command to simulate expired + non-304 responses from the server: UPDATE tiles SET expires = 1483675317, etag = "xxx";. When loading the map, I'm still seeing entire loaded, but expired tiles vanish and gradually reappear. It looks like this patch solves the particular case of relayouting, but doesn't solve the flicker for expired tiles replacing the previous tile.

Here's a frame sequence I recorded with this patch: http://bl.ocks.org/kkaefer/raw/a422cc89a78917aaf718fe022c61bfe8/

[kkaefer]

This pull request fixes a part of the issue; the other part of the issue was introduced just a few days ago in 99c1a0e#diff-7c17b75ae445adc15c24e3f167193e4eR217:

When a tile in the cache is expired, we start a refresh request. In many cases, we're getting a 304 response (not modified) back, in which case we don't do anything. However, when the tile changed, we're calling GeometryTile::setData(), which triggers GeometryTileWorker::setData() on the worker thread. However, this is a reused worker, that has processed the previous (expired) tile already. Back when it processed the first tile, it eventually ran GeometryTileWorker::redoLayout and this line:

std::vector<std::vector<std::unique_ptr<Layer>>> groups = groupByLayout(std::move(*layers));

This means that it moved the layer information objects out of itself, and through some hoops into the SymbolLayout object. When reparsing the tile with new data, setLayers isn't called (since nothing about them changed), but the layers vector optional is engaged, the the actual vector is empty, meaning that the layout process will not produce any buckets. This empty bucket list now overwrites the previous (stale, but parsed) buckets. They reappear because immediately after invoking GeometryTileWorker::setData(), we're also calling redoLayout(), which gets a fresh set of cloned layer object over to the worker, and starts processing again.

I implemented a fix in 7615...7615-reset-layers-after-redolayout, which works by disengaging the optional that holds the (now empty) layer objects.

[kkaefer]

Incomplete fix

[jfirebaugh]

Thanks for tracking down #7586 as a cause of flickering. Indeed, GeometryTileWorker shouldn't have been consuming the layers like that. The intent is that GeometryTileWorker::set{Layers,Data,PlacementConfig} can be invoked any number of times, in any order, and GeometryTileWorker will do the right thing. In particular GeometryTileWorker shouldn't assume that GeometryTile is going to call redoLayout directly after invoking setData. I want to replace GeometryTile::redoLayout with GeometryTile::setLayers, so that the public GeometryTile interface is oriented around what data has changed rather than what needs to happen in response to something changing, which should be entirely the responsibility of the implementation.

[kkaefer]

Is it guaranteed that layerIDs always has at least one element?

[jfirebaugh]

That invariant should hold: the layer IDs are constructed from the layer group, and if there's no group, no SymbolLayout will be created. However, just in case it doesn't, I used at(0) here rather than [0] to get an exception rather than invalid memory access.

[kkaefer]

what data has changed rather than what needs to happen in response to something changing

Yeah, agreed. There's a call to redoLayout(); in GeometryTile::setData(...). Instead of sending over the layers every time we call setData(), we should call redoLayout() when the object is constructed. With an expired tile, we right now call redoLayout() on the worker side 3×: once as part of the initial (expired) setData, once when calling setData with the fresh data, and then immediately after that again when calling redoLayout after the setData call.