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

[core] Ensure correct GL context in Map::onLowMemory() #6972

Merged
merged 1 commit into from
Nov 9, 2016
Merged

Conversation

jfirebaugh
Copy link
Contributor

Fixes #5731

@mention-bot
Copy link

@jfirebaugh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kkaefer, @brunoabinader and @1ec5 to be potential reviewers.

@jfirebaugh
Copy link
Contributor Author

This probably fixes #1509 as well.

@mb12
Copy link

mb12 commented Nov 9, 2016

It most likely doesn't fix #1509. It's a single map with memory under 25 MB the last time I tried. The geometries are literally quads that resselate into two triangles per quad. It doesn't use any symbols etc. The vector tiles are trivial quads literally.

Copy link
Member

@brunoabinader brunoabinader left a comment

Choose a reason for hiding this comment

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

Great finding, @jfirebaugh - though for future reference, it might be interesting for the Painter object to receive a reference for the whole Backend on its ctor instead of just Context, because Context can't currently activate itself. We could then have a RAII-based guard on every painter call that handles OpenGL calls, like cleanup() and reset() for instance.

@ivovandongen ivovandongen mentioned this pull request Nov 9, 2016
@kkaefer
Copy link
Contributor

kkaefer commented Nov 9, 2016

FWIW, I tried to trigger this particular code path when I debugged the original issue, but couldn't get it to crash even in severely RAM constrained environments, however, I only used one map so there probably was no GL context conflict.

While we're at it: we should also move the call to style->onLowMemory() before the painter cleanup; it may release resources that are not cleaned up. We seem to be triggering a rerender, but I'm not sure this is strictly necessary. An alternative could be to not call painter->cleanup(), and exclusively rely on the cleanup call that happens as part of the repaint anyway. However, this could potentially worsen the memory situation since a redraw might potentially allocate even more memory.

@jfirebaugh
Copy link
Contributor Author

@brunoabinader @kkaefer -- good ideas. Can you work on followup PRs for those?

@jfirebaugh jfirebaugh merged commit 7ea0457 into master Nov 9, 2016
@jfirebaugh jfirebaugh deleted the 5731 branch November 9, 2016 17:57
boundsj added a commit that referenced this pull request Nov 9, 2016
This ports changes in
#6972 to this version
of the map.
@1ec5 1ec5 mentioned this pull request Nov 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants