[qt] Performance degradation with v1.2.0

[rinigus]

Hi!

I have updated from 1.1.0 to 1.2.0 and observed significant slowdown in drawing additional layers on top of the map:

• when drawing point source with the label showing its {name} (tested on Linux PC)

• when drawing point source with assigned image (tested on Sailfish OS phone)

Here, the point source is defined as GeoJSON object, has name property and point coordinates. The program is updating the name and coordinate every specified dt to make the point move and its name change accordingly together with the movement.

Used setup: Qt bindings as released via qt-staging, version v1.2.0; qmapboxgl exposed via QML plugin https://github.com/rinigus/mapbox-gl-qml.

Label:

It looks like every time when I change the label of the point source by updating it, the old text disappears first and then is drawn. The process can take ~0.5s on desktop. Its all looking nice for slow updates with the labels around removed in the case of collision, but such speed is not sufficient when the label is updated more frequently. For example, if I update the label every 50ms, I don't see it at all. In v1.1.0, I did not observe any on/off flickering when changing the label. As a result, the text was floating (or jumping depending on dt) without disappearing.

Image:

When point source is shown using an image (symbol with image source), every time I update the coordinates, it seems to remove the image and add it later leading to blinking of the image. This has been observed in full-blown map application and maybe there is something else going on than just update of coordinates. I may have to check on simpler example. However, such behavior was not observed with v1.1.0

Specific conversions and QMapboxGL calls:

Corresponding point definition (https://github.com/rinigus/mapbox-gl-qml/blob/master/app/qml/main.qml#L191) and update (https://github.com/rinigus/mapbox-gl-qml/blob/master/app/qml/main.qml#L395) are implemented in QML. The update is translated to C++ via map->updateSource at https://github.com/rinigus/mapbox-gl-qml/blob/master/src/qmapboxsync.cpp#L36 after conversion at https://github.com/rinigus/mapbox-gl-qml/blob/master/src/qquickitemmapboxgl.cpp#L533.

Not sure whether these issues have been reported already.

[brunoabinader]

Hi @rinigus - thanks for the feedback. Is this still the case using the latest version of master branch? Also, could you please provide a sample of the style you're using when performing the benchmark tests?

[rinigus]

Hi @brunoabinader, I will look into it a bit later. I plan to check against master branch, make working example and then we can look together on whether its fixed in the latest version. There are couple other issues that I have to fix first in my bindings for QML, but then I'll work on it. As for timeline, its probably 1-2 weeks I may need before working on this performance problem.

[rinigus]

Started looking into it. Current master has the same issue. To demonstrate the problem, I patched demo app to add moving source (Press 4 to activate) that has changing label. As it is visible in example, every update leads to blinking of the label (label is taken down and later redrawn slowly). By changing the timer period (10 ms, for example), one can get to the state where label is mostly invisible.

This patch (not clean, no wish to merge) is available https://github.com/rinigus/mapbox-gl-native/tree/issue_10644_blinky at with the corresponding commit rinigus@247a5a4

I'll try to find where this issue started by checking older commits.

[rinigus]

I have been bisecting to find the commit. Turned out to be rather non-trivial since several commits didn't compile, unfortunately. Bisecting log with the last check done manually (couldn't fix the compilation issue in few minutes that I had):

git bisect start
# bad: [4fc070dcfd84324d2596a748f3bb815a2e4a7b5f] [ios, macos] Converted strings files to UTF-8
git bisect bad 4fc070dcfd84324d2596a748f3bb815a2e4a7b5f
# good: [1cf8ef0437bdd71da4dd18034a6030f5af20bd29] [qt] use const for setting ResourceTransform
git bisect good 1cf8ef0437bdd71da4dd18034a6030f5af20bd29
# bad: [730054276ed6a5dc7ba9d19765b8457a818854f7] [core] C++ port of TinySDF
git bisect bad 730054276ed6a5dc7ba9d19765b8457a818854f7
# good: [91dabd01dfcd52dc40aa1c1d1db9d3f48f7abc97] [ios] Silence smart invert warnings (#10425)
git bisect good 91dabd01dfcd52dc40aa1c1d1db9d3f48f7abc97
# good: [225fba8a0aad5ea52ff878443f58d65bb2221d74] Merge branch 'release-agua' into tvn-merge-release
git bisect good 225fba8a0aad5ea52ff878443f58d65bb2221d74
# bad: [934fa4933cf694cd835780b898b0ef0e51a74020] [android] Add Grid source sample for Custom Geometry Source
git bisect bad 934fa4933cf694cd835780b898b0ef0e51a74020
# bad: [3e074390b5afb0bf6d50b83c5327786bc011cd95] [core] Added ignores for two query tests that return the same set of items but in a different order.
git bisect bad 3e074390b5afb0bf6d50b83c5327786bc011cd95

