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

MapView reuse, integration test suite #14127

Merged
merged 5 commits into from
Mar 22, 2019
Merged

MapView reuse, integration test suite #14127

merged 5 commits into from
Mar 22, 2019

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Mar 15, 2019

Follow up from #13926 and #13976. Since 7.2.0, our map freezes when used in a View reuse scenario. Since this went unnoticed, I'm am adding a new integration test suite using UiAutomator.

MapView freezes in RecyclerView - 4d98991

  • GLSurfaceView, calling OnSurfaceCreated in a similar fashion to OnSurfaceDestroyed.
  • TextureView, not destroying the map renderer as part of surface destruction, surface can be recreated

Integration test suite - 676d7dd

This PR introduces an integration test-suite that will harden against regressions. I added a handful of others occurrences that had regressed in the past.

image

cc @mapbox/maps-android

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Mar 15, 2019
@tobrun tobrun added this to the release-liquid milestone Mar 15, 2019
@tobrun tobrun self-assigned this Mar 15, 2019
@tobrun tobrun changed the title Surface reuse, integration test suite MapView reuse, integration test suite Mar 19, 2019
@tobrun
Copy link
Member Author

tobrun commented Mar 19, 2019

@ivovandongen
Both master as before #13926, we do not allow reuse of a MapView using a GLSurfaceView.

Below a gif showing the crash:

The crash occurs when you scroll the MapView back into view. When that happens a nativeOnSurfaceCreated is triggered from the render thread and execution fails on renderer.reset() in map_renderer#L169.

The symbolized stack trace here.

@tobrun tobrun mentioned this pull request Mar 20, 2019
@ivovandongen
Copy link
Contributor

@tobrun Like we discussed on chat. I think ideally we would introduce finer grained callbacks, distinguishing from (EGL) context and surface creation/destroys. Cleanup of the renderer / backend can then happen on the context related signals and no longer on surface signals so that re-use of the context with a different surface can be supported seamlessly.

In the TextureViewRenderer we have plenty of control and it should be relatively easy to do. For the GLSurfaceViewRenderer, I think we can set a custom EGLContextFactory that can provide us the needed signals.

Let's make that a new issue though and continue on this path now.

@ivovandongen
Copy link
Contributor

BTW: great addition of the integration tests! Makes everything so much easier to reproduce.

@tobrun tobrun force-pushed the tvn-reuse-view-crash branch 2 times, most recently from 14f96bc to f109f38 Compare March 22, 2019 13:47
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Thanks for all those tests! A couple of minor comments/questions below.

@tobrun tobrun force-pushed the tvn-reuse-view-crash branch 3 times, most recently from 3b19a79 to 9f71a1d Compare March 22, 2019 16:48
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

:shipit:

@tobrun tobrun merged commit dcd6efd into master Mar 22, 2019
@tobrun tobrun deleted the tvn-reuse-view-crash branch March 22, 2019 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants