MapView: eglSwapBuffers Error

[ivovandongen]

When chaining some operations on the MapView the map, applications crash due to an error while rendering. Specifically:

07-16 11:25:32.255 7133-7133/com.mapbox.mapboxsdk.testapp E/libEGL: call to OpenGL ES API with no current context (logged once per thread)
07-16 11:25:32.258 7133-7133/com.mapbox.mapboxsdk.testapp D/mbgl: {unknown}[Android]: NativeMapView::notifyMapChange()
07-16 11:25:32.259 7133-7133/com.mapbox.mapboxsdk.testapp E/mbgl: {unknown}[OpenGL]: eglSwapBuffers() returned error 12301
07-16 11:25:32.259 7133-7133/com.mapbox.mapboxsdk.testapp D/AndroidRuntime: Shutting down VM
07-16 11:25:32.260 7133-7133/com.mapbox.mapboxsdk.testapp E/AndroidRuntime: FATAL EXCEPTION: main
                                                                            Process: com.mapbox.mapboxsdk.testapp, PID: 7133
                                                                            java.lang.Error: eglSwapBuffers() failed
                                                                                at com.mapbox.mapboxsdk.maps.NativeMapView.nativeRender(Native Method)
                                                                                at com.mapbox.mapboxsdk.maps.NativeMapView.render(NativeMapView.java:131)
                                                                                at com.mapbox.mapboxsdk.maps.MapView.onDraw(MapView.java:1302)
                                                                                at android.view.View.draw(View.java:16184)
                                                                                at android.view.View.updateDisplayListIfDirty(View.java:15180)

As @jfirebaugh suggests, it might be that the the MapView doesn't reactivate the GL Context properly. We need to check that the the context is deactivated and re-activated properly. Atm, it's easily re-produced by removing a layer within a camera animation finish callback.

It might be a good idea to isolate from issues like this by placing callbacks on the main looper. This ensures that the callbacks always happen on the main thread as well. A similar solution is also used on the QT bindings I believe (cc @tmpsantos).

cc @tobrun @zugaldia @bleege

[ivovandongen]

@jfirebaugh I'm reading up on the core classes and came accross Map::removeLayer. It uses an AsyncTask to execute the removal. Assuming this is scheduled on the main thread (using the ALooper implementation), is the removal actually guaranteed to be completed when deactivate is called?

void Map::removeLayer(const std::string& id) {
    impl->view.activate();

    impl->style->removeLayer(id);
    impl->updateFlags |= Update::Classes;
    impl->asyncUpdate.send();

    impl->view.deactivate();
}

cc @tmpsantos

[jfirebaugh]

@ivovandongen Are you referring to the impl->asyncUpdate.send() line? That part of the code is responsible for triggering updates of internal state used for rendering and invalidation of the view. But all work that requires an active context during removal is accomplished by impl->style->removeLayer(id).

[ivovandongen]

@jfirebaugh Yes. Thanks for the clarification.

[ivovandongen]

Dove a little bit further in this. The issues seem to be caused by a combination of factors. In short; the View::notifyMapChange (which translates in the onFinish callback from camera update animations) is called synchronously when a GL context is already active. Calling removeLayer in the callback tries to open the context again -> crash.

Assuming the callback in core is meant to happen synchronously there are two approaches we can take here:

• Make View::activate and View::deactivate support "nested transactions", ie check if the context is already open and only close the context on the last deactivate.
• Wrap all map callbacks in a main looper post on the Android side.

I prefer the second approach as it nicely isolates the callbacks, has a minimal impact and also ensures callbacks are always on the main thread. A similar approach is already in place on other platforms, at least QT.

[ivovandongen]

Something like:

    protected void onMapChanged(final int mapChange) {
        if (mOnMapChangedListener != null) {
            for (final OnMapChangedListener listener : mOnMapChangedListener) {
                mainHandler.post(new Runnable() {
                    @Override
                    public void run() {
                        listener.onMapChanged(mapChange);
                    }
                });

            }
        }
    }

[zugaldia]

Wrap all map callbacks in a main looper post on the Android side.

That's also similar to what we do with offline callbacks, e.g.:

@Override
public void onList(final OfflineRegion[] offlineRegions) {
    getHandler().post(new Runnable() {
        @Override
        public void run() {
            callback.onList(offlineRegions);
        }
    });
}

[jfirebaugh]

Great diagnosis @ivovandongen -- I didn't think about nesting, but that's definitely the problem.

The danger in reposting change events on the event loop is that someone might be relying on the current synchronous behavior, for example to update UI elements in sync with the map. We've definitely seen cases where, when similar events were not synchronous enough, they caused desynchronization between the map and other UI elements -- #1813, #5489.

My preference would be to implement the "nested transaction" behavior for View::activate/deactivate like you suggest.

[ivovandongen]

Thanks for confirming @jfirebaugh. We actually have 2 options then; Depending on wether multiple platforms need this, we can either a) implement it in core as the default behaviour or b) in the android MapView implementation itself. I'd prefer the first and implement it in map.cpp directly. However, I don't have a complete overview of wether this could impact other platforms negatively in any way.

Either way, I wouldn't mind taking a stab at it.

cc @tmpsantos @zugaldia @frederoni

[bleege]

/sub