Fix raster tiles flicker & hang

[lucaswoj]

fixes #2208
fixes #2201

cc @jfirebaugh @ansis @mourner

[lucaswoj]

6171a2f makes me think this might be the wrong solution.

[ansis]

This looks like the right solution! I think this broke with 4f99e94

[lucaswoj]

@ansis I amended this PR to fix both bugs. ?

[ansis]

In what cases are tiles added to the cache while they are already in the cache? Is that a bug?

[lucaswoj]

It happened rarely during very fast zooms, probably due to a race condition. The LRU cache shouldn't get stuck in an infinite loop if the same key is added twice. We can debug the race condition separately. I want to get this out in a release ASAP.

[lucaswoj]

You're right. Reading the code, it is mysterious that this method could ever add the same tile twice.

mapbox-gl-js/js/source/tile_pyramid.js

Line 359 in e7b551c

this._cache.add(tile.coord.wrapped().id, tile);

[lucaswoj]

I have a hunch that the duplicate tiles have the same root cause as #1834. Our re-use of tiles across the dateline isn't perfect. Because the LRU cache uses wrapped coords but the tile pyramid doesn't, we might add duplicate entries for a wrapped coord.

[ansis]

Yeah, your hunch sounds right. I'll take a quick look to see if I can find something that regressed recently otherwise we can merge this to fix the fire and then properly fix it later.

[lucaswoj]

Did you mention that we were (unintentionally) not using the cache at all until a recent fix?

[ansis]

Did you mention that we were (unintentionally) not using the cache at all until a recent fix?

We were using it, but only for the tiles at the current zoom. e6cb15c makes it look for parents too, which could have introduced this bug, or it could have just made it easier to hit.

[ansis]

Could this be it?

• a tile across the dateline is need for the current view, addTile is called for it.
• this does not return a tile because the tile has never been added for "world 0"
• it gets added to this._tiles
• you pan and now need the "world 0" version of that tile as well, call addTile for it
• all the checks for existing tiles fail and it gets loaded instead of sharing the first version
• you now have tiles in this._tiles that have distinct Tile objects but the same tile.coord.wrapped().id
• both will be added to the cache with this wrapped id when they are removed

Ideally c33ad33 isn't needed but it is a safe fix. Merging now, releasing and then properly fixing later sounds good!

[ryanbaumann]

@lucaswoj @ansis
In version 0.15.0, the flicker and pan/zoom performance is improved.

However, I still notice flickering layers on Satellite maps with Map Pitch > 0 while panning, zooming, rotating on Chrome w/ Nvidia GTX680. Example below:

Video Example

JSFiddle

I only see the flicker on Satellite maps.

[ansis]

@ryanbaumann thanks for the video and jsfiddle. It looks like a different bug so I've opened a new issue: #2212