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

[core] Prefetch low resolution tiles #7741

Merged

tmpsantos merged 3 commits into master from tmpsantos-prefetch_low_res_tiles

Jul 7, 2017

Contributor

tmpsantos commented Jan 16, 2017

The idea is to try to cover more map area and show the user something ASAP while the other tiles are still loading.

Exposes 2 new APIs:

Dynamic: Will request tiles from current zoom level - N. Better resolution on the prefetched tiles.
Fixed: Will always request tiles from zoom level N regardless of the current zoom level. Allows optimize for bandwidth.

Fixes #1626

mention-bot commented Jan 16, 2017

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

tmpsantos requested a review from kkaefer

January 16, 2017 16:14

tmpsantos self-assigned this

tmpsantos added feature ⚠️ DO NOT MERGE labels

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch 5 times, most recently from fc8ce2b to da4e126 Compare

January 17, 2017 11:05

kkaefer suggested changes

View reviewed changes

include/mbgl/map/map.hpp Outdated

@@ @@ -185,6 +185,10 @@ class Map : private util::noncopyable { @@
                   std::vector<Feature> queryRenderedFeatures(const ScreenBox&,        const optional<std::vector<std::string>>& layerIDs = {});
                   AnnotationIDs queryPointAnnotations(const ScreenBox&);
+                  // Tile prefatching

Contributor

kkaefer Jan 17, 2017

typo

include/mbgl/map/map.hpp Outdated

@@ @@ -185,6 +185,10 @@ class Map : private util::noncopyable { @@
                   std::vector<Feature> queryRenderedFeatures(const ScreenBox&,        const optional<std::vector<std::string>>& layerIDs = {});
                   AnnotationIDs queryPointAnnotations(const ScreenBox&);
+                  // Tile prefatching
+                  void setFixedPrefetchZoom(uint32_t);
+                  void setDynamicPrefetchZoom(uint32_t);

Contributor

kkaefer Jan 17, 2017

Missing documentation

src/mbgl/style/source_impl.cpp Outdated

+                      // Request lower zoom level tiles (if configure to do so) in an attempt
+                      // to show something on the screen faster at the cost of a little of bandwidth.
+                      lowResolutionZoom = parameters.fixedPrefetchZoom ?

Contributor

kkaefer Jan 17, 2017

This breaks if fixedPrefetchZoom is set to 0, which is a valid use case.

src/mbgl/style/source_impl.cpp Outdated

+                      lowResolutionZoom = lowResolutionZoom < zoomRange.min ? zoomRange.min : lowResolutionZoom;
+                      if (lowResolutionZoom < idealZoom) {
+                          lowResolutionTiles = util::tileCover(parameters.transformState, lowResolutionZoom);

Contributor

kkaefer Jan 17, 2017

In previous implementations (JS), we've called this "panZoom". Not sure if that's a better name though.

Contributor Author

tmpsantos Jan 23, 2017

I'm OK with that. Let's use panZoom for the sake of consistency.

src/mbgl/style/source_impl.cpp Outdated

+                      if (retain.find(tile.id) == retain.end()) {
+                          retain.emplace(tile.id);
+                          tile.setNecessity(necessity);
+                      }

Contributor

kkaefer Jan 17, 2017

if (retain.emplace(tile.id).second) {
    tile.setNecessity(necessity);
}

.second is true when something was inserted, false otherwise.

src/mbgl/style/source_impl.cpp Outdated

+                      // to show something on the screen faster at the cost of a little of bandwidth.
+                      lowResolutionZoom = parameters.fixedPrefetchZoom ?
+                          parameters.fixedPrefetchZoom :
+                          idealZoom - parameters.dynamicPrefetchZoom;

Contributor

kkaefer Jan 17, 2017

possible integer overflow here if idealZoom < parameters.dynamicPrefetchZoom?

src/mbgl/style/source_impl.cpp Outdated

+                          parameters.fixedPrefetchZoom :
+                          idealZoom - parameters.dynamicPrefetchZoom;
+                      lowResolutionZoom = lowResolutionZoom < zoomRange.min ? zoomRange.min : lowResolutionZoom;

Contributor

kkaefer Jan 17, 2017

Can we use std::min to make this more readable?

src/mbgl/style/update_parameters.hpp Outdated

@@ @@ -42,6 +46,9 @@ class UpdateParameters { @@
                   // TODO: remove
                   Style& style;
+                  const uint32_t fixedPrefetchZoom;
+                  const uint32_t dynamicPrefetchZoom;

Contributor

kkaefer Jan 17, 2017

What is the use case for having both fixed and dynamic prefetch? Which one takes precedence?

Contributor Author

tmpsantos Jan 23, 2017

Documented now. Fixed takes precedence.

src/mbgl/util/tile_cover.cpp Outdated

    
              int32_t coveringZoomLevel(double zoom, SourceType type, uint16_t size) {

                  zoom += std::log(util::tileSize / size) / std::log(2);

              uint32_t coveringZoomLevel(double zoom, SourceType type, uint16_t size) {

                  zoom = std::max(0., zoom) + std::log(util::tileSize / size) / std::log(2);

Contributor

kkaefer Jan 17, 2017

Should this max call include the addition (which accounts for satellite tile size differences)? Otherwise, it could be impossible to use z0 for satellite tiles.

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch 6 times, most recently from d6b1bfe to 3cc2f15 Compare

January 24, 2017 00:20

tmpsantos removed the ⚠️ DO NOT MERGE label

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch from 3cc2f15 to e033cf8 Compare

January 24, 2017 16:12

Contributor Author

tmpsantos commented Jan 24, 2017

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch from e033cf8 to a991620 Compare

January 25, 2017 09:11

tmpsantos added the ✓ ready for review label

kkaefer self-requested a review

January 27, 2017 16:19

tmpsantos requested a review from jfirebaugh

January 27, 2017 20:22

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch 2 times, most recently from ae6d2a2 to dfd719a Compare

February 3, 2017 10:30

Contributor Author

tmpsantos commented Feb 13, 2017

Should I close this as deferred?

jfirebaugh reviewed

View reviewed changes

Contributor

jfirebaugh left a comment

I tested this out with a dynamicPrefetchZoomDelta of 3, and it does exacerbate #7026.

Without prefetching, the flickering occurs only if you have tiles in the disk cache at a lower zoom level than the area you are panning over. With prefetching, the flickering always occurs.

So I think we should hold this feature until we fix #7026.

src/mbgl/map/map.cpp Outdated

		@@ -95,6 +95,9 @@ class Map::Impl : public style::Observer {

		std::unique_ptr<AsyncRequest> styleRequest;

		int32_t fixedPrefetchZoom = -1;

Contributor

jfirebaugh Feb 14, 2017

To indicate the lack of a value, use optional rather than negative integers.

Contributor Author

tmpsantos Apr 4, 2017

Done.

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch 2 times, most recently from 438a29d to a49c473 Compare

April 4, 2017 17:46

Contributor Author

tmpsantos commented Apr 6, 2017 •

edited

Loading

@kkaefer is disabling partial tile rendering enough for landing this patch?

I did a cheap test by simply tweaking isRenderable(), and it does improve flickering, but pretty much makes prefetching a requirement for areas with a lot of symbols.

+++ b/src/mbgl/tile/tile.hpp
@@ -74,7 +74,7 @@ public:
     // partial state is still waiting for network resources but can also
     // be rendered, although layers will be missing.
     bool isRenderable() const {
-        return availableData != DataAvailability::None;
+        return isComplete();
}

That said, nothing stops us for landing this today, since it is still very useful for a satellite map and would spare me from rebasing this branch and it is optional anyway.

kkaefer suggested changes

View reviewed changes

Contributor

kkaefer left a comment

is disabling partial tile rendering enough for landing this patch?

No. Disabling partial rendering with the patch you suggested means that older/outdated buckets can't be used either. That means that rotating a map makes it appear blank until the buckets are reparsed.

kkaefer added the Core label

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch from a49c473 to db83506 Compare

June 26, 2017 16:10

Contributor Author

tmpsantos commented Jun 27, 2017

@kkaefer rebased and ready for reviewing again. This new version enables prefetching only for raster until we fix the label flickering. On this gif you see the difference (the tiles are offline cached on both scenarios, it is even more evident when you get tiles from the network).

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch from db83506 to 5d960d5 Compare

June 29, 2017 07:22

kkaefer reviewed

View reviewed changes

src/mbgl/renderer/tile_pyramid.cpp

+                      algorithm::updateRenderables(getTileFn, createTileFn, retainTileFn,
+                              [](const UnwrappedTileID&, Tile&) {}, panTiles, zoomRange, panZoom);
+                  }

Contributor

kkaefer Jul 3, 2017

Can we merge this call to updateRenderables into the other one by merging the panTiles into idealTiles?

Contributor Author

tmpsantos Jul 5, 2017

Capturing from the chat: we have a separated algorithm::updateRenderables because we don't create render tiles for pan tiles.

include/mbgl/map/map.hpp Outdated

+                  optional<uint8_t> getFixedPrefetchZoom() const;
+                  void setDynamicPrefetchZoomDelta(optional<uint8_t> delta);
+                  optional<uint8_t> getDynamicPrefetchZoomDelta() const;

Contributor

kkaefer Jul 3, 2017

Thanks for adding the documentation, but I'm still not understanding why we need two different APIs for this. Can you please elaborate?

Contributor Author

tmpsantos Jul 4, 2017

The fixed is if you want to be cheap: you can set the prefetch zoom level to say, zoom level 5 and you will never have a blank spot on the map, but it will look very low res and overscaled. Soon you will get the whole world at zoom 5 in your cache.

The dynamic IMO is more interesting, but it will use more resources. You could set it to say, 2, and it will always prefetch "CurrentZoom - 2", it will pretty much make you never see blank spots with better looking overscaled tile but it will fill the cache faster and also use more bandwidth.

I find both approaches useful. We could drop the "dynamic" API and leave it to be implemented client side if you think we don't need 2 APIs.

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch 2 times, most recently from bef817d to 87ad0f4 Compare

July 6, 2017 13:26

brunoabinader mentioned this pull request

Tile coverage updates #9284

Closed

tmpsantos and others added 3 commits

July 6, 2017 15:56


          [core] Prefetch low resolution tiles

b33a892


          [tests] Add unit test for tile prefetching

86bb87f


          [core] Fix sorting order for render tiles

05f548c

Sort by z order, so lower res tiles don't get rendered
over high res tiles.

tmpsantos force-pushed the tmpsantos-prefetch_low_res_tiles branch from 87ad0f4 to 05f548c Compare

July 6, 2017 13:57

Contributor

1ec5 commented Jul 6, 2017

Would you like to implement the SDK side of this change, or should we track that in separate tickets?

Contributor Author

tmpsantos commented Jul 6, 2017

Would you like to implement the SDK side of this change, or should we track that in separate tickets?

I would like to track it in a separated ticket.

This was referenced Jul 6, 2017

Add option to prefetch low-resolution tiles #9437

Closed

Add option to prefetch low-resolution tiles #9438

Closed

Contributor

1ec5 commented Jul 6, 2017

#9437 for Android; #9438 for iOS and macOS.

kkaefer approved these changes

View reviewed changes

tmpsantos merged commit e962b79 into master

tmpsantos deleted the tmpsantos-prefetch_low_res_tiles branch

July 7, 2017 11:28

jfirebaugh mentioned this pull request

Offline raster tiles does not use correct zoom level #9563

Closed

brunoabinader mentioned this pull request

[core] Allow prefetching tiles for all source types #10548

Merged

brunoabinader mentioned this pull request

Tile coverage updates #12310

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels