[BUG] Android SoundPlayer not working along firebase_messaging

diegogarciar commented 3 years ago

Sounds Version :

sounds: ^1.0.0-beta13

Severity

Result is not what expected
Minor issue

Platforms you faced the error

Android

Logs

(This is very important. Most of the time we cannot do anything if we do not have information on your bug)

flutter doctor :

[✓] Flutter (Channel stable, 1.22.5, on macOS 11.1 20C69 darwin-x64, locale en-MX)

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.2) [✓] Xcode - develop for iOS and macOS (Xcode 12.3) [✓] Android Studio (version 4.1) [✓] VS Code (version 1.52.1) [✓] Connected device (3 available)

flutter pub deps :

just firebase_messaging, here is a repo of a project replicating the issue https://github.com/markusaksli-nc/sounds-messaging-issue

Screenshots

Describe the bug I submitted this bug on firebase a while ago but I think it is also worth it to mention it here. Theres a firebase_messaging function to receive background notifications and when the function is registered, the communication from Android to Dart stops working in this plugin. I honestly don't know which package to blame, but I think a lot of us might use this combination of plugins and the bug took me hours to detect that could save time to others.

The issue is well described in the original firebase issue. so I'll keep it short here

To Reproduce Steps to reproduce the behavior:

Register onBackgroundMessage function from firebase_messaging
Play audio file
See error

Expected behavior A clear and concise description of what you expected to happen. The progress indicator would not move, it seems that android is indeed sending updates of the progress but dart is not receiving them. After commenting the registration of the background message, the progress indicators works as expected.

Additional context**

I did some research on their plugin and it seems that they are creating a new FlutterEngine on that function, maybe this plugin (or all plugins) are confusing it with the original FlutterEngine?

bsutton commented 3 years ago

The first possibility that Springs to mind if that I think the callbacks need to be in the main UI thread.

I wonder if firebase results in the callbacks bring scheduled in a different thread and we haven't done enough work to ensure the callbacks are in the main thread.

On Fri, 15 Jan 2021, 3:18 am Diego Garcia, notifications@github.com wrote:

Sounds Version :

sounds: ^1.0.0-beta13

Severity

Result is not what expected

Minor issue

Platforms you faced the error

Android

Logs

(This is very important. Most of the time we cannot do anything if we do not have information on your bug)

flutter doctor :

[✓] Flutter (Channel stable, 1.22.5, on macOS 11.1 20C69 darwin-x64, locale en-MX)

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.2) [✓] Xcode - develop for iOS and macOS (Xcode 12.3) [✓] Android Studio (version 4.1) [✓] VS Code (version 1.52.1) [✓] Connected device (3 available)

flutter pub deps :

just firebase_messaging, here is a repo of a project replicating the issue https://github.com/markusaksli-nc/sounds-messaging-issue

Screenshots

https://user-images.githubusercontent.com/17276155/101049648-f20c3f00-3540-11eb-923d-58c1ad464b92.png

Describe the bug I submitted this bug on firebase https://github.com/FirebaseExtended/flutterfire/issues/4301 a while ago but I think it is also worth it to mention it here. Theres a firebase_messaging function to receive background notifications and when the function is registered, the communication from Android to Dart stops working in this plugin. I honestly don't know which package to blame, but I think a lot of us might use this combination of plugins and the bug took me hours to detect that could save time to others.

The issue is well described in the original firebase issue. so I'll keep it short here

To Reproduce Steps to reproduce the behavior:

Register onBackgroundMessage function from firebase_messaging

Play audio file

See error

Expected behavior A clear and concise description of what you expected to happen. The progress indicator would not move, it seems that android is indeed sending updates of the progress but dart is not receiving them. After commenting the registration of the background message, the progress indicators works as expected.

Additional context**

I did some research on their plugin and it seems that they are creating a new FlutterEngine on that function, maybe this plugin (or all plugins) are confusing it with the original FlutterEngine?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bsutton/sounds/issues/61, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFY3JK6DEON37X2A3LSZ4KNNANCNFSM4WCVM66A .

diegogarciar commented 3 years ago

Yes its strange, I checked the plugin's android implementation and it is not doing something different from other plugins. From what I learned, each flutterEngine has its own thread, so it shouldn't interfere.

bsutton commented 3 years ago

So I think I've found the problem.

In the SoundPlayer and recorder we have:

@UiThread
    private void sendUpdateProgress(MediaPlayer mp) {
        try {
            JSONObject json = new JSONObject();
            json.put("duration", String.valueOf(mp.getDuration()));
            json.put("current_position", String.valueOf(mp.getCurrentPosition()));
            getPlugin().invokeCallbackWithString(slotNo, "updateProgress", json.toString());

            // reschedule ourselves.
            tickHandler.postDelayed(() -> sendUpdateProgress(mp), (model.progressInterval));
        } catch (Exception e) {
            Log.d(TAG, "Exception: " + e.toString());
        }
    }

I had assumed that magically @UiThread made this method run on the main thread. But that was just lazy thinking :<

So the problem is when we allocate the tick handler:

    final private Handler tickHandler = new Handler();

This needs to be changed to:

    final private Handler tickHandler = new Handler(Looper.getMainLooper());

The same goes for the SoundRecorder.

diegogarciar commented 3 years ago

Awesome! I’m glad you were able to find the bug. Will you be submitting an update?

bsutton commented 3 years ago

Will try to get something out over the next week.

On Fri, 15 Jan 2021, 5:56 pm Diego Garcia, notifications@github.com wrote:

Awesome! I’m glad you were able to find the bug. Will you be submitting an update?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bsutton/sounds/issues/61#issuecomment-760694416, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OEV6FKR2FN6Z2EZJQ3SZ7RJTANCNFSM4WCVM66A .

bsutton commented 3 years ago

Fixed in 1.0.0

bsutton / sounds