SimpleMobileTools / Simple-Music-Player

A clean music player with a customizable widget, stylish interface and no ads.
https://www.simplemobiletools.com
GNU General Public License v3.0
1.26k stars 410 forks source link

Rewrite using media3 #587

Closed naveensingh closed 10 months ago

naveensingh commented 10 months ago

This pull request introduces significant changes to the project, primarily focusing on modernizing media playback, reliability and simplifying player implementation. The major changes include:

Major changes:

Related Issues:

Although I wasn't actively trying to fix any issues, this change may resolve the following issues:

Testing:

All major testing was performed on the Samsung M34 (Android 13), Motorola G52 (Android 12), Xiaomi Pad 6 (Android 13), and a very weak RealMe phone (Android 11).

Media Playback: I tested media playback on the above devices in different conditions and everything seems to work smoothly. I tested with queue sizes ranging from 10 tracks to 3000 tracks and found no issues. I did stress testing using 5000 monkey events and didn't get any ANRs. Gapless playback: I tested this with a generated tone (sine wave) split into multiple parts. I didn't notice any difference compared to the media player implementation currently in the release version. Android Auto: For ease of testing, I used the desktop head unit emulator and Media Controller Test app. Battery usage: I did not do any testing related to this but as per the docs, ExoPlayer consumes slightly more power than the Android MediaPlayer we were using before. Exoplayer has experimental support for offloading audio processing to DSP when the screen is off but I have worked with that API before when I was experimenting with my own custom wakeword app and I did not find it reliable. We might enable it in the future but it may also cause playback to randomly stop after some time.

Code style:

I may have broken a few rules (or did things that we don't normally do) in this PR:

Checklist:

Additional Notes:

I was initially going to rewrite the whole UI part to work as a media browser like the media3 demo app does but it felt like too much work for not many benefits and it didn't seem like the best idea (plus I didn't feel confident enough about media3 yet). That's why the whole MediaItemProvider logic is there, to keep the browser service part working as nicely as possible (which is now only used by Android Auto and any other external media browsers). Maybe we'll do it in the future.

Although I have tried to keep everything working as before, I may have missed a thing or two. If you find anything broken, please let me know.

tibbi commented 10 months ago

cant test, it crashes even on fresh install on my Android 13

Process: com.simplemobiletools.musicplayer.debug, PID: 19541 java.lang.IllegalStateException: Room cannot verify the data integrity. Looks like you've changed schema but forgot to update the version number. You can simply fix this by increasing the version number. Expected identity hash: c7cc50ecee4eade523754a064beb4aae, found: 13a4da587bff027d4690fc3b0845863a at androidx.room.RoomOpenHelper.checkIdentity(RoomOpenHelper.kt:147) at androidx.room.RoomOpenHelper.onOpen(RoomOpenHelper.kt:128) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onOpen(FrameworkSQLiteOpenHelper.kt:287) at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:527) at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:413) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableOrReadableDatabase(FrameworkSQLiteOpenHelper.kt:232) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.innerGetDatabase(FrameworkSQLiteOpenHelper.kt:190) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getSupportDatabase(FrameworkSQLiteOpenHelper.kt:151) at androidx.sqlite.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.kt:104) at androidx.room.RoomDatabase.inTransaction(RoomDatabase.kt:638) at androidx.room.RoomDatabase.assertNotSuspendingTransaction(RoomDatabase.kt:457) at com.simplemobiletools.musicplayer.interfaces.QueueItemsDao_Impl.getAll(QueueItemsDao_Impl.java:211) at com.simplemobiletools.musicplayer.extensions.MediaControllerKt$maybePreparePlayer$1.invoke(MediaController.kt:46) at com.simplemobiletools.musicplayer.extensions.MediaControllerKt$maybePreparePlayer$1.invoke(MediaController.kt:45) at com.simplemobiletools.commons.helpers.ConstantsKt.ensureBackgroundThread$lambda-0(Constants.kt:442) at com.simplemobiletools.commons.helpers.ConstantsKt.$r8$lambda$ukHVGNDmwl8QsD9hCK7IQG-Hwww(Unknown Source:0) at com.simplemobiletools.commons.helpers.ConstantsKt$$ExternalSyntheticLambda0.run(Unknown Source:2)

naveensingh commented 10 months ago

I'm checking it. Maybe you had the debug version installed? Added a new cover art column in the genre table, the database migration was handled from the last release since the later changes weren't released yet. Clearing data or a uninstall/reinstall should fix it.

naveensingh commented 10 months ago

nah, that crash won't/doesn't happen on a fresh install. I tested on two Android 13s and one Android 12.

naveensingh commented 10 months ago

this is still in the draft because I'm sorting out how to reorder items in the queue while shuffle mode is enabled in exoplayer. It seems to reshuffle everything when a new media item is added.

tibbi commented 10 months ago

it crashes on fresh install, not some update

naveensingh commented 10 months ago

Could you please do the following and run it again?