# manually checked
git bisect good dcd7da14f6627a09a5840a3046b5708449962f69 [docs] Fix link in node readme (#10395)

This leaves the following commits responsible for blinky symbols on update (git bisect visualize):

commit 3e074390b5afb0bf6d50b83c5327786bc011cd95
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 14:36:30 2017 -0800

    [core] Added ignores for two query tests that return the same set of items but in a different order.

commit 9fb30dfd52204bfbf02fae49d37e74312d316f40
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 14:36:30 2017 -0800

    [core] Removed ignores for fixed issues.

commit 54b990eb33e87b44127e2f9baf35fd38588fffc1
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Update unit tests for viewport collision.
    Add 'GridIndex' unit test.

commit ff58848cfdb071886a323b246144089ecc7c657d
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Remove dead code
     - CollisionTile
     - FrameHistory
     - PlacementConfig

commit 4137f559f08b50ac6d8d5ac9ff0489faec470cdf
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Use floats instead of ints for Shaping.
    Brings gl-native shaping closer to gl-js.

commit 30de793c2ab2f864af3e524be24f917f6897634a
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Update shaders/tests from GL JS.

commit 77bfb47fdabbd257272a01bef591d73e8c53c9c7
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Update queryRenderedFeatures to use global CollisionIndex.

commit c0cb210ddca1901a956cd68e9142b7fb04183248
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Switch from background to foreground placement
     - Background placement code now just generates static symbol buffers
     - Don't render GeometryTiles until their symbols are loaded. This is necessary for the CrossTileSymbolIndex to successfully prevent flicker.
     - SymbolInstances are transferred to SymbolBucket for use on foreground during collision detection
     - Symbols are sorted on foreground by sorting their index buffer but leaving vertex buffers intact (only works within one segment)
     - Vertical glyphs are generated at same time as horizontal glyphs. `reprojectLineLabels` chooses which one to use at render time and hides the other.
     - Icons are now always represented with a single collision box, even if they're placed along a line (this means their rotation alignment may be wrong, but the approach of representing them with multiple collision boxes wasn't very accurate either).
     - Generate vertices for new debug collision boxes and collision circles
     - Only add symbols within tile boundaries (reduces work, avoids double-draw)
     - Update symbol_projection.cpp to support line label projection calls from CollisionIndex.

commit 9a5b2fdfc362e7041a10d5066161b51aedbb0a31
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Enable dynamic updates of index buffers.

commit 4c6f0e8138338f0d4a8d2298a053513ad30be5ea
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Add global CollisionIndex to replace CollisionTile.
     - Switches from tile to viewport coordinates
     - Represents line labels with circle geometries
     - Projects line labels at collision detection time to improve accuracy
     - Adapts tile-based symbol queries to viewport coordinates

commit 18d735ccc20e1e7d4616b3d3b5b1a527a01d1b91
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Add Placement class.
    Responsible for running global collision detection/symbol placement algorithm and updating symbol opacity buffers accordingly.

commit 601a1c8dcf73f1a419c1ed5c39fbd14c581abf50
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:43 2017 -0800

    [core] Add circle geometries to GridIndex.
     - Adds early exiting "hitTest" query for fast collision detection
     - GridIndex now determines cell count separately for x and y axes based on grid dimensions.

commit 31f0f47195ae7d4a6f96b1c64cd39e1268fdfc8d
Author: Chris Loer <chris.loer@gmail.com>
Date:   Thu Nov 9 13:24:25 2017 -0800

    [core] Add CrossTileSymbolIndex.
    This class is responsible for tracking which symbols are "the same" between tiles at different zoom levels, so that symbol opacities (fade animations) can be copied smoothly between tiles.

Looks like the commits are coming from #10436

And the search was interrupted by failing to compile c0cb210

[rinigus]

@brunoabinader and @jfirebaugh: please let me know if additional information is needed. As it is, I have found the PR which is responsible for blinking symbols on update of the source (#10436 by @ChrisLoer), but couldn't go down to a single commit.

This issue is a blocker for us since we are getting blinking on current position on every update. So, navigation applications have "here" symbol blinking once per second or so disturbing the users. As a result, I am holding off updates to v1.2.0 of qt stack and we are still using v1.1.0

[ChrisLoer]

Hi @rinigus this sounds like issue #10559 which we should have fixed in PR #10899, except that PR went into master almost two weeks ago and it looks like you're already based on top of that.

I'll try to build your demo app to see if I can reproduce and get a better understanding of what's going on.

/cc @ansis

[rinigus]

@ChrisLoer , I also had hopes for that fix, but it didn't really work for me. I do wonder whether this blinking is visible on Qt only.

Thank you very much for looking into it.

[ChrisLoer]

@rinigus I can reproduce locally using your branch. I suspect it's something specific to how we use gl-native in Qt, but I'm not very familiar with our Qt build so I'm still getting oriented.

[ChrisLoer]

Hi @rinigus I'm sorry I wasn't thinking clearly -- this is actually expected behavior now that we do collision detection between multiple sources (one of the main features introduced with #10436). When you update the position of the feature, it won't start showing until we've run collision detection and determined that there's space for it (at which point it starts fading in). However, we can display the symbol immediately without any fading if we know ahead of time it won't collide with anything. In your demo, you can add:

m_map->setLayoutProperty("location-label", "text-allow-overlap", true);

Notice in the example this will still cause the moving label to knock out the labels below it (you can control this separately with text-ignore-placement).

Hopefully this will restore the functionality you need, but it will still be a little different from the previous behavior: before, if two symbols within the same source collided, you could knock one out without interrupting the animation. Now, to do a smooth animation of a symbol with updating positions, you need to allow it to overlap with every other symbol. We're looking into ways to add more granularity to this behavior in the future.

[rinigus]

Hi @ChrisLoer , great idea, I'll test it tonight. I did try to use text-ignore-placement before submitting the issue, but that was before #10899 and forgot to test it this time. I'll report back with the results.

[rinigus]

@ChrisLoer, just tested against master and using text-allow-overlap set to true. All works nicely - the location label is not removed and always visible. Same was when using icon with icon-allow-overlap set to true - no more blinking of the icon on update.

Thank you very much, I am closing the issue and it can be added as one more resolved issue to the list of #10899