Unified canvas rendering #6082

Merged
merged 5 commits into from Dec 6, 2016

Projects

None yet

6 participants

@ahocevar
Member
ahocevar commented Nov 6, 2016 edited

This pull request is the result of a cleanup and refactoring of the canvas renderer. In particular, its goals are:

  • Share as much code as possible between the layer renderers.
  • Use transforms instead of pixel calculations wherever possible. It turned out that pixel calculations for tile positioning were hard to understand and error prone.
  • No more direct composition of tiles to the target canvas - the performance gain was minimal, at the cost of code that was hard to read and understand, and poor rendering quality during zoom animations. The new image composition code is more efficient than the one we had before the direct composition refactoring. It creates a canvas with all tiles that intersect the view extent and reuses it as long as that extent fully contains the view extent.
  • Avoid overlapping rendering of vector tiles of different resolutions during loading. This is done by rendering replays for high resolution tiles first, and cut extents of already rendered high resolution tiles out of lower resolution tiles that are rendered later.

This refactoring also fixes several bugs:

Fixes #5690.
Fixes #5936 (alternative to #5999).
Fixes #5821.
Fixes #5601.

@ahocevar ahocevar changed the title from Unified canvas rendering to [WIP] Unified canvas rendering Nov 7, 2016
@ahocevar ahocevar changed the title from [WIP] Unified canvas rendering to Unified canvas rendering Nov 7, 2016
@ahocevar
Member
ahocevar commented Nov 7, 2016

This is now ready for review.

@tschaub
Member
tschaub commented Nov 8, 2016

Impressive work @ahocevar. I haven't found time for a proper review yet. I'd also like to bring a build of this branch into an application where we often render 30-40 tile layers at once. I was guessing that we were benefitting from not creating an intermediate canvas for each. I should be able to benchmark and profile the two approaches.

@tschaub
Member
tschaub commented Nov 11, 2016 edited

Here are some rendering results with built versions of the library before and after this change.

First, two animated zoom loops with a full tile cache.

Zooming with the single canvas (old) approach:

old

Zooming with the multiple canvas (new) approach:

new

Hard to convey in an animated gif, but if feels like there is a higher frame rate with the old approach.

@tschaub
Member
tschaub commented Nov 11, 2016

Here are two timelines during zoom animations. I've zoomed in on a single animated transition in both cases. The second section (with horizontal pink bars) shows the frame time.

Zooming with the single canvas (old) approach:

old

Zooming with the multiple canvas (new) approach:

new

In the second timeline, there are more "long" frames (e.g. the 110ms frames or ~9fps). The third section of the charts highlights the JS heap over time.

@tschaub
Member
tschaub commented Nov 11, 2016

Not sure if there is too much to surmise from the graphics above. These are perhaps extreme examples, with 30 layers in each case. Ordinarily, I think that number of layers would be hard to justify - we've been taking advantage of the fact that there is not much additional cost per layer with the current (or old) approach. We can rework this particular example to use server-side tile composition, resulting in far fewer client-side layers being visible at once.

Clearly there is value in addressing the rendering issues that motivated this change. So perhaps that's enough to outweigh any drop in rendering performance (this drop might not be noticeable in most cases).

@ahocevar
Member
ahocevar commented Nov 12, 2016 edited

Thanks for the testing and benchmarks @tschaub.

Tough decision. A drop in performance is not a good thing, but 30 layers is indeed a lot. I tried a map with 4 layers, and didn't feel much difference. Maybe @marcjansen can also weigh in here, because I know he's been struggling with performance on maps with many layers as well.

In addition to what @tschaub said, This change also makes the tile renderer code much easier to maintain. Maybe also something to consider.

@tschaub
Member
tschaub commented Nov 12, 2016

I've reviewed the code changes and think they look great.

I'll admit that I still don't get why composition to an intermediate canvas is required. I wonder if there is a simple way to demonstrate what is possible by rendering to two intermediate canvases and then composing to a third that would not be possible by rendering directly to a single canvas. I can imagine cases involving opacity changes on the intermediate canvases (where we might have overlapping images rendered) - but I don't think that is the kind of thing we're trying to solve here.

@ahocevar is it possible to illustrate the kind of thing that is not possible with direct composition in a simple static example? I understand if this would take too much effort to produce, but it would be great to be able to illustrate the problem very directly. Are we dealing with bugs/limitations in the Canvas implementations in certain browsers? Or is this about something that is really not possible with the Canvas API?