If you still see the crash, I guess I'll handle the migration from the master branch instead of the last release. I'm asking because I also encountered the crash a couple of times and every time I had the master version already installed. Expected identity hash: c7cc50ecee4eade523754a064beb4aae, found: 13a4da587bff027d4690fc3b0845863a is exactly what I see if I install the master version and then install the media3 version. 13a4da587bff027d4690fc3b0845863a corresponds to the current master database schema.

tibbi commented 10 months ago

alright it works now, I could compile and run it. Will test actual functionality soon.

tibbi commented 10 months ago

done some quick tests in the car too, it seems to be more reliable than the previous version :) One bug I found is that the queue in the car always shows 1/1, while the previous version properly showed the current and max tracks like 3/20. And in the car there are now 2 new buttons for toggling shuffle and repetition mode, which werent there previously at all. Pressing them has no effect on the app, but I havent tested it fully yet. Have you met something like that?

naveensingh commented 10 months ago

One bug I found is that the queue in the car always shows 1/1

Do you have "Repeat current song" enabled? It might do that in that case since exoplayer handles repetition modes somewhat differently than what we had with the media player.

in the car there are now 2 new buttons for toggling shuffle and repetition mode, which werent there previously at all. Pressing them has no effect on the app, but I havent tested it fully yet. Have you met something like that?

I haven't seen anything like that yet. Are you using Bluetooth connection (as always) or Android Auto?

tibbi commented 10 months ago

I dont have Repeat current song. And ye, I use bluetooth, just like previously.

naveensingh commented 10 months ago

One bug I found is that the queue in the car always shows 1/1, while the previous version properly showed the current and max tracks like 3/20

It's a media3 + android issue. Other apps using media3 also have this problem. I'll create an issue there (related: https://github.com/androidx/media/issues/430). I checked the master version and most of the time, metadata isn't updated at all on bluetooth devices, however, when it is properly updated, it does show the correct trackNumber/trackCount info.

new buttons for toggling shuffle and repetition mode

I removed the set repeat mode command from the player, can you check if it's still there in the bluetooth console? If not, if it's not that important, we can solve this later.

naveensingh commented 10 months ago

Updated PR description with details. I'm still testing it out for bugs and issues but marking it for review now...

naveensingh commented 10 months ago

actually ForegroundServiceStartNotAllowedException is being thrown in some cases, I'm investigating it...

naveensingh commented 10 months ago

Fixed crashes, please proceed...

naveensingh commented 10 months ago

in the car there are now 2 new buttons for toggling shuffle and repetition mode, which werent there previously at all. Pressing them has no effect on the app

Fixed that too. Those buttons were working as expected, it just wasn't reflected in the UI.

tibbi commented 10 months ago

if a song is paused and I change the current position by clicking on the progressbar or skip forward/back, lets resume the track automatically

naveensingh commented 10 months ago

Should I also resume playback when the next/previous buttons are clicked? That's how it is on master.

tibbi commented 10 months ago

sure, it should resume it

naveensingh commented 10 months ago

done :+1:

tibbi commented 10 months ago

seems to work fine, tested on Android 14 and updating from master too. Lets just replace ctx with context though, it is more readable.

naveensingh commented 10 months ago

Lets just replace ctx with context though

Actually, it was temporary. Renaming it to context will cause it's usage to be replaced with view.getContext() in some places where we have a view as a receiver and I didn't do it to avoid any subtle context-related issues. Later I'll be making the activity in MyRecyclerViewAdapter open so we can override it instead.

Can we rename it to something like musicActivity? or musicContext for now until I update commons?

tibbi commented 10 months ago

lets keep cxt then, do not make it longer unnecessarily

naveensingh commented 10 months ago

okay then

tibbi commented 10 months ago

guess it is ready to be accepted then, next big testing will be done by actual users :) Thanks

naveensingh commented 10 months ago

Cool :) Once this gets released, I'll ask reporters whether the mentioned issues above can now be closed.

tibbi commented 10 months ago

so is this ready to be released? Guess Ill try releasing it slowly.

naveensingh commented 10 months ago

Yes, already tested it on different devices in different configurations and fixed any bugs and crashes I could find. I don't expect any major issues though there may be some minor issues or edge cases that I haven't spotted. Releasing is the only way to do more tests on a larger scale :)

Viewbinding changes will be ready in a day or two so if you wanna wait, wait.

tibbi commented 10 months ago

I wont wait, but I wont be here during the weekend again, so Ill just release it to like 3% to see some initial crashes

naveensingh commented 10 months ago

How many active users are there? 3% sounds about right for a safe start. You could do 5% too.

tibbi commented 10 months ago

631k active. Cant compile master with latest commons though, check it please.

naveensingh commented 10 months ago

latest commons has many library updates and changes that require targeting SDK34 and upgrading the gradle plugin.

Later I'll be making the activity in MyRecyclerViewAdapter open so we can override it instead.

That's why I haven't done that yet.

tibbi commented 10 months ago

ok whatever, so Ill wait for the viewbinding and library updates with the release

naveensingh commented 10 months ago

okay :+1: