mapbox / mapbox-maps-android

Interactive, thoroughly customizable maps in native Android powered by vector tiles and OpenGL.
https://www.mapbox.com/mobile-maps-sdk
Other
466 stars 131 forks source link

Crash when starting animation from background thread #1556

Closed kmadsen closed 2 years ago

kmadsen commented 2 years ago

When testing solutions for this https://github.com/mapbox/mapbox-maps-android/issues/1413

Ran into this crash

2022-08-02 14:05:14.328 15236-15358/com.mapbox.navigation.qa_test_app E/AndroidRuntime: FATAL EXCEPTION: animation_thread
    Process: com.mapbox.navigation.qa_test_app, PID: 15236
    com.mapbox.maps.exception.WorkerThreadException: The exception that is thrown when an application attempts to 
    perform a map operation on a worker thread.
        at com.mapbox.maps.ThreadChecker.throwIfNotMainThread(ThreadChecker.kt:88)
        at com.mapbox.maps.MapboxMap.checkNativeMap(MapboxMap.kt:1722)
        at com.mapbox.maps.MapboxMap.checkNativeMap$default(MapboxMap.kt:1720)
        at com.mapbox.maps.MapboxMap.setUserAnimationInProgress(MapboxMap.kt:490)
        at com.mapbox.maps.plugin.animation.CameraAnimationsPluginImpl$registerInternalListener$1.onAnimationStart(CameraAnimationsPluginImpl.kt:279)
        at android.animation.Animator$AnimatorListener.onAnimationStart(Animator.java:539)
        at android.animation.ValueAnimator.notifyStartListeners(ValueAnimator.java:1031)
        at android.animation.ValueAnimator.startAnimation(ValueAnimator.java:1280)
        at android.animation.ValueAnimator.start(ValueAnimator.java:1082)
        at android.animation.ValueAnimator.start(ValueAnimator.java:1106)
        at com.mapbox.maps.plugin.animation.animator.CameraAnimator.start(CameraAnimator.kt:84)
        at android.animation.ValueAnimator.startWithoutPulsing(ValueAnimator.java:1099)
        at android.animation.AnimatorSet.handleAnimationEvents(AnimatorSet.java:1149)
        at android.animation.AnimatorSet.startAnimation(AnimatorSet.java:1234)
        at android.animation.AnimatorSet.start(AnimatorSet.java:729)
        at android.animation.AnimatorSet.start(AnimatorSet.java:684)
        at com.mapbox.navigation.ui.maps.AnimatorStarter.start$lambda-0(AnimatorStarter.kt:24)
        at com.mapbox.navigation.ui.maps.AnimatorStarter.$r8$lambda$2IUz_vHjCSjMs8QFoHeCgNrZq9g(Unknown Source:0)
        at com.mapbox.navigation.ui.maps.AnimatorStarter$$ExternalSyntheticLambda0.run(Unknown Source:2)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:210)
        at android.os.Looper.loop(Looper.java:299)
        at android.os.HandlerThread.run(HandlerThread.java:67)

Verifying ValueAnimator works on background thread

Here is the code I used to verify that ValueAnimator is designed to work on a background thread. This code can be used in an app without mapbox to create animations. It's the mapbox WorkerThreadException that is preventing downstream from using a background thread for camera animations.

        var lastAnimation: LogAnimation? = null
        lifecycle.addObserver(object : DefaultLifecycleObserver {
            override fun onCreate(owner: LifecycleOwner) {
                AnimatorStarter.onAttached()

                lifecycle.coroutineScope.launch {
                    while (isActive) {
                        lastAnimation?.cancel()
                        lastAnimation = LogAnimation().apply {
                            duration = 500
                            AnimatorStarter.start(this)
                        }
                        delay(1000)
                    }
                }
            }

            override fun onDestroy(owner: LifecycleOwner) {
                AnimatorStarter.onDetached()
                lastAnimation?.cancel()
            }
        })

class LogAnimation : ValueAnimator() {
    init {
        addPauseListener(
            onResume = { Log.i("animation_debug", "onResume") },
            onPause = { Log.i("animation_debug", "onPause") },
        )
        addListener(
            onStart = { Log.i("animation_debug", "onStart") },
            onCancel = {
                Log.i("animation_debug","onCancel")
            },
            onEnd = { Log.i("animation_debug", "onEnd") },
            onRepeat = { Log.i("animation_debug", "onRepeat") },
        )
        addUpdateListener {
            Log.i("animation_debug", "onUpdate ${it.animatedValue as Float}")
        }
        setFloatValues(5f, 10f)
        duration = 500
    }
}

object AnimatorStarter {
    private const val HANDLE_THREAD_NAME = "animation_thread"
    private val EVENT: Any = Object()

    private var startTime = 0L
    private var isRunning = false
    private var handlerThread: HandlerThread? = null
    private var handler: Handler? = null

    fun <T: Animator> start(animator: T): T {
        handler?.post {
            animator.start()
        }
        return animator
    }

    fun onAttached() {
        isRunning = true
        startTime = System.nanoTime()
        handlerThread = HandlerThread(HANDLE_THREAD_NAME, Process.THREAD_PRIORITY_DISPLAY).apply {
            start()
            handler = Handler(this.looper)
        }
        Log.i("animation_debug", "LogAnimation startThread")
    }

    fun onDetached() {
        handler?.removeCallbacksAndMessages(EVENT)
        handlerThread?.quitSafely()
        handler = null
        isRunning = false
    }
}
yunikkk commented 2 years ago

@kmadsen we indeed require MapboxMap to be called from the main thread only. In Debug builds we have a check that will throw an exception, however in Release it won't. Btw - is Xiaomi you're running fully stopping Main thread when app is in BG (or only Choreographer is stopped) ? Is it possible to post animation updates to main?

kmadsen commented 2 years ago

Is it possible to post animation updates to main?

It's the Choreographer.getInstance().postFrameCallback that needs to be posted by a background thread.

A callstack example is through Animator.start

Choreographer.getInstance().postFrameCallback -- AnimationHandler.addAnimationFrameCallback --|-- ValueAnimator.start()

@yunikkk Why not make it so MapboxMap will declare the main thread when the call stack reaches Mapbox domain in the CameraAnimationsPluginImpl?

yunikkk commented 2 years ago

@yunikkk Why not make it so MapboxMap will declare the main thread when the call stack reaches Mapbox domain in the CameraAnimationsPluginImpl?

That's definitely possible, guess we just hadn't seen a reason to allow animator to run from the BG thread and post updates to main internally.

It's the Choreographer.getInstance().postFrameCallback that needs to be posted by a background thread.

Not sure I follow, you're saying that Choreographer is working on the BG thread? I was asking whether MainThread's looper itself is working and simple mainHandler.post will be handled while app is in bg. Guess it is, since platform android views (eg speed limit) are updating

kmadsen commented 2 years ago

Not sure I follow, you're saying that Choreographer is working on the BG thread? I was asking whether MainThread's looper itself is working and simple mainHandler.post will be handled while app is in bg. Guess it is, since platform android views (eg speed limit) are updating

On Xiaomi, calls to Choreographer.postFrameCallback are ignore when called from the main thread (only when the app is put into background). The MIUI has a unique Choreographer with this behavior. https://github.com/mapbox/mapbox-maps-android/issues/1413. Some execution is needed in order to see animations or render updates by Android Auto head units.

The MapboxRenderThread is working on Xiaomi devices for this reason, it is posting the frame calls from a background thread.

kiryldz commented 2 years ago

Could be closed as the approach was revisited in https://github.com/mapbox/mapbox-maps-android/pull/1638.