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

Notification fix #1175

Conversation

reactorcoremeltdown
Copy link
Contributor

Fixes #1151

It appears to be a race condition. It is not based on any kind of thorough debugging and whatsoever, I just had a look at other Java/Kotlin based music players that use the same approach.

The culprit, at least as I see it, is metadataBuilder that pushes album art bitmap to the current metadata block.

Putting a metadata builder function call right after pushing cover art to metadata seems to cure this problem.

Also moved the Playback status update after metadata push, as it happens in some other players.

For the reference: https://github.com/SimpleMobileTools/Simple-Music-Player/blob/ef6706fc3f333264b06726ae90fd41ab85a2731b/app/src/main/kotlin/com/simplemobiletools/musicplayer/services/MusicService.kt#L716-L720

Please feel free to close if the PR does not fit for any reason, but on my device it seems to have fixed the problem.

@@ -124,6 +112,9 @@ public void updateSession(Song song, int state) {
boolean showCover = SharedPrefHelper.getSettings(mContext).getBoolean(PrefKeys.COVER_ON_LOCKSCREEN, PrefDefaults.COVER_ON_LOCKSCREEN);
if (showCover) {
metadataBuilder.putBitmap(MediaMetadataCompat.METADATA_KEY_ALBUM_ART, cover);
// Checking this for debug purposes
// I suspect Vanilla fails at rendering album art timely
mMediaSession.setMetadata(metadataBuilder.build());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed? I don't see why this is needed since we will call the same function on line 134 anyway later

@@ -134,6 +125,19 @@ public void updateSession(Song song, int state) {
mMediaSession.setMetadata(metadataBuilder.build());
}

// Moved this block below to potentially fix the race condition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with moving this down here - but lets remove the comment :)

@eylenburg
Copy link

If it works, can it be merged please?

@reactorcoremeltdown
Copy link
Contributor Author

If it works, can it be merged please?

Apologies for a big delay, it's been quite an unpleasant start of the year for me, let alone my Android app pipeline that failed entirely, so I'll have to service it once again.

Regarding the fix: it does not work as expected. I really suspect there is some kind of a race condition. However, after forcing Vanilla to change playback status(toggle play/pause), it somehow picks and starts displaying correct metadata.

This is quite irritating for me too, as I use Vanilla in conjunction with my fitness bracelet & Gadgetbridge app. I'll see if I could fix my Android pipeline any time soon; if there will be no progress during next 2 weeks, I'll change the status of the PR.

Thanks everyone for patience!

@reactorcoremeltdown
Copy link
Contributor Author

Found more consistent solution to this problem, submitting in a few.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification bar doesn't update
3 participants