Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Will PR) MediaPlayer on Android is *not* thread safe, but it is used in multiple threads, causing subtle bugs #1524

Closed
fzyzcjy opened this issue May 29, 2023 · 4 comments
Labels
bug platform-android Affects the android platform

Comments

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 29, 2023

I can make a PR for this.

https://developer.android.com/reference/android/media/MediaPlayer says:

MediaPlayer is not thread-safe. Creation of and all access to player instances should be on the same thread.

However, I have made some logging and realize the call to MediaPlayer in this library is from multiple threads.

For example, I have seen:

  1. mediaPlayer.reset (thread=6854, this=e3ed388, )
  2. mediaPlayer.stop (thread=2, this=e3ed388, )

If you are interested, my logging code looks like:

    override fun start() {
        log("mediaPlayer.start")
        mediaPlayer.start()
    }

    private fun log(message: String) {
        MyFlutterLog.log(
            TAG,
            "$message " +
                    "(" +
                    "thread=${Thread.currentThread().id.toString(16)}, " +
                    "this=${myShortHash(this)}, " +
                    ")"
        )
    }
@fzyzcjy fzyzcjy added the bug label May 29, 2023
@fzyzcjy fzyzcjy changed the title MediaPlayer on Android is *not* thread safe, but it is used in multiple threads, causing subtle bugs (Will PR) MediaPlayer on Android is *not* thread safe, but it is used in multiple threads, causing subtle bugs May 29, 2023
@Gustl22 Gustl22 added the platform-android Affects the android platform label May 30, 2023
@Gustl22
Copy link
Collaborator

Gustl22 commented May 30, 2023

It depends from where it's called. Usually it helps if wrapping the call in the main loop handler. But we should think about running the asynchronous stuff also on the main thread.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 30, 2023

UPDATE

The cause of multi-threading is not what I originally thought (I wrongly thought Flutter calls the kotlin code on non-ui threads). Instead, it is this:

class AudioplayersPlugin : FlutterPlugin, IUpdateCallback {

...
        methods = MethodChannel(binding.binaryMessenger, "xyz.luan/audioplayers")
        methods.setMethodCallHandler { call, response -> safeCall(call, response, ::methodHandler) }
...

    private fun safeCall(
        call: MethodCall,
        response: MethodChannel.Result,
        handler: FlutterHandler,
    ) {
        mainScope.launch(Dispatchers.IO) {
            try {
                handler(call, response)
            } catch (e: Exception) {
                response.error("Unexpected AndroidAudioError", e.message, e)
            }
        }
    }

The Dispatchers.IO says:

The CoroutineDispatcher that is designed for offloading blocking IO tasks to a shared pool of threads.
Additional threads in this pool are created and are shutdown on demand.

Thus, it is this safeCall that moves things to the non-UI threads.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 30, 2023

ANOTHER UPDATE

ExoPlayer (related: #1526) says that, it should (usually) be called directly on the ui thread. Therefore, if we refactor into ExoPlayer, we can safely remove the safeCall without the worry that running things on ui thread may block anything.

@Gustl22
Copy link
Collaborator

Gustl22 commented Jun 12, 2023

Closing in favor of #1525

@Gustl22 Gustl22 closed this as completed Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug platform-android Affects the android platform
Projects
None yet
Development

No branches or pull requests

2 participants