Look into bringing back SurfaceView

[tobrun]

Currently on Android we are using a TextureView in favor of SurfaceView. @brunoabinader and @tmpsantos mentioned to look into moving it back to SurfaceView for performance reasons (Qt is using similar concept).

History behind the initial move from SurfaceView -> TextureView in #2244.

[tobrun]

@tmpsantos @brunoabinader looking through some SDK code, I'm also seeing the class GLSurfaceView which subclasses the mentioned SurfaceView mentioned above.
Is this the class you indicated in chat?

[tmpsantos]

Yes, that is the one we should give a try IMO. Seems like we could remove a lot of code we use today for creating/destroying display/surface/context/etc if we use that and get rendering on a separated thread for free.

The challenge I see is that all the UI events come from - obviously - the UI thread. We need to forward them to the Renderer object thread where the Map object will now live. That can be done IMO.

[adogor]

Hi,
We are experiencing Mapbox and started with the last beta (v4.1.0-beta.2) as stated in the documentation. Unfortunately performances are not as good as the iOS version. As said in #4488 there are jank / stuttering when zooming or scrolling, it feel unusable.

I've downgraded to v4.0.1 which is better for performances, but not as good as Google Maps for example. I haven't seen github issues with the beta but I assume you are working on it ?

Finally, I've decided to apply the patch (f290bf4) from @tobrun but on the top of the v4.0.1. Performances are really amazing ! I can forward a test apk if needed.

SurfaceView seems to be be the way to go for performances.

Thanks !

[tobrun]

I have been looking into implementing the GLSurfaceView approach in the last few weeks. While I was able to get parts of the code running (setup glContext and display).. I was' t able to render or send notifications from core (mapchange events) back to the android binding. This as indicated by @tmpsantos will require some extra plumbing. I will keep on looking into this issue in next weeks and see if we can migrate.

As mentioned by @adogor, there is already a branch with a working SurfaceView. We still prefer moving to the GLSurfaceView since it will automise the process of setting up GL (context,display) and move drawing to a seperate Renderer object.

[justasm]

Since a migration to GLSurfaceView requires significant work it may make sense to treat it as a separate longer-term goal.

If it is possible to migrate to a standard SurfaceView without changing the existing architecture and without major regressions, it would be a worthwhile change in and of itself from a performance point of view. It would also more quickly identify any regressions due to a move away from TextureView, giving contributors time to address them before fully committing to a GLSurfaceView-based architecture.

[tobrun]

@justasm that is a good idea. I'm currently looking into suggested approach and I identified 2 issues:

• MarkerViews don't show up anymore
• Snapshot code for TextureView is incompatible with SurfaceView

[tobrun]

I was able to resolve the MarkerView issue, but as initially thought the synchronisation between View objects and the SurfaceView is not that great. Here you can see the Marker View jiggling.

While with TextureView the synchronisation is better, but overal performance is worse:

[tobrun]

SurfaceView has been merged to master with 99a4850.
Next up is working on a GLSurfaceView implementation.

[tobrun]

I have been working on the GLSurfaceView the latest 2 days and I identified the following issue:
Rendering with GLSurfaceView occurs on a separate rendering thread and is currently resulting in broken mapChange events. When we tried to communicate from the render thread back to jni/java/ui-thread, it crashes. One solution for this would be to use Runnables (but this wouldn't be ideal for high trafficking code as map change events). Another solution would be to introduce an map state object where we update the fields when a change occurs.

After disabling the map change events in total. I'm running into not being able to properly load a style (401). Loading up a style locally works but after a couple of render calls it also crashes. Symbolicating the stacktraces shows:

Tobruns-Mac-mini:mapbox-gl-native nurbot$ adb logcat | ndk-stack -sym build/android-arm-v7/Release/lib.target
******* Crash dump: **********
Build fingerprint: 'lge/awifi_open_eu/awifi:4.2.2/JDQ39B/V50010a.1380629540:user/release-keys'
pid: 17944, tid: 18065, name: Thread-4235 >>> com.mapbox.mapboxsdk.testapp <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 00000048
Stack frame #00 pc 00155460 /data/app-lib/com.mapbox.mapboxsdk.testapp-1/libmapbox-gl.so: Routine std::__ndk1::__tree_node_base<void>* std::__ndk1::__tree_minstd::__ndk1::__tree_node_base<void_>(std::__ndk1::_tree_node_base<void>) at /Users/nurbot/mapbox-gl-native/mason_packages/osx-x86_64/android-ndk/arm-9-r12b/bin/../lib/gcc/arm-linux-androideabi/4.9.x/../../../../include/c++/4.9.x/__tree:134 (discriminator 1)
Stack frame #1 pc 000eaf1c /data/app-lib/com.mapbox.mapboxsdk.testapp-1/libmapbox-gl.so: Routine mbgl::Painter::render(mbgl::style::Style const&, mbgl::FrameData const&, mbgl::SpriteAtlas&) at /Users/nurbot/mapbox-gl-native/build/android-arm-v7/../../src/mbgl/renderer/painter.cpp:193

[bleege]

Per internal discussion we're going to revert the SurfaceView work #5000 (comment) as issues with Navigation Drawers not rendering correctly have again resurfaced on an internal app (same issue as original attempt to go to SurfaceView and revert to TextureView in the Mapbox Android SDK #2244 (comment)). While we'd like to use SurfaceView and GLSurfaceView in the SDK this is too big of bug for developers to be able to use Mapbox Android SDK at this time. We can keep working on this for GLSurfaceView in development branches but before we can feel comfortable merging this into master we'll need a functioning Navigation Drawer example in the TestApp.

The good news though is that this issue appears to be resolved in the upcoming Android Nougat release (see quote from Google below). The bad news is it'll be awhile before the Nougat is 1) actually released officially by Google and 2) the market share of Nougat on people's devices is actually significant.

Note: Starting in platform version N, SurfaceView's window position is updated synchronously with other View rendering. This means that translating and scaling a SurfaceView on screen will not cause rendering artifacts. Such artifacts may occur on previous versions of the platform when its window is positioned asynchronously.

/cc @zugaldia @ivovandongen @tobrun @cammace

[bleege]

The team has spent some time since Wednesday (July 20th) looking into ways to solve the Navigation Drawer issue (see #5738) and communicating about other issues that we've found with SurfaceView since it's been in master. While having SurfaceView in master has been extremely useful in helping us find these bugs, master is also where the team generates releases from and these bugs are too big to be able to ship product to customers with now without negatively impacting the UX / UI of their apps. To be clear we all REALLY want to get SurfaceView into Mapbox Android SDK and are actively working on doing so.

The Way Forward

Work on SurfaceView and it's bugs will be moved back to a development branch for time being until the 3 big blocking bugs can be addressed. When they are addressed we will bring the SurfaceView code back into master so that the wider development team and community can test with it. In other words: we'll repeat this process. If things go well, which I'm expecting them to, we can then leave SurfaceView in master and ship it. 🚀

Known Blocking Issues

• Navigation Drawer SurfaceView rendering with NavigationDrawer and statusbar #5738
• Location Picker Location Picker Native Marker No Longer Shows #5717
• SNAPSHOT API Fix screenshot/snapshot api for SurfaceView #5587

[zugaldia]

For improved performance, we're shipping SurfaceView since 4.2.0-beta.2 #6155. We'll make this functionality available in the next final 4.2.0 release too.