My comments shouldn't be taken as blocking this change. The code looks really nice. I'm just not the right one to push for it to get in since I have not been hit by any of the existing bugs that this addresses and am hit by the drop in performance. If this improves things for most people, then I'm in favor. I would just be a bit more satisfied if I understood that the existing problems cannot be solved without a performance hit.

@ahocevar
Member
ahocevar commented Nov 12, 2016 edited

@tschaub Simply put, the problem with rendering tiles directly to the map canvas is that as soon as a scale or rotate transform is applied to the map canvas when rendering, you get gaps between tiles. I'll set up an example to show the problem.

@tschaub
Member
tschaub commented Nov 12, 2016

Curious if we could scale and rotate the map canvas in CSS. This would mean generating a larger map canvas than the viewport - but that is also required by the intermediate canvas alternative.

@ahocevar
Member

Curious if we could scale and rotate the map canvas in CSS. This would mean generating a larger map canvas than the viewport - but that is also required by the intermediate canvas alternative.

This should work, and it would indeed solve the problem I mentioned above.

@ahocevar
Member
ahocevar commented Nov 13, 2016 edited

I thought the CSS scaling and rotation approach through. Rotation would work, but scaling wouldn't. This is because we could have tile layers with different nominal tile resolutions (depending on their tile grids). So CSS scaling could be applied when rendering at fractional view zoom levels, but we'd still have a problem with fractional tile source zoom levels.

@ahocevar ahocevar changed the title from Unified canvas rendering to [WIP] Unified canvas rendering Nov 13, 2016
@ahocevar
Member
ahocevar commented Nov 13, 2016 edited

@tschaub I pushed 9c3cf13 with a change to render tiles to the map canvas directly. Here is the result with the bing-maps example:
nov-13-2016 09-56-03
If you're able and willing to spend some time on this, we could try to find a solution together. Maybe I'm just doing something wrong and the gaps when rendering to a transformed canvas can be avoided? I think the renderer code in this pull request is simple enough to make it easy to experiment with it.

@twpayne
Contributor
twpayne commented Nov 13, 2016

The reasons for rendering tile layers to an intermediate canvas were:

  1. Visual quality: To avoid tile seams. I don't know of any other way to avoid this. When the map is scaled fractionally or rotated, then pixel values will be interpolated from pixels in the tiles. Pixels near tile edges need to be interpolated from pixels from two tiles (at the edges) or four tiles (at the corners). If the tiles are rendered one-by-one, the data required for interpolation at tile edges and corners is simply never available.
  2. Visual quality: To simply the rendering of interim tiles (the use of a low resolution tile while waiting for high resolution tiles to load.
  3. Performance: during interactions, the intermediate canvas can reused without being redrawn (it's a single draw call at the graphics layer). This makes zooming and rotation very fast, even on low power devices (e.g. mobile), and reduces garbage generation and so avoids jank.
@marcjansen
Member

I hope to test the performance impact in one of our apps with many layers and report back on monday

@marcjansen
Member

I am sorry to say that I couldn't find time to properly test this yesterday. I am very unsure if I will have time the next days.

@ahocevar ahocevar changed the title from [WIP] Unified canvas rendering to Unified canvas rendering Nov 15, 2016
@ahocevar
Member
ahocevar commented Nov 15, 2016 edited

@tschaub c8cff76 is a commit on top of this pull request which still does everything with transforms, but with direct composition (and resulting in gaps when rendering rotated or scaled views). Everything works with that commit, so it should be easy to experiment with it.

@bartvanandel

Wouldn't relying on CSS for rotation (and scaling) cause issues with text labels that don't scale and rotate with the map? Or is this just for the tiles? Note that I'm not working on this issue, I just stumbled upon it.

@ahocevar
Member
ahocevar commented Nov 27, 2016 edited

@marcjansen Any chance you could try again and set aside some time to test this? What I want to know is whether the performance is acceptable or not. I know that the current approach is faster than what I'm proposing here. The goal of the changes here was to simplify the code base and make it more readable and maintainable.

@marcjansen
Member

Hey @ahocevar,

I have had a look at the effects of the changes in this PR. I took the wms-tiled and issued the following lines in the console:

var numLayers = 30;
var opacity = 1 / numLayers;
while(--numLayers > 0) {
  var s = new ol.source.TileWMS({
    url: 'http://ows.terrestris.de/osm/service',
    params: {LAYERS: 'OSM-WMS'}
  });
  var l = new ol.layer.Tile({
    source: s,
    opacity: opacity
  });
  map.addLayer(l);
}

then I zoomed in and out, while panning around.

This is not a real setup (30 times the same layer isn't the usual case, right?), but I saw what you have already stated: the code-execution in this branch feels less snappy compared to master. I cannot put this into something measurable, only my feelings while playing with the example. Map interaction feels slower.

I cannot qualify how much more cleaned up the code is (wrt to future changes, improvements and maintenance).

I hope this findings help at least a little bit, Andreas.

@ahocevar
Member
ahocevar commented Dec 5, 2016 edited

Thanks @marcjansen, this helped indeed. I now made a significant improvement: when a tile layer renderer has an image with tiles already, the image will now be used if too much time was spent already in a render frame. This way, image composition to the intermediate canvas no longer needs to happen in the same render frame for all layers, which drastically improves user experience and reduces CPU/GPU load.

@tschaub, you may want to give this a final test with your setup as well.

@ahocevar ahocevar added this to the v3.20.0 milestone Dec 5, 2016
@tschaub
Member
tschaub commented Dec 5, 2016

The latest changes do look like a significant improvement compared to the last commit I tested. I'm still seeing more long frames in this branch compared to master, but fewer than before. Testing with something like @marcjansen's example would make for more straightforward profiling.

Reviewing the code now.

src/ol/renderer/canvas/layer.js
*/
-ol.renderer.canvas.Layer.prototype.getPixelOnCanvas = function(pixelOnMap, imageTransformInv) {
- return ol.transform.apply(imageTransformInv, pixelOnMap.slice());
+ol.renderer.Layer.prototype.forEachLayerAtCoordinate = function(coordinate, frameState, callback, thisArg) {
@tschaub
tschaub Dec 5, 2016 Member

Should this be ol.renderer.canvas.Layer?

src/ol/renderer/canvas/tilelayer.js
- renderables.push(tile);
+ var hints = frameState.viewHints;
+ if (!(this.renderedResolution_ && Date.now() - frameState.time > 16 &&
+ (hints[ol.View.Hint.ANIMATING] || hints[ol.View.Hint.ANIMATING])) &&
@tschaub
tschaub Dec 5, 2016 Member

Do you mean hints[ol.View.Hint.ANIMATING] || hints[ol.View.Hint.INTERACTING]?

@@ -50,7 +53,7 @@ describe('ol.renderer.canvas.TileLayer', function() {
var canvas = document.createElement('canvas');
canvas.width = 200;
canvas.height = 100;
- var context = {
+ context = {
@tschaub
tschaub Dec 6, 2016 Member

Is this intentionally global?

@tschaub
Member
tschaub commented Dec 6, 2016

@ahocevar - thanks for your persistence on this. I left a few minor comments that I think are worth addressing, but otherwise I think this looks good to go.

@ahocevar
Member
ahocevar commented Dec 6, 2016

Thanks for testing @tschaub and @marcjansen, and thanks for the thorough review @tschaub.

@ahocevar ahocevar merged commit 621589b into openlayers:master Dec 6, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 86.598%
Details
@ahocevar ahocevar deleted the ahocevar:unified-canvas-rendering branch Dec 6, 2016
@Benjaki2
Contributor
Benjaki2 commented Dec 6, 2016

@ahocevar I appears that the map.forEachLayerAtPixel method is broken by the merge. At least for canvas.tilelayer renderers. The context here is dimensionless which leads me to believe it isn't being precomposed. Any Ideas?

@ahocevar
Member
ahocevar commented Dec 6, 2016 edited

@Benjaki2 Do you have a JSFiddle to help me reproduce this issue? As soon as you see the image rendered, the context will have dimensions and contain the image you see. You can also see it working in the getfeatureinfo-tile.html example.

Are you trying to call map.forEachLayerAtPixel() before anything is rendered?

@Benjaki2
Contributor
Benjaki2 commented Dec 6, 2016

@ahocevar After further testing it looks what I was seeing is related to the zoom. If you look at getfeatureinfo-tile.html after zooming around you should see that you are getting 'hits' when you shouldn't.

@ahocevar
Member
ahocevar commented Dec 6, 2016

@Benjaki2 I created #6211 for this. Feel free to create a pull request with a fix if you're willing/able to.

@Benjaki2
Contributor
Benjaki2 commented Dec 7, 2016

@ahocevar I will dig into it in the next day or two.

@ahocevar
Member

The remaining issue mentioned by @Benjaki2 has been fixed with #6222.

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