Skip to content

Commit

Permalink
Final fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@kiryldz
kiryldz committed Feb 1, 2022
1 parent ee444ef commit 2364753
Show file tree
Hide file tree
Showing 7 changed files with 159 additions and 76 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

Mapbox welcomes participation and contributions from everyone.

# 10.4.0-beta.1

## Features ✨ and improvements 🏁
* Refactor scheduling logic for render thread in general slightly improving rendering performance. ([#1068](https://github.com/mapbox/mapbox-maps-android/pull/1068))

## Bug fixes 🐞
* Fix dropping / crashing in user events scheduled on a render thread with `MapView#queueEvent`. ([#1068](https://github.com/mapbox/mapbox-maps-android/pull/1068))

# 10.3.0 February 7, 2022

## Dependencies
Expand Down
3 changes: 1 addition & 2 deletions ...src/androidTest/java/com/mapbox/maps/testapp/integration/surface/SurfaceViewReopenTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import androidx.test.ext.junit.rules.ActivityScenarioRule
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.LargeTest
import com.mapbox.maps.testapp.examples.SimpleMapActivity
import com.mapbox.maps.testapp.examples.TextureViewActivity
import com.mapbox.maps.testapp.integration.BaseIntegrationTest
import com.mapbox.maps.testapp.integration.launchActivity
import org.junit.Rule
Expand All @@ -30,7 +29,7 @@ class SurfaceViewReopenTest : BaseIntegrationTest() {
device.pressHome()
device.waitForIdle()
activityRule.scenario.onActivity {
device.launchActivity(it, TextureViewActivity::class.java)
device.launchActivity(it, SimpleMapActivity::class.java)
device.waitForIdle()
}
}
Expand Down
8 changes: 8 additions & 0 deletions app/src/main/java/com/mapbox/maps/testapp/examples/SimpleMapActivity.kt
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package com.mapbox.maps.testapp.examples

import android.opengl.GLES20
import android.os.Bundle
import androidx.appcompat.app.AppCompatActivity
import com.mapbox.common.Logger
import com.mapbox.geojson.Point
import com.mapbox.maps.CameraOptions
import com.mapbox.maps.MapView
Expand All @@ -14,6 +16,12 @@ class SimpleMapActivity : AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
val mapView = MapView(this)
mapView.queueEvent(
{
Logger.e("KIRYLDD", GLES20.glGetString(GLES20.GL_VERSION))
},
needRender = false
)
setContentView(mapView)
mapView.getMapboxMap()
.apply {
Expand Down
60 changes: 60 additions & 0 deletions sdk/src/androidTest/java/com/mapbox/maps/RenderTest.kt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package com.mapbox.maps

import android.opengl.GLES20
import androidx.test.ext.junit.rules.ActivityScenarioRule
import androidx.test.filters.LargeTest
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.Parameterized
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit
import java.util.concurrent.TimeoutException

@RunWith(Parameterized::class)
@LargeTest
class RenderTest(
private val useTexture: Boolean,
private val eventNeedRender: Boolean,
) {

@get:Rule
val rule = ActivityScenarioRule(EmptyActivity::class.java)

@Test
fun userQueueEventNeedRender() {
val latch = CountDownLatch(1)
var openGlVersionString = ""
rule.scenario.onActivity {
it.runOnUiThread {
val mapView = MapView(it, MapInitOptions(it, textureView = useTexture))
mapView.queueEvent(
{
openGlVersionString = GLES20.glGetString(GLES20.GL_VERSION)
// assume anything should already have OpenGL ES 3+...
assert(openGlVersionString.contains("OpenGL ES 3."))
latch.countDown()
},
needRender = eventNeedRender
)
it.frameLayout.addView(mapView)
mapView.onStart()
}
}
if (!latch.await(LATCH_TIMEOUT_SEC, TimeUnit.SECONDS)) {
throw TimeoutException()
}
}

companion object {
private const val LATCH_TIMEOUT_SEC = 5L
@JvmStatic
@Parameterized.Parameters
fun data() = listOf(
arrayOf(/* useTexture */ true, /* eventNeedRender */ true),
arrayOf(/* useTexture */ true, /* eventNeedRender */ false),
arrayOf(/* useTexture */ false, /* eventNeedRender */ true),
arrayOf(/* useTexture */ false, /* eventNeedRender */ false),
)
}
}
99 changes: 38 additions & 61 deletions sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderThread.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import androidx.annotation.WorkerThread
import com.mapbox.common.Logger
import com.mapbox.maps.renderer.egl.EGLCore
import java.util.LinkedList
import java.util.concurrent.CopyOnWriteArrayList
import java.util.concurrent.locks.ReentrantLock
import javax.microedition.khronos.egl.EGL10
import javax.microedition.khronos.egl.EGL11
Expand Down Expand Up @@ -51,7 +50,10 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal var renderTimeNs = 0L
private var expectedVsyncWakeTimeNs = 0L
private var awaitingNextVsync = false

@Volatile
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal var awaitingNextVsync = false
private var sizeChanged = false
@Volatile
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
Expand All @@ -65,7 +67,8 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
/**
* We track moment when EGL context is created and associated with current Android surface.
*/
private var eglContextCreated = false
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal var eglContextCreated = false

/**
* Render thread should be treated as valid (prepared to render a map) when both flags are true.
Expand Down Expand Up @@ -221,16 +224,7 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
// assuming render event queue holds user's runnables with OpenGL ES commands
// it makes sense to execute them after drawing a map but before swapping buffers
// **note** this queue also holds snapshot tasks
renderEventQueueLock.withLock {
renderEventQueue.apply {
if (isNotEmpty()) {
forEach {
it.runnable?.run()
}
clear()
}
}
}
drainQueue(renderEventQueueLock, renderEventQueue)
when (val swapStatus = eglCore.swapBuffers(eglSurface)) {
EGL10.EGL_SUCCESS -> {}
EGL11.EGL_CONTEXT_LOST -> {
Expand Down Expand Up @@ -354,28 +348,8 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
}
this.width = width
this.height = height
// we clean only Mapbox events to avoid outdated runnables associated with previous EGL context
renderEventQueueLock.withLock {
// using iterator to avoid concurrent modification exception
val iterator = renderEventQueue.iterator()
while (iterator.hasNext()) {
val next = iterator.next()
if (next.eventType == EventType.SDK) {
iterator.remove()
}
}
}
// TODO use function
nonRenderEventQueueLock.withLock {
// using iterator to avoid concurrent modification exception
val iterator = nonRenderEventQueue.iterator()
while (iterator.hasNext()) {
val next = iterator.next()
if (next.eventType == EventType.SDK) {
iterator.remove()
}
}
}
clearQueue(renderEventQueueLock, renderEventQueue)
clearQueue(nonRenderEventQueueLock, nonRenderEventQueue)
// we do not want to clear render events scheduled by user
renderHandlerThread.clearMessageQueue(clearAll = false)
prepareRenderFrame(creatingSurface = true)
Expand All @@ -399,28 +373,12 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
@WorkerThread
override fun doFrame(frameTimeNanos: Long) {
// it makes sense to draw not only when EGL config is prepared but when native renderer is created
Logger.e("KIRYLDD", "do Frame, size start: ${nonRenderEventQueue.size}")
if (renderThreadPrepared && !paused) {
draw()
}
// TODO use function, logic same as for render
Logger.e("KIRYLDD", "doFrame before lock")
nonRenderEventQueueLock.withLock {
nonRenderEventQueue.apply {
if (isNotEmpty()) {
forEach {
Logger.e("KIRYLDD", "doFrame in lock, run ${it.runnable.hashCode()} start")
it.runnable?.run()
Logger.e("KIRYLDD", "doFrame in lock, run ${it.runnable.hashCode()} end")
}
Logger.e("KIRYLDD", "doFrame in lock clear")
clear()
}
}
}
Logger.e("KIRYLDD", "doFrame after lock")
// we drain queue despite buffers swapped are not
drainQueue(nonRenderEventQueueLock, nonRenderEventQueue)
awaitingNextVsync = false
Logger.e("KIRYLDD", "do Frame, size end: ${nonRenderEventQueue.size}")
}

@AnyThread
Expand All @@ -432,7 +390,6 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
}
}
// in case of native Mapbox events we schedule render event only when we're fully ready for render
// TODO ideally to be fixed in core
if (renderEvent.eventType == EventType.SDK) {
if (renderThreadPrepared) {
postPrepareRenderFrame()
Expand All @@ -457,20 +414,14 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
// if we already waiting listening for next VSYNC then add runnable to queue to execute
// after actual drawing otherwise execute asap on render thread;
if (awaitingNextVsync && !nonRenderEventQueueLock.isLocked) {
Logger.e("KIRYLDD", "postNonRenderEvent before lock")
nonRenderEventQueueLock.withLock {
Logger.e("KIRYLDD", "postNonRenderEvent in lock, add ${renderEvent.runnable.hashCode()}")
nonRenderEventQueue.add(renderEvent)
}
Logger.e("KIRYLDD", "postNonRenderEvent after lock")
} else {
Logger.e("KIRYLDD", "postNonRenderEvent postDelayed start")
renderHandlerThread.postDelayed(
{
if (renderThreadPrepared || renderEvent.eventType == EventType.DESTROY_RENDERER) {
Logger.e("KIRYLDD", "postNonRenderEvent postDelayed inside run ${renderEvent.runnable.hashCode()} start")
renderEvent.runnable?.run()
Logger.e("KIRYLDD", "postNonRenderEvent postDelayed inside run ${renderEvent.runnable.hashCode()} end")
} else {
Logger.w(TAG, "Non-render event could not be run, retrying in $RETRY_DELAY_MS ms...")
postNonRenderEvent(renderEvent, delayMillis = RETRY_DELAY_MS)
Expand All @@ -479,7 +430,6 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
delayMillis,
renderEvent.eventType
)
Logger.e("KIRYLDD", "postNonRenderEvent postDelayed end")
}
}

Expand Down Expand Up @@ -518,6 +468,33 @@ internal class MapboxRenderThread : Choreographer.FrameCallback {
mapboxRenderer.map = null
}

private fun clearQueue(lock: ReentrantLock, queue: LinkedList<RenderEvent>) {
// we clean only Mapbox events to avoid outdated runnables associated with previous EGL context
lock.withLock {
// using iterator to avoid concurrent modification exception
val iterator = queue.iterator()
while (iterator.hasNext()) {
val next = iterator.next()
if (next.eventType == EventType.SDK) {
iterator.remove()
}
}
}
}

private fun drainQueue(lock: ReentrantLock, queue: LinkedList<RenderEvent>) {
lock.withLock {
queue.apply {
if (isNotEmpty()) {
forEach {
it.runnable?.run()
}
clear()
}
}
}
}

companion object {
private const val TAG = "Mbgl-RenderThread"
private val ONE_SECOND_NS = 10.0.pow(9.0).toLong()
Expand Down
1 change: 0 additions & 1 deletion sdk/src/main/java/com/mapbox/maps/renderer/MapboxRenderer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ internal abstract class MapboxRenderer : MapClient {

@AnyThread
override fun scheduleTask(task: Task) {
Logger.e("KIRYLDD", "scheduleTask from core")
renderThread.queueRenderEvent(
RenderEvent(
runnable = { task.run() },
Expand Down
Loading

0 comments on commit 2364753

Please sign in to comment.