New audio media player service #4208

ezaquarii · 2019-07-08T19:40:33Z

This is a new media service based on state machine to address #3061. I tried to salvage the old solution but decided to do a complete re-work. I firmly believe that new architecture will allow us to fix any current and future edge cases easily.

However, the work is not complete yet and there are few issues to consider before we move forward.
Some featutres are cut out, some didn't work anyway, so we need to make Decisions(tm).

Restoring minimized player exists preview

Open media preview
Minimize the screen (media stops)
Restore app from recent apps
Expected: media preview screen, maybe resume playback?
Actual: media preview fragment is closed

I discovered that this also happens on current master, so this is an existing issue, not a regression.
This is most likely caused by FileDisplayActivity.onAccountSet(false) which re-sets displayed fragments.

Total time is not displayed when streaming media

I could not confirm if this is regression, as streaming media does not work on prod and master.
It seems that android.media.MediaPlayer returns 0. I can imagine that media file header is not available in streaming mode, but also the media player could do something smart to work around it, so I'd like to have a 2nd opinion.

Here are my results:

	mp3	ogg
total	0:00	0:00
progress bar	0:00	stuck at 0:02
rewind	nope	nope

I'll need some help with testing it in other environments, as I'm not sure if this is expected to work (which require more work, obviously) or not (which require setting some flags to disable those controls).

Media control is a bit wonky

Play-pause is not propagated correctly as it is polling the player. This is something to be addressed with further refactoing.

Audio manager

~~Audio focus handling is still not implemented, as it is not required at this stage.~~
Implemented.

Auto-stop on file deletion

Auto-stop on file deletion is currently removed (I left the placeholder to trigger it). Closing preview stops the player and it seems that background playback was not implemented anyway.

Notification is removed for now

~~Again, no background playback == not needed at this stage. Besides, it didn't provide controls, so more work is needed if we want to bring that into usable state.~~
Old notification behavior is restored.

Last but not least is the StateMachine framework

No longer an issue - I'm a maintainer and stateless4j is in good shape again.

@tobiasKaminsky @AndyScherzinger @DPTJKKVH Please give it a go and let me know how we'd like to proceed with that.

Fixes #4515

DPTJKKVH · 2019-07-08T23:28:52Z

@ezaquarii @nextcloud-android-bot Amazing! I will give it a thorough test tomorrow after work. (Tuesday)

redtux · 2019-07-09T17:41:43Z

Unortunately, it is not working. I have just installed the above build and successfully logged in, but when clicking play on an mp3 file the client immediately crashes. (Been trying several times, so it's reproducible.) 😕

ezaquarii · 2019-07-09T19:14:34Z

Unortunately, it is not working. I have just installed the above build and successfully logged in, but when clicking play on an mp3 file the client immediately crashes. (Been trying several times, so it's reproducible.) confused

Thanks for testing.

I'll try to ship another build with ACRA crash reporter later this evening, so people actually can get a chance to submit helpful bug reports.

~~In the meantime, if you can get any logs from logcat (I know few people have development tools installed, so no worries if you can't), it would help to figure it out.~~ Ne need, the app provides the stack trace now.

redtux · 2019-07-09T20:27:25Z

I have logcat installed, but my phone is not rooted and the adb command is not working. I'll try your next build and upload the resulting output here. Thank you in advance! 👍

DPTJKKVH · 2019-07-09T20:48:15Z

I will wait for the new build as well. Thanks from me, too! 👍

src/main/java/com/nextcloud/client/media/Player.kt

src/main/java/com/nextcloud/client/media/PlayerError.kt

src/main/java/com/nextcloud/client/media/PlayerStateMachine.kt

src/main/java/com/owncloud/android/media/MediaControlView.java

tobiasKaminsky

Some small questions / change ideas

ezaquarii · 2019-09-27T06:54:18Z

Finite State Machine aka FSM is a well recognized abbreviation used in CS literature. https://en.m.wikipedia.org/wiki/Finite-state_machine I get that we don't do abbreviations to be more friendly to contributors so as this is our cultural standard I'll change it to stateMachine. Np.

tobiasKaminsky · 2019-09-27T06:56:21Z

Bug(?):

start an audio
suspend app
restart app
click on pause
click on back
still see "is playing" notification

Old behaviour was that when pausing (as we do not have a stop) and leaving player the notification is gone away.

tobiasKaminsky · 2019-09-27T06:57:13Z

Except this, it looks very very good and is working fine 🎉 🎉 🎉 🎉
I guess this is a huge enhancement.
Thanks again for your wonderful work!

ezaquarii · 2019-09-27T18:18:14Z

Bug(?):

Weeeeell, yes and no, aka it's complicated.

Previous implementation simply turned off service foreground mode, but did not care about clearing the player state nor even releasing the binders.

This had a side effect of clearing the icon, but the player was left in paused state. It was usually also bound to FileListActivity (and derivatives) as well, preventing the service from being destroyed by the OS most of the time.

Relying on a background service to retain it's state guarantees to have funky edge issues caused by Android resources reclamation.

Current implementation preserves the backgrounding behaviour of old player, but does not disable foreground mode, which manifests itself with the icon being still on screen (because the player is technically still up and running, just paused). Otherwise it works as the old one MINUS possible breakages on service kill.

I added a STOP button to the notification, so you can stop the player if needed.

I was hoping that we simply add proper media player controls to this notification, like every other media player does, allowing the user to go back, pause, resume and stop from there.

If the old behavior is important to retain, we could:

bind the service to MainApp to protect it against Grim Reaper in paused-and-not-foreground state
toggle foreground mode when entering/exiting paused state

This won't play nice if we want to have media controls in notification, as pausing it would close the media controls (and then you can resume it again of course).

We have some complaints about the current pause/stop experience, and I think this "magical" pausing is not a good UX at all.

Let me know what we want to do here.

Fixes #3061 Fixes #4412 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

nextcloud-android-bot · 2019-09-27T19:17:41Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11019.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

… [skip ci] Signed-off-by: nextcloud-android-bot <android@nextcloud.com>

nextcloud-android-bot · 2019-09-27T19:20:37Z

Codacy

277

Lint

Type	master	PR
Warnings	59	59
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	139
Total	427

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	141
Total	429

nextcloud-android-bot · 2019-09-27T19:23:04Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11020.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2019-09-27T19:27:41Z

Codacy

277

Lint

Type	master	PR
Warnings	59	59
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	139
Total	427

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	27
Correctness Warnings	69
Internationalization Warnings	12
Malicious code vulnerability Warnings	4
Multithreaded correctness Warnings	9
Performance Warnings	120
Security Warnings	47
Dodgy code Warnings	141
Total	429

tobiasKaminsky · 2019-10-01T05:44:52Z

I added a STOP button to the notification, so you can stop the player if needed.

Nice, I think this is enough.

I was hoping that we simply add proper media player controls to this notification, like every other media player does, allowing the user to go back, pause, resume and stop from there.

This would indeed be very cool, but should be done in another PR 👍

tobiasKaminsky · 2019-10-01T05:45:50Z

Thanks again for this great PR 🎉 🎉 🎉

ea696c5 Merge pull request #4425 from nextcloud/autoUploadSubfolder 37c6600 Upload files into subfolder a1c28f9 Merge pull request #4470 from stephanritscher/master 9c1fedb add test cases 6184fcd add test cases 0f97564 Merge pull request #4208 from nextcloud/ezaquarii/new-media-player-service d83792e [tx-robot] updated from transifex 9a2fc28 [tx-robot] updated from transifex f10843e daily dev 20190928

ezaquarii requested review from AndyScherzinger and tobiasKaminsky July 8, 2019 19:40

ezaquarii mentioned this pull request Jul 9, 2019

The built-in media player is unable to play the media file #3601

Closed

ezaquarii force-pushed the ezaquarii/new-media-player-service branch from 4157b1b to fbec2d5 Compare July 9, 2019 20:48

ezaquarii force-pushed the ezaquarii/new-media-player-service branch from fbec2d5 to c716468 Compare July 9, 2019 20:51

nextcloud deleted a comment Jul 9, 2019