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

[NOT READY] raster tile rendering #84

Closed
wants to merge 43 commits into from
Closed

[NOT READY] raster tile rendering #84

wants to merge 43 commits into from

Conversation

incanus
Copy link
Contributor

@incanus incanus commented Feb 28, 2014

This is a first cut at raster tile rendering for #70. Work in progress.

  • Raster handling has been separated from sprite loading so as to be reused by tiles as well.
  • Raster tiles are drawn at 512px, so they don't look great. Doing 256px tiles makes them look great, but of course is not good for vector performance. I'm going to see if there's anything I can borrow from WebGL here.
  • We clearly need styling for rasters, as well as combination with vectors in the rendering. Right now rasters are just straight passthroughs in the shader.
  • Raster textures aren't disposed of yet. There are probably other memory issues.
  • Only PNGs are handled. Time to revisit platform-specific image decoding before adding libjpeg into the mix @kkaefer @springmeyer?

anigif-1393570951

@incanus
Copy link
Contributor Author

incanus commented Mar 7, 2014

My main bottleneck currently seems to be texture binding, but not a lot can be done directly when arbitrary textures are coming in over the network (e.g. preparing a texture atlas). I'm experimenting with using up to GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS (8 on iOS) based on tile->clip_id but that doesn't seem likely to help given a scenario like retina iPad ~50 tiles/textures per screen.

@kkaefer any thoughts offhand for good directions to explore?

I'll keep at it and see if I can narrow down other useful points of optimization. I had considered not doing anything with the stencil buffer during raster calls, but this won't likely work when we get to the point where we want to combine raster + vector imagery in the same render loop, so instead I'm just doing this for now:

if (tile->use_raster)
    renderRaster(tile);
else
    renderLayers(tile, style.layers);

if (settings.debug)
    renderDebug(tile);

renderBackground();

@incanus
Copy link
Contributor Author

incanus commented Mar 7, 2014

If I'm understanding things properly, multiple texture units aren't relevant unless a shader is using more than one texture within it.

Currently trying to group texture loads and/or binds near the beginning of the frame and/or deferring actual draw of a texture until a future render loop if it needs to be loaded first.

@incanus
Copy link
Contributor Author

incanus commented Mar 8, 2014

Latest on this:

  • Binding textures isn't going to go away, if we have to render upwards of 50 textures on the screen in a render pass.
  • I built a texture manager, and 32 textures rotated through for use by glBindTexture seems to be working pretty well, to avoid the overhead of glGenTextures inside the render loop.
  • I'm now working with a local, cached, repetitive image tile for now in order to optimize the CPU first.
  • Some CPU/memory-based improvements I've made to performance include:
    • Platform-based limiting of simultaneous network connections via HTTPMaximumConnectionsPerHost = 2 and better cache use with 100MB of disk capacity and requestCachePolicy = NSURLRequestReturnCacheDataElseLoad.
    • Experimenting with changes to min_covering_zoom when calling findLoadedParent, which is a big bottleneck with the default of 10 during zooming out.
    • Avoiding calling findLoadedChildren and findLoadedParent when (transform.rotating || transform.scaling || transform.panning).
    • Avoiding calling glDeleteTextures or otherwise loading/unloading textures when (transform.rotating || transform.scaling || transform.panning).
    • Calling free(img) immediately after texture loading instead of waiting for raster (i.e. tile object) destruction.
  • We'll eventually need a center-weighted tile load sorting algorithm at some point for aesthetic reasons.
  • I'm also profiling some severe memory leaks in the use of libpng (not sure yet if it's only in my specific multi-use or if this exists in the master single sprite use, too) so I may take up my temporarily-abandoned platform-specific calls for PNG (and later, JPEG etc.) parsing (or just fix them, of course).

@kkaefer
Copy link
Member

kkaefer commented Mar 8, 2014

We'll eventually need a center-weighted tile load sorting algorithm at some point for aesthetic reasons.

This should be in master since ba5070b

@incanus
Copy link
Contributor Author

incanus commented Mar 10, 2014

Nice, thanks for the heads up. Been trying to merge weekly but may need to up that.

 - simplify namespacing
 - static raster tile image for now
@incanus
Copy link
Contributor Author

incanus commented Mar 12, 2014

Working through pretty substantial conflicts on merging master right now.

