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

chore: improve metadata handling #85

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Conversation

puckey
Copy link
Contributor

@puckey puckey commented Jul 4, 2023

  • fix pixelated display of artwork (see Android 13 Notification Player Images are pixelated and blurry react-native-track-player#1934)
  • move MediaSource.getMediaMetadataCompat() extension to MediaItem.getAudioItemHolder()
  • introduce MediaItem.getAudioItemHolder() extension
  • throttle invalidate function directly
  • avoid unnecessary resetting of media metadata provider
  • fix unnecessary resetting of artwork bitmap in notificationMetadata setter
  • remove tail call of invalidate (unless someone can show it is needed)
  • remove PlayerConfig.androidNotificationThrottleInterval again
  • avoid flickering of notification when changing config by skipping recreation of notification manager when it is not necessary

- fix pixelated display of artwork
- move MediaSource.getMediaMetadataCompat() extension to MediaItem.getAudioItemHolder()
- introduce MediaItem.getAudioItemHolder() extension
- throttle invalidate function directly
- avoid unnecessary resetting of media metadata provider
- fix unnecessary resetting of artwork bitmap in notificationMetadata setter
- remove tail call of invalidate (unless someone can show it is needed)
- remove PlayerConfig.androidNotificationThrottleInterval again
@puckey puckey marked this pull request as draft July 4, 2023 10:46
@puckey
Copy link
Contributor Author

puckey commented Jul 4, 2023

I will be testing this pr tomorrow on various devices

@puckey
Copy link
Contributor Author

puckey commented Jul 5, 2023

Added some changes:

  • treat notification metadata bitmap as seperate from the one belonging to individual tracks
  • introduce notification metadata helpers to avoid duplication on somewhat complicated fallbacks from notification metadata to media metadata to item fields etc
  • inline getMediaMetadataCompat helper into NotificationManager
  • remove notificationMetadataFlow since it isn’t necessary
  • separate cache for now playing bitmap
  • when bitmap is loaded, call invalidate() in order to refire mediaSessionConnector.setMediaMetadataProvider and getCurrentLargeIcon callbacks
  • avoid lockscreen ui jumping when loading artwork by returning transparent placeholder image in getCurrentLargeIcon

Should be ready for review now..

@puckey puckey marked this pull request as ready for review July 5, 2023 20:28
- treat notification metadata bitmap as seperate from the one belonging to individual tracks
- introduce notification metadata helpers to avoid duplication on somewhat complicated fallbacks from notification metadata to media metadata to item fields etc
- inline getMediaMetadataCompat helper into NotificationManager
- remove notificationMetadataFlow since it isn’t necessary
- separate cache for now playing bitmap
- when bitmap is loaded, call invalidate() in order to refire mediaSessionConnector.setMediaMetadataProvider and getCurrentLargeIcon callbacks
- avoid lockscreen ui jumping when loading artwork by returning transparent placeholder image in getCurrentLargeIcon
@puckey
Copy link
Contributor Author

puckey commented Jul 5, 2023

I will be testing this pr tomorrow on various devices

(tested this briefly on 7 devices from android 7 to android 13..)

Copy link
Contributor

@jspizziri jspizziri left a comment

Choose a reason for hiding this comment

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

LGTM

mediaSessionConnector.invalidateMediaSessionMetadata()
if (invalidateThrottleCount++ == 0) {
scope.launch {
internalNotificationManager?.invalidate()
Copy link
Contributor

Choose a reason for hiding this comment

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

still feels like we're spamming the invalidate methods, but I guess if they don't complain and it generally works its fine.

@jspizziri
Copy link
Contributor

Thanks for doing this @puckey !

@puckey
Copy link
Contributor Author

puckey commented Jul 14, 2023

Merged in the following pr which fixes an issue where the controls were flickering when the notification was configured multiple times on older versions of Android: #82

Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Quite the revamp! Thanks for looking into this :) great work

@dcvz dcvz merged commit 4caf58f into doublesymmetry:main Jul 18, 2023
2 checks passed
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.

4 participants