From aadda035364a5900d38e816b444284ad789e5832 Mon Sep 17 00:00:00 2001 From: Anand Thakker Date: Wed, 28 Jun 2017 07:39:21 -0400 Subject: [PATCH] Pause tile reloads when worker layer update is pending (#4893) Fixes #4738 --- src/source/source_cache.js | 68 ++++++++++++++++++--------- src/style/style.js | 10 +++- test/unit/source/source_cache.test.js | 44 ++++++++--------- test/unit/style/style.test.js | 61 ++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 46 deletions(-) diff --git a/src/source/source_cache.js b/src/source/source_cache.js index d267160922f..ccfc34bfe86 100644 --- a/src/source/source_cache.js +++ b/src/source/source_cache.js @@ -35,7 +35,7 @@ class SourceCache extends Evented { // for sources with mutable data, this event fires when the underlying data // to a source is changed. (i.e. GeoJSONSource#setData and ImageSource#serCoordinates) - if (this._sourceLoaded && e.dataType === "source" && e.sourceDataType === 'content') { + if (this._sourceLoaded && !this._paused && e.dataType === "source" && e.sourceDataType === 'content') { this.reload(); if (this.transform) { this.update(this.transform); @@ -50,7 +50,7 @@ class SourceCache extends Evented { this._source = Source.create(id, options, dispatcher, this); this._tiles = {}; - this._cache = new Cache(0, this.unloadTile.bind(this)); + this._cache = new Cache(0, this._unloadTile.bind(this)); this._timers = {}; this._cacheTimers = {}; this._maxTileCacheSize = null; @@ -97,16 +97,29 @@ class SourceCache extends Evented { return this._source; } - loadTile(tile, callback) { + pause() { + this._paused = true; + } + + resume() { + if (!this._paused) return; + const shouldReload = this._shouldReloadOnResume; + this._paused = false; + this._shouldReloadOnResume = false; + if (shouldReload) this.reload(); + if (this.transform) this.update(this.transform); + } + + _loadTile(tile, callback) { return this._source.loadTile(tile, callback); } - unloadTile(tile) { + _unloadTile(tile) { if (this._source.unloadTile) return this._source.unloadTile(tile); } - abortTile(tile) { + _abortTile(tile) { if (this._source.abortTile) return this._source.abortTile(tile); } @@ -139,13 +152,18 @@ class SourceCache extends Evented { } reload() { + if (this._paused) { + this._shouldReloadOnResume = true; + return; + } + this._cache.reset(); for (const i in this._tiles) { - this.reloadTile(i, 'reloading'); + this._reloadTile(i, 'reloading'); } } - reloadTile(id, state) { + _reloadTile(id, state) { const tile = this._tiles[id]; // this potentially does not address all underlying @@ -161,7 +179,7 @@ class SourceCache extends Evented { tile.state = state; } - this.loadTile(tile, this._tileLoaded.bind(this, tile, id, state)); + this._loadTile(tile, this._tileLoaded.bind(this, tile, id, state)); } _tileLoaded(tile, id, previousState, err) { @@ -221,7 +239,7 @@ class SourceCache extends Evented { * @returns {boolean} whether the operation was complete * @private */ - findLoadedChildren(coord, maxCoveringZoom, retain) { + _findLoadedChildren(coord, maxCoveringZoom, retain) { let found = false; for (const id in this._tiles) { @@ -308,7 +326,8 @@ class SourceCache extends Evented { */ update(transform) { this.transform = transform; - if (!this._sourceLoaded) { return; } + if (!this._sourceLoaded || this._paused) { return; } + let i; let coord; let tile; @@ -351,7 +370,7 @@ class SourceCache extends Evented { for (i = 0; i < visibleCoords.length; i++) { coord = visibleCoords[i]; - tile = this.addTile(coord); + tile = this._addTile(coord); retain[coord.id] = true; @@ -360,10 +379,10 @@ class SourceCache extends Evented { // The tile we require is not yet loaded. // Retain child or parent tiles that cover the same area. - if (!this.findLoadedChildren(coord, maxCoveringZoom, retain)) { + if (!this._findLoadedChildren(coord, maxCoveringZoom, retain)) { parentTile = this.findLoadedParent(coord, minCoveringZoom, retain); if (parentTile) { - this.addTile(parentTile.coord); + this._addTile(parentTile.coord); } } } @@ -383,12 +402,12 @@ class SourceCache extends Evented { // fadeEndTime is in the future, then this tile is still // fading in. Find tiles to cross-fade with it. if (typeof tile.fadeEndTime === 'undefined' || tile.fadeEndTime >= Date.now()) { - if (this.findLoadedChildren(coord, maxCoveringZoom, retain)) { + if (this._findLoadedChildren(coord, maxCoveringZoom, retain)) { retain[id] = true; } parentTile = this.findLoadedParent(coord, minCoveringZoom, parentsForFading); if (parentTile) { - this.addTile(parentTile.coord); + this._addTile(parentTile.coord); } } } @@ -408,7 +427,7 @@ class SourceCache extends Evented { // Remove the tiles we don't need anymore. const remove = util.keysDifference(this._tiles, retain); for (i = 0; i < remove.length; i++) { - this.removeTile(+remove[i]); + this._removeTile(+remove[i]); } } @@ -418,7 +437,7 @@ class SourceCache extends Evented { * @returns {Tile} the added Tile. * @private */ - addTile(tileCoord) { + _addTile(tileCoord) { let tile = this._tiles[tileCoord.id]; if (tile) return tile; @@ -438,7 +457,7 @@ class SourceCache extends Evented { const zoom = tileCoord.z; const overscaling = zoom > this._source.maxzoom ? Math.pow(2, zoom - this._source.maxzoom) : 1; tile = new Tile(tileCoord, this._source.tileSize * overscaling, this._source.maxzoom); - this.loadTile(tile, this._tileLoaded.bind(this, tile, tileCoord.id, tile.state)); + this._loadTile(tile, this._tileLoaded.bind(this, tile, tileCoord.id, tile.state)); } tile.uses++; @@ -452,7 +471,7 @@ class SourceCache extends Evented { const expiryTimeout = tile.getExpiryTimeout(); if (expiryTimeout) { this._timers[id] = setTimeout(() => { - this.reloadTile(id, 'expired'); + this._reloadTile(id, 'expired'); this._timers[id] = undefined; }, expiryTimeout); } @@ -474,7 +493,7 @@ class SourceCache extends Evented { * @returns {undefined} nothing * @private */ - removeTile(id) { + _removeTile(id) { const tile = this._tiles[id]; if (!tile) return; @@ -497,8 +516,8 @@ class SourceCache extends Evented { this._setCacheInvalidationTimer(wrappedId, tile); } else { tile.aborted = true; - this.abortTile(tile); - this.unloadTile(tile); + this._abortTile(tile); + this._unloadTile(tile); } } @@ -507,8 +526,11 @@ class SourceCache extends Evented { * @private */ clearTiles() { + this._shouldReloadOnResume = false; + this._paused = false; + for (const id in this._tiles) - this.removeTile(id); + this._removeTile(id); this._cache.reset(); } diff --git a/src/style/style.js b/src/style/style.js index dce72ba2e3e..be1ccc957d8 100644 --- a/src/style/style.js +++ b/src/style/style.js @@ -465,7 +465,12 @@ class Style extends Evented { // https://github.com/mapbox/mapbox-gl-js/issues/3633 const removed = this._removedLayers[id]; delete this._removedLayers[id]; - this._updatedSources[layer.source] = removed.type !== layer.type ? 'clear' : 'reload'; + if (removed.type !== layer.type) { + this._updatedSources[layer.source] = 'clear'; + } else { + this._updatedSources[layer.source] = 'reload'; + this.sourceCaches[layer.source].pause(); + } } this._updateLayer(layer); @@ -507,6 +512,7 @@ class Style extends Evented { this._updatedSymbolOrder = true; if (layer.source && !this._updatedSources[layer.source]) { this._updatedSources[layer.source] = 'reload'; + this.sourceCaches[layer.source].pause(); } } } @@ -720,6 +726,7 @@ class Style extends Evented { this._updatedLayers[layer.id] = true; if (layer.source && !this._updatedSources[layer.source]) { this._updatedSources[layer.source] = 'reload'; + this.sourceCaches[layer.source].pause(); } this._changed = true; } @@ -845,6 +852,7 @@ class Style extends Evented { } _reloadSource(id) { + this.sourceCaches[id].resume(); this.sourceCaches[id].reload(); } diff --git a/test/unit/source/source_cache.test.js b/test/unit/source/source_cache.test.js index 65d56818997..96c12344c9f 100644 --- a/test/unit/source/source_cache.test.js +++ b/test/unit/source/source_cache.test.js @@ -77,7 +77,7 @@ test('SourceCache#addTile', (t) => { } }); sourceCache.onAdd(); - sourceCache.addTile(coord); + sourceCache._addTile(coord); }); t.test('adds tile when uncached', (t) => { @@ -89,7 +89,7 @@ test('SourceCache#addTile', (t) => { t.end(); }); sourceCache.onAdd(); - sourceCache.addTile(coord); + sourceCache._addTile(coord); }); t.test('uses cached tile', (t) => { @@ -110,9 +110,9 @@ test('SourceCache#addTile', (t) => { tr.width = 512; tr.height = 512; sourceCache.updateCacheSize(tr); - sourceCache.addTile(coord); - sourceCache.removeTile(coord.id); - sourceCache.addTile(coord); + sourceCache._addTile(coord); + sourceCache._removeTile(coord.id); + sourceCache._addTile(coord); t.equal(load, 1); t.equal(add, 1); @@ -132,7 +132,7 @@ test('SourceCache#addTile', (t) => { sourceCache._setCacheInvalidationTimer = (id) => { sourceCache._cacheTimers[id] = setTimeout(() => {}, 0); }; - sourceCache.loadTile = (tile, callback) => { + sourceCache._loadTile = (tile, callback) => { tile.state = 'loaded'; tile.getExpiryTimeout = () => time; sourceCache._setTileReloadTimer(coord.id, tile); @@ -148,17 +148,17 @@ test('SourceCache#addTile', (t) => { t.notOk(sourceCache._timers[id]); t.notOk(sourceCache._cacheTimers[id]); - sourceCache.addTile(coord); + sourceCache._addTile(coord); t.ok(sourceCache._timers[id]); t.notOk(sourceCache._cacheTimers[id]); - sourceCache.removeTile(coord.id); + sourceCache._removeTile(coord.id); t.notOk(sourceCache._timers[id]); t.ok(sourceCache._cacheTimers[id]); - sourceCache.addTile(coord); + sourceCache._addTile(coord); t.ok(sourceCache._timers[id]); t.notOk(sourceCache._cacheTimers[id]); @@ -180,8 +180,8 @@ test('SourceCache#addTile', (t) => { }) .on('dataloading', () => { add++; }); - const t1 = sourceCache.addTile(coord); - const t2 = sourceCache.addTile(new TileCoord(0, 0, 0, 1)); + const t1 = sourceCache._addTile(coord); + const t2 = sourceCache._addTile(new TileCoord(0, 0, 0, 1)); t.equal(load, 2); t.equal(add, 2); @@ -197,9 +197,9 @@ test('SourceCache#removeTile', (t) => { t.test('removes tile', (t) => { const coord = new TileCoord(0, 0, 0); const sourceCache = createSourceCache({}); - sourceCache.addTile(coord); + sourceCache._addTile(coord); sourceCache.on('data', ()=> { - sourceCache.removeTile(coord.id); + sourceCache._removeTile(coord.id); t.notOk(sourceCache._tiles[coord.id]); t.end(); }); @@ -221,8 +221,8 @@ test('SourceCache#removeTile', (t) => { tr.height = 512; sourceCache.updateCacheSize(tr); - sourceCache.addTile(coord); - sourceCache.removeTile(coord.id); + sourceCache._addTile(coord); + sourceCache._removeTile(coord.id); t.end(); }); @@ -243,8 +243,8 @@ test('SourceCache#removeTile', (t) => { } }); - sourceCache.addTile(coord); - sourceCache.removeTile(coord.id); + sourceCache._addTile(coord); + sourceCache._removeTile(coord.id); t.equal(abort, 1); t.equal(unload, 1); @@ -705,7 +705,7 @@ test('SourceCache#clearTiles', (t) => { }); sourceCache.onAdd(); - sourceCache.addTile(coord); + sourceCache._addTile(coord); sourceCache.clearTiles(); t.equal(abort, 1); @@ -860,7 +860,7 @@ test('SourceCache#loaded (no errors)', (t) => { sourceCache.on('data', (e) => { if (e.sourceDataType === 'metadata') { const coord = new TileCoord(0, 0, 0); - sourceCache.addTile(coord); + sourceCache._addTile(coord); t.ok(sourceCache.loaded()); t.end(); @@ -879,7 +879,7 @@ test('SourceCache#loaded (with errors)', (t) => { sourceCache.on('data', (e) => { if (e.sourceDataType === 'metadata') { const coord = new TileCoord(0, 0, 0); - sourceCache.addTile(coord); + sourceCache._addTile(coord); t.ok(sourceCache.loaded()); t.end(); @@ -987,12 +987,12 @@ test('SourceCache reloads expiring tiles', (t) => { expiryDate.setMilliseconds(expiryDate.getMilliseconds() + 50); const sourceCache = createSourceCache({ expires: expiryDate }); - sourceCache.reloadTile = (id, state) => { + sourceCache._reloadTile = (id, state) => { t.equal(state, 'expired'); t.end(); }; - sourceCache.addTile(coord); + sourceCache._addTile(coord); }); t.end(); diff --git a/test/unit/style/style.test.js b/test/unit/style/style.test.js index 2f2b489ddba..ef5d98e167d 100644 --- a/test/unit/style/style.test.js +++ b/test/unit/style/style.test.js @@ -1,10 +1,12 @@ 'use strict'; const test = require('mapbox-gl-js-test').test; +const sinon = require('sinon'); const proxyquire = require('proxyquire'); const Style = require('../../../src/style/style'); const SourceCache = require('../../../src/source/source_cache'); const StyleLayer = require('../../../src/style/style_layer'); +const Transform = require('../../../src/geo/transform'); const util = require('../../../src/util/util'); const Evented = require('../../../src/util/evented'); const window = require('../../../src/util/window'); @@ -1012,6 +1014,65 @@ test('Style#moveLayer', (t) => { t.end(); }); +test('Style#setPaintProperty', (t) => { + t.test('#4738 postpones source reload until layers have been broadcast to workers', (t) => { + const style = new Style(util.extend(createStyleJSON(), { + "sources": { + "geojson": { + "type": "geojson", + "data": {"type": "FeatureCollection", "features": []} + } + }, + "layers": [ + { + "id": "circle", + "type": "circle", + "source": "geojson" + } + ] + })); + + const tr = new Transform(); + tr.resize(512, 512); + + style.once('style.load', () => { + style.update(); + style._recalculate(tr.zoom); + const sourceCache = style.sourceCaches['geojson']; + const source = style.getSource('geojson'); + + let begun = false; + let styleUpdateCalled = false; + + source.on('data', (e) => setImmediate(() => { + if (!begun && sourceCache.loaded()) { + begun = true; + sinon.stub(sourceCache, 'reload').callsFake(() => { + t.ok(styleUpdateCalled, 'loadTile called before layer data broadcast'); + t.end(); + }); + + source.setData({"type": "FeatureCollection", "features": []}); + style.setPaintProperty('circle', 'circle-color', {type: 'identity', property: 'foo'}); + } + + if (begun && e.sourceDataType === 'content') { + // setData() worker-side work is complete; simulate an + // animation frame a few ms later, so that this test can + // confirm that SourceCache#reload() isn't called until + // after the next Style#update() + setTimeout(() => { + styleUpdateCalled = true; + style.update(); + }, 50); + } + })); + }); + }); + + t.end(); +}); + test('Style#setFilter', (t) => { function createStyle() { return new Style({