Conflicts:
	include/llmr/map/map.hpp
	include/llmr/map/tile.hpp
	include/llmr/platform/platform.hpp
	include/llmr/renderer/painter.hpp
	src/map/map.cpp
	src/map/tile.cpp
	src/renderer/painter.cpp
	src/style/sprite.cpp
@incanus
Copy link
Contributor Author

incanus commented Mar 13, 2014

Ok, that was a pretty epic merge. @kkaefer couple things FYI I had to modify about my new interaction with master, which I'm pretty sure will stay solid through finishing this feature branch:

  • llmr::covering_tiles() needs to know about llmr::Tile because with raster tiles, it needs to multiply the number of tiles in each dimension by 2 (for ordinary) or 4 (for retina), assuming 512px tile size, so it needs access to Tile::children() if we don't want to duplicate that functionality. So I moved from vec3<uint32_t> back to Tile::ID there.
  • Can you take a look at the semantics around accessing this->texturer in https://github.com/mapbox/llmr-native/blob/94209350e48d943f864a2af813a900dcf8f858db/src/map/map.cpp#L425-L435 and make sure I'm not doing anything dumb? Wasn't entirely sure of the syntax there to safely capture for the lambda.

#include <forward_list>

namespace llmr {

std::forward_list<vec3<int32_t>> covering_tiles(int32_t zoom, const box& points);
std::forward_list<Tile::ID> covering_tiles(int32_t zoom, const box& points, const bool use_raster = false, const bool use_retina = false);
Copy link
Member

Choose a reason for hiding this comment

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

In our code, we shouldn't have a binary distinction between non-retina and retina, but rather use a floating point scale factor. Binary retina makes sense for UI elements where scaling things is difficult, but we should be able to scale without weird half-pixel lines etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing a bool for this was because of separation between the map/transform and the map coverage algorithm (coverage) and decision of which raster version to fetch (tile_data). This leaves the map and transform, which are responsible for all drawing/tiling, as well as have access to pixel_ratio, to make the determination of whether retina raster tiles are required. Raster tiles are binary -- either they are retina, or they are not.

@kkaefer
Copy link
Member

kkaefer commented Mar 17, 2014

  • When zooming in/out, it doesn't retain children/parents until the new (better fitting) tiles are loaded
  • When fading in tiles, they sometimes stay halfway faded in and pop to fully faded in as soon as you move the map

@kkaefer
Copy link
Member

kkaefer commented Mar 17, 2014

  • The TexturePool destructor crashes on my system

@incanus
Copy link
Contributor Author

incanus commented Mar 17, 2014

For tile retention, yes, this needs work.

I will also investigate the TexturePool issue. @kkaefer have a stack trace?

Otherwise it sounds like this is good to merge very soon, then we'll pick up improvement work in master.

@kkaefer
Copy link
Member

kkaefer commented Mar 18, 2014

@incanus
Copy link
Contributor Author

incanus commented Mar 18, 2014

The TexturePool destructor crashes on my system

I see this on OS X, but only on app quit. Did you experience it otherwise @kkaefer?

@incanus
Copy link
Contributor Author

incanus commented Mar 18, 2014

Ok, I hit the smaller-scale fixes that I could inline + have the rest queued up for post-merge ticketing. Will work on squash + merge now.

Conflicts:
	include/llmr/map/coverage.hpp
	macosx/main.mm
	src/map/coverage.cpp
	src/map/tile.cpp
@incanus
Copy link
Contributor Author

incanus commented Mar 19, 2014

Squash-merged into master as of 00a3245.

@incanus incanus closed this Mar 19, 2014
@incanus incanus deleted the raster-work branch March 19, 2014 00:02
@mikemorris
Copy link
Contributor

What tools are you using for profiling @incanus @springmeyer @kkaefer?

@kkaefer
Copy link
Member

kkaefer commented Apr 15, 2014

@mikemorris Instruments.app

@springmeyer
Copy link
Contributor

Hint: use iprofiler to run from the command line when this is useful. It then dumps files which can be visualized in instruments.app.

On Apr 15, 2014, at 8:57 AM, Konstantin Käfer notifications@github.com wrote:

@mikemorris Instruments.app


Reply to this email directly or view it on GitHub.

@incanus
Copy link
Contributor Author

incanus commented Apr 21, 2014

Same, only Instruments.app for me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants