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

Crash when deleting buffers #1016

Merged
merged 1 commit into from
Mar 20, 2015
Merged

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Mar 18, 2015

I'm seeing a fairly frequent crash bug that happens randomly during buffer deletion. The buffer IDs are valid and do exist.

What could be happening is that we have an OpenGL context loss that we're ignoring, which invalidates all of the buffers.

buffer hpp 2015-03-17 18-10-54

@kkaefer kkaefer added bug iOS Mapbox Maps SDK for iOS crash labels Mar 17, 2015
@kkaefer kkaefer added this to the iOS Beta 1 milestone Mar 17, 2015
@robipresotto
Copy link

screen shot 2015-03-17 at 18 30 04

buffer.hpp line 24
mbgl::VectorTileData::~VectorTileData()

@ljbade
Copy link
Contributor

ljbade commented Mar 18, 2015

Hmm the major problem is the GL implementation should never crash. It is supposed to check for bad state and return a GL error instead.

@kkaefer kkaefer mentioned this pull request Mar 18, 2015
@kkaefer
Copy link
Contributor Author

kkaefer commented Mar 18, 2015

After some debugging, it looks like the following is happening:

  • The GLKView has its (internal) display method run, which triggers the (internal) setFramebuffer method. This likely activates the context on the main thread and runs OpenGL functions for framebuffer creating/attaching.
  • Typically this is fine since we're not running OpenGL code unless the main thread tells us to, except:
  • At the same time, the Map thread runs a shared_ptr destructor of a DebugBucket object, which in turn deletes the VAO and associated OpenGL objects

vao cpp 2015-03-18 12-06-13

Here's my proposed change: Instead of directly deleting all OpenGL objects, we should accumulate the IDs and delete them during the next render step.

@mb12
Copy link

mb12 commented Mar 18, 2015

Why does the stack trace have ~LiveTileData in the back trace. Is there some code path that triggers invalidateTiles outside mapThread?

@kkaefer
Copy link
Contributor Author

kkaefer commented Mar 18, 2015

~LiveTileData is run in the map thread

@incanus
Copy link
Contributor

incanus commented Mar 18, 2015

Cross-ref to #1022 — I will double-check that things are kosher here.

On iOS, the main thread does framebuffer operations just before it requests a rerender of the view contents. This means that GL operations on the Map thread will interfere with that and cause a crash. While most GL operations happen throughout rendering (which is synchronized with the main thread), deletion of shared_ptrs release in turn their OpenGL objects. This may happen at any time and isn't synced with rendering.

This patch changes this so instead of deleting all objects immediately, we are collecting all abandoned IDs and delete them in bulk once the main thread has given us permission to use OpenGL calls (i.e. during render()).
@kkaefer kkaefer force-pushed the 1016-coalesce-opengl-object-deletion branch from 5830a57 to 61fa436 Compare March 20, 2015 18:47
kkaefer added a commit that referenced this pull request Mar 20, 2015
@kkaefer kkaefer merged commit 84d4728 into master Mar 20, 2015
@kkaefer kkaefer deleted the 1016-coalesce-opengl-object-deletion branch March 20, 2015 21:06
@picciano
Copy link

+1 Awesome, no more crashing. Thanks!

@@ -113,7 +127,6 @@ void Map::start(bool startPaused) {

// Remove all of these to make sure they are destructed in the correct thread.
style.reset();
workers.reset();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This crashes in Debug mode since the Worker threads are now joined from the wrong thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9d30d40

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug crash iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants