Render raster loop #3399

vicapow · 2016-10-18T09:17:27Z

Tested this using a modified "debug" page.

The issue is somewhat complex but here's a pseudo code description of the looping behavior:

map: _render()
  if _sourcesDirty?
    this._sourcesDirty = false;
    this.style._updateSources(this.transform)
      source_cache: update()
        recompute `visibleCoords` without parent id.
        source_cache: removeTile(someParent)
          triggers 'data' event with { tile: tile, dataType: 'tile' }
            maps: _update()
              maps._sourcesDirty = true;
               maps: _rerender
                 sets up reqAnimFrame listener on next render.
  painter: Painter.render()
    painter: Painter.renderLayer()
      draw_raster: drawRaster()
        draw_raster: drawRasterTile()
          source_cache: findLoadedParent()
            source_cache: addTile()
              adds the tile back to source_cache._tiles

To break out of this loop, I avoid adding the tile back inside of drawRasterTile()

mourner · 2016-10-18T17:54:03Z

Please fix the build (linting errors) for the last commit.

mourner · 2016-10-18T19:15:11Z

Still failing a test. Also, you should probably rebase the branch to fresh master (after it switched to ES6 features like let/const #3388)

vicapow · 2016-10-19T17:22:40Z

okay! I rebased from master. does this look good now?

lucaswoj · 2016-10-20T18:56:23Z

js/util/ajax.js

@@ -68,7 +77,6 @@ exports.getImage = function(url, callback) {
            context.drawImage(img, 0, 0);
            return context.getImageData(0, 0, img.width, img.height).data;
        };
-        return img;


It looks like these code changes originate in #3302. Can you remove that code from this PR and keep the two independent?

lucaswoj · 2016-10-20T18:59:03Z

js/source/source_cache.js

+                parentTile = this.findLoadedParent(coord, minCoveringZoom, parentsForFading);
+                if (parentTile) {
+                    this.addTile(parentTile.coord);
+                }


Why did you remove this.addTile from findLoadedParent and then add it to all the call sites of findLoadedParent?

findLoadedParent() is also called from within draw_raster.js. It was important not to call addTile again from there to avoid the the render loop. We could have fixed this another way but this seemed to make the most sense given the name findLoadedParent() I wouldn't have expected it to retain a tile.

Got it. This makes sense 👍

lucaswoj · 2016-10-20T18:59:40Z

test/js/source/source_cache.test.js

@@ -722,7 +722,6 @@ test('SourceCache#findLoadedParent', (t) => {
        t.equal(sourceCache.findLoadedParent(new TileCoord(2, 0, 0), 0, retain), tile);
        t.deepEqual(retain, expectedRetain);
        t.equal(sourceCache._cache.order.length, 0);
-        t.equal(sourceCache._tiles[tile.coord.id], tile);

        t.end();
    });


Can you add a test that triggers the render raster loop to ensure this behavior does not regress?

lucaswoj · 2016-10-25T17:12:16Z

This is looking good @vicapow! Just two items to hit before we can 🚢 this:

Launch Checklist

remove code originating in Fix a bug preventing map.loaded() from returning true with raster tiles. #3302
add a regression unit test

mourner · 2016-11-01T22:04:16Z

@vicapow any updates?

vicapow · 2016-11-02T00:15:51Z

@mourner I've spent a bit of time trying to write a test for this but got stuck trying to replicate the issue in the testing environment and haven't had time to dig into it more.

vicapow · 2016-11-02T02:51:42Z

Specifically, I get the following error. Maybe this is related to the tests running from Node.JS instead of the browser?

Error: Uncaught [TypeError: texImage2D(GLenum, GLint, GLenum, GLint, GLenum, GLenum, ImageData)]
    at reportException (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:58:24)
    at invokeEventListeners (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:235:9)
    at invokeInlineListeners (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:166:7)
    at HTMLImageElementImpl._dispatch (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:122:7)
    at HTMLImageElementImpl.dispatchEvent (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)
    at HTMLImageElementImpl.dispatchEvent (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:36:27)
    at e (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/resource-loader.js:51:15)
    at Object.check (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Document-impl.js:85:11)
    at /Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Document-impl.js:104:12
    at HTMLImageElementImpl._attrModified (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLImageElement-impl.js:35:37)
    at Object.exports.appendAttribute (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/attributes.js:204:11)
    at HTMLImageElementImpl.setAttribute (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Element-impl.js:329:18)
    at HTMLImageElementImpl.set src [as src] (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLImageElement-impl.js:51:10)
    at HTMLImageElement.set (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/generated/HTMLImageElement.js:40:20)
    at exports.getArrayBuffer (/Users/victorpowell/Documents/projects/mapbox-gl-js/js/util/ajax.js:70:21)
    at FakeXMLHttpRequest.xhr.onload (/Users/victorpowell/Documents/projects/mapbox-gl-js/js/util/ajax.js:41:13) TypeError: texImage2D(GLenum, GLint, GLenum, GLint, GLenum, GLenum, ImageData)
    at WebGLRenderingContext.texImage2D (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/gl/webgl.js:3499:13)
    at RasterTileSource.done (/Users/victorpowell/Documents/projects/mapbox-gl-js/js/source/raster_tile_source.js:80:20)
    at HTMLImageElement.img.onload (/Users/victorpowell/Documents/projects/mapbox-gl-js/js/util/ajax.js:63:13)
    at HTMLImageElement.callback.(anonymous function) (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:289:32)
    at invokeEventListeners (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:219:27)
    at invokeInlineListeners (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:166:7)
    at HTMLImageElementImpl._dispatch (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:122:7)
    at HTMLImageElementImpl.dispatchEvent (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)
    at HTMLImageElementImpl.dispatchEvent (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:36:27)
    at e (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/browser/resource-loader.js:51:15)
    at Object.check (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Document-impl.js:85:11)
    at /Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Document-impl.js:104:12
    at HTMLImageElementImpl._attrModified (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLImageElement-impl.js:35:37)
    at Object.exports.appendAttribute (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/attributes.js:204:11)
    at HTMLImageElementImpl.setAttribute (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/Element-impl.js:329:18)
    at HTMLImageElementImpl.set src [as src] (/Users/victorpowell/Documents/projects/mapbox-gl-js/node_modules/jsdom/lib/jsdom/living/nodes/HTMLImageElement-impl.js:51:10)

If it helps, this is what I have so far.

    t.test('#loaded() stabilizes with raster tile zooming', t => {
        window.server.respondWith(request => {
            request.respond(204, {'Content-Type': 'image/png'}, '');
        });
        window.server.autoRespond = true;
        window.server.autoRespondAfter = 10;
        const style = createStyle();
        const sourceName = 'example-raster-source';
        style.sources[sourceName] = {
            type: 'raster',
            tileSize: 256,
            scheme: 'xyz',
            tilejson: '2.0.0',
            tiles: ['http://example.com/{z}/{x}/{y}.png']
        };
        style.layers.push({
            id: `${sourceName}-layer`,
            type: 'raster',
            source: sourceName,
            'source-layer': 'raster_full',
            paint: {
                'raster-opacity': 1
            }
        });

        console.log('create map');
        const map = createMap({style, zoom: 15, center: [-77.0186, 38.8888]});
        map.on('load', () => {
          console.log('map loaded');
          console.log(map.loaded());
          map.jumpTo({zoom: 14});
          setTimeout(() => {
            map.jumpTo({zoom: 16});
            setTimeout(() => {
              let renderCount = 0;
              map.on('render', () => {
                // should only be called once.
                t.end();
                // for debugging the tests...
                renderCount++;
                console.log(renderCount);
              });
            }, 10);
          }, 10);
        });
    });

vicapow · 2016-11-28T10:34:21Z

Any recommendation on the issues I'm running into with writing tests for this fix?

lewis500 · 2016-12-03T22:04:50Z

has this issue been resolved? I noticed that in release 0.27 #3302 had been resolved. does that fix this problem? My CPU is still blowing up when I zoom around on raster tiles.

nicooprat · 2016-12-05T11:09:51Z

Until it's resolved, is there any quick workaround to cancel a render? Something like:

let dirty = false;

map.on('movestart', function(e) {
  dirty = true;
});

map.on('moveend', function(e) {
  dirty = false;
});

map.on('render', function(e) {
  if(!dirty) e.target.cancelRender();
});

Thanks.

andrewharvey · 2016-12-22T23:55:38Z

What's the status of this/what's left to do to?

I've manually tested this change before/after and it fixes the loop/high CPU on raster sources. npm run test-unit is all passing.

lucaswoj · 2016-12-23T14:11:30Z

@andrewharvey We're trying to get a regression test in place for this bug before merging this PR. @mollymerp is working on that today.

mollymerp

I spent a day trying to figure out how to fake the rendering loop but sinon and jsdom have limitations such that writing a test for this is not straightforward. in the interest of getting this fix 🚢 for users who are blocked by this, lets merge and I'll open a ticket to investigate alternative testing methods and/or refactoring of SourceCache entirely.

per chat -- shipping now for expediency. will re-investigate testing at a later date

mollymerp · 2016-12-23T19:22:55Z

Thanks again for tracking this down @vicapow !

vicapow force-pushed the render-raster-loop branch from e9dc713 to 78e6c51 Compare October 18, 2016 20:23

lucaswoj previously requested changes Oct 20, 2016

View reviewed changes

lucaswoj added the not ready for review label Oct 31, 2016

mourner mentioned this pull request Nov 15, 2016

constant high CPU from raster sources #3620

Closed

kriscarle mentioned this pull request Nov 15, 2016

Zooming in raster tile layers cause triggers a continues render loop #3398

Closed

vicapow added 3 commits December 22, 2016 12:04

Prevent rendering from looping after zooming into raster tiles.

91b8df3

Fix linting errors in source_cache.

b40c45c

Fix source_cache tests

edee34a

mollymerp force-pushed the render-raster-loop branch from 78e6c51 to edee34a Compare December 22, 2016 17:19

mollymerp approved these changes Dec 23, 2016

View reviewed changes

mollymerp merged commit 61e7191 into mapbox:master Dec 23, 2016

mollymerp mentioned this pull request Dec 23, 2016

Design tests to prevent infinite rendering loops #3859

Closed

averas mentioned this pull request Dec 29, 2016

map.loaded() seems broken for raster layers #3872

Closed

indus mentioned this pull request Jan 9, 2017

features of parent tiles showing up while rotating #3810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render raster loop #3399

Render raster loop #3399

vicapow commented Oct 18, 2016 •

edited

Loading

mourner commented Oct 18, 2016

mourner commented Oct 18, 2016

vicapow commented Oct 19, 2016

lucaswoj Oct 20, 2016

lucaswoj Oct 20, 2016

vicapow Oct 25, 2016

lucaswoj Oct 25, 2016

lucaswoj Oct 20, 2016

vicapow Oct 25, 2016

lucaswoj commented Oct 25, 2016

mourner commented Nov 1, 2016

vicapow commented Nov 2, 2016

vicapow commented Nov 2, 2016

vicapow commented Nov 28, 2016 •

edited

Loading

lewis500 commented Dec 3, 2016

nicooprat commented Dec 5, 2016 •

edited

Loading

andrewharvey commented Dec 22, 2016

lucaswoj commented Dec 23, 2016

mollymerp left a comment

mollymerp commented Dec 23, 2016

Render raster loop #3399

Render raster loop #3399

Conversation

vicapow commented Oct 18, 2016 • edited Loading

mourner commented Oct 18, 2016

mourner commented Oct 18, 2016

vicapow commented Oct 19, 2016

lucaswoj Oct 20, 2016

Choose a reason for hiding this comment

lucaswoj Oct 20, 2016

Choose a reason for hiding this comment

vicapow Oct 25, 2016

Choose a reason for hiding this comment

lucaswoj Oct 25, 2016

Choose a reason for hiding this comment

lucaswoj Oct 20, 2016

Choose a reason for hiding this comment

vicapow Oct 25, 2016

Choose a reason for hiding this comment

lucaswoj commented Oct 25, 2016

Launch Checklist

mourner commented Nov 1, 2016

vicapow commented Nov 2, 2016

vicapow commented Nov 2, 2016

vicapow commented Nov 28, 2016 • edited Loading

lewis500 commented Dec 3, 2016

nicooprat commented Dec 5, 2016 • edited Loading

andrewharvey commented Dec 22, 2016

lucaswoj commented Dec 23, 2016

mollymerp left a comment

Choose a reason for hiding this comment

mollymerp commented Dec 23, 2016

vicapow commented Oct 18, 2016 •

edited

Loading

vicapow commented Nov 28, 2016 •

edited

Loading

nicooprat commented Dec 5, 2016 •

edited

Loading