Create media session UI and fix player notification #8678

Stypox · 2022-07-22T13:58:39Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR solves one of the points in #8616: it introduces MediaSessionPlayerUi, moving more code out of the Player, and fixes various serious issues related to the player notification and the media session. I tried to put as much info as possible in commit messages and comments, so that it is simpler to debug and git blame.

Some useless classes were removed
- MediaSessionCallback and PlayerMediaSession was just a wrapper for some player functions (half of which used in one place, half in another), but it didn't make any sense not to use the player instance directly bca9543
- All of MediaSessionManager was moved in MediaSessionPlayerUi 49be75e
Make sure to trigger a metadata update after the thumbnail finishes loading, otherwise it ~~could happen~~ almost always happened that the new thumbnail would never be shown. 80df363
Once a new video info starts loading in the player, the thumbnail of the previous video info is unset, so that any metadata update would just get (correctly) a null thumbnail, until the new thumbnail loading finishes. The placeholder thumbnail is not used anymore, as it makes more sense to just not set a thumbnail. 25f40a6
Since the builder used to create player notifications is reused, now the thumbnail is set to null when the user disables all thumbnails from settings while the notification is open (this requires a notification update to kick in), otherwise the lastly set thumbnail would be incorrectly shown. c9c55b1
Let exoplayer decide when to update metadata, instead of deciding ourselves. This seems to fix the wrong seekbar properties (duration and current position) being shown in the notification sometimes. MediaSessionConnector.setMetadataDeduplicationEnabled(true) was used to keep the previous behavior of not updating metadata if nothing changed from the already-set metadata. Now the number of updates is a little bit higher (tested by setting "noisy non-suspending breakpoints"), but there is no random update while playing or with other actions that do not change the current stream info, which would drain battery. E.g. 7 metadata updates instead of 2 when the player is started, and 3 instead of 2 when a new stream info plays. (740ab8a) and most importantly d0677bb
Keep strong reference to Picasso thumbnail loading target. Before the Target would sometimes be garbage collected before being called with the loaded thumbnail, since Picasso holds weak references to targets. Also see Keep strong references to Picasso notification icon loading targets #8677 for a similar change. e9e9e80

You can test changes to the media session by installing this neat media session testing app. Some of the Tests provided there are unsuccessful, but I don't know why since the actions they test are actually performed correctly (I included this sentence just so you know, as fixing the tests, which are related to player actions, is out of the scope of this PR).

This is related to #7166, so I'd like @litetex to review.

Before/After Screenshots/Screen Record

I am not providing screenshots of all of the various issues there were before, since they take a while to reproduce and if you have used NewPipe for a while you know about them by now.

Fixes the following issue(s)

Strangely enough I could not find any... Maybe somebody of you has a clue?

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

I read the contribution guidelines.

litetex

Code LGTM mostly.

but there are build failures and some minor problems:

PS: I will test this when the build works again

litetex · 2022-07-24T12:23:19Z

app/src/main/java/org/schabi/newpipe/player/mediasession/MediaSessionPlayerUi.java

+    @Override
+    public void onBroadcastReceived(final Intent intent) {
+        super.onBroadcastReceived(intent);
+        // TODO decide whether to handle ACTION_HEADSET_PLUG or not


TODO detected

This was already there before, I will not take care of it in this PR. I added it to #8616.

Actually, I don't know. Maybe we should remove this TODO and call it a day, since it has been there since forever and NewPipe's behavior when headsets are plugged or unplugged is currently correct. What do you think?

Then I think we should remove it.

If someone has a problem, we can create an issue and handle it there.

litetex · 2022-07-24T12:26:49Z

app/src/main/java/org/schabi/newpipe/player/mediasession/MediaSessionPlayerUi.java

+        Optional.ofNullable(player.getThumbnail())
+                .filter(bitmap -> showThumbnail)
+                .ifPresent(bitmap -> {
+                    builder.putBitmap(MediaMetadataCompat.METADATA_KEY_ALBUM_ART, bitmap);
+                    builder.putBitmap(MediaMetadataCompat.METADATA_KEY_DISPLAY_ICON, bitmap);
+                });


Wouldn't something like

if (player.getPrefs().getBoolean(context.getString(R.string.show_thumbnail_key), true)) { final BitMap bitmap = player.getThumbnail(); // might be done inline builder.putBitmap(MediaMetadataCompat.METADATA_KEY_ALBUM_ART, bitmap); builder.putBitmap(MediaMetadataCompat.METADATA_KEY_DISPLAY_ICON, bitmap); }

be better here?

Previously the bitmap was set only if it was non null, so I made sure the same behavior is kept, avoiding setting null thumbnails, even though it should not really be a problem.

app/src/main/java/org/schabi/newpipe/player/notification/NotificationUtil.java

app/src/main/java/org/schabi/newpipe/player/mediasession/PlayQueueNavigator.java

Stypox · 2022-07-26T14:38:41Z

I applied two review suggestions and rebased.

there are build failures

Not a problem in this PR, see #8679

opusforlife2 · 2022-07-26T19:46:17Z

new thumbnail would never be shown

wrong seekbar properties (duration and current position) being shown in the notification sometimes

Anything else apart from these 2 issues?

since they take a while to reproduce and if you have used NewPipe for a while you know about them by now.

Well, yes, but we wouldn't know which ones to look for between the current release and this PR.

Gotta test whatever we can, I guess.

opusforlife2 · 2022-07-31T19:16:34Z

In the middle of a video, I suddenly got a buffering indicator for ~half a second, and then the app completely crashed, with no error caught by Newpipe. Scoop's crash report:

FATAL EXCEPTION: main
Process: org.schabi.newpipe.debug.mediasessionui, PID: 3279
java.lang.NullPointerException: Attempt to invoke virtual method 'org.schabi.newpipe.player.ui.PlayerUiList org.schabi.newpipe.player.Player.UIs()' on a null object reference
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment.lambda$addVideoPlayerView$19$org-schabi-newpipe-fragments-detail-VideoDetailFragment(VideoDetailFragment.java:1314)
	at org.schabi.newpipe.fragments.detail.VideoDetailFragment$$ExternalSyntheticLambda9.run(Unknown Source:2)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loop(Looper.java:214)
	at android.app.ActivityThread.main(ActivityThread.java:7356)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:940)

litetex

Code LGTM

Did a quick test and everything seems to work fine.
Profiler also said that there are no bigger performance problems.

However you may address opus comment above before merging.

And btw you may also cleanup your fork-repo, there are a ton of unused branches there which are fetched by Git when I check it out :/

Stypox · 2022-08-04T09:25:13Z

However you may address opus comment above before merging.

@opusforlife2's problem is unrelated to this PR. It is instead another instance of #8661. ~~I'm gonna open yet another PR...~~ Actually, I'm including the fix in #8731.
_{Which problems does your phone have with that little piece of code? 😂}

And btw you may also cleanup your fork-repo, there are a ton of unused branches there which are fetched by Git when I check it out :/

@litetex Ok, I'll do it, I never cared about old branches 😅. But did you know you don't need to check my fork-repo out? You can just add this to your ~/.gitconfig:

[alias]
    pr = "!_() { git fetch upstream pull/\"$1\"/head && git checkout -b pr\"$1\" FETCH_HEAD; }; _"
    pr-f = "!_() { git checkout master; git branch -D pr\"$1\"; git fetch upstream pull/\"$1\"/head && git checkout -b pr\"$1\" FETCH_HEAD ; }; _"

And then you can do git pr 8678 to switch to the branch pr8678 containing this PR's code (pr-f is for force-fetching). If you then want to push to this branch you can do git push git@github.com:Stypox/NewPipe.git pr8678:media-session-ui (a little more cumbersome but still doable fast).

litetex

Rebased and retested, LGTM

Also thank you for the tip Stypox but I will stay at least for now with my
git remote add Stypox https://github.com/Stypox/NewPipe.git and do the rest via GH desktop :D

The player is now passed directly, it made no sense to wrap around it in a callback that was not really a callback but rather, actually, a wrapper.

The thumbnail was not being updated in the media session metadata after it was loaded, since there was no metadata update in that case, only a notification update.

Before the thumbnail finishes loading for the new video the player is now playing, the old thumbnail was being used, leading to wrong thumbnails set in the media session and the notification.

This makes sure the thumbnail is removed from the notification if the user disables thumbnails

A while ago NewPipe called the metadata update function very often, so checks were needed to ensure not wasting time updating metadata if it were already up to date. Now, instead, the metadata update function is called exactly when needed, i.e. when metadata changes, so such checks are not needed anymore (and were probably also a little resource-heavy).

Though still make sure metadata is updated after the thumbnail is loaded. This fixes the wrong seekbar properties (duration and current position) being shown in the notification sometimes.

Before the Target would sometimes be garbage collected before being called with the loaded thumbnail, since Picasso holds weak references to targets

+ Automatically fixed code style (imports)

sonarcloud · 2022-08-25T15:11:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Stypox requested a review from litetex July 22, 2022 14:10

AudricV added bug Issue is related to a bug player notification Anything to do with the MediaStyle notification labels Jul 22, 2022

Stypox added the player Issues related to any player (main, popup and background) label Jul 22, 2022

Stypox mentioned this pull request Jul 22, 2022

Remove Optional method parameters. #8662

Closed

5 tasks

litetex requested changes Jul 24, 2022

View reviewed changes

opusforlife2 mentioned this pull request Jul 25, 2022

[HELP NEEDED] Major planned/missing features #6448

Open

34 tasks

TacoTheDank mentioned this pull request Jul 26, 2022

Migrate ExoPlayer to AndroidX Media3 (incomplete) #8672

Closed

5 tasks

Stypox mentioned this pull request Jul 22, 2022

Player-related code that should be improved #8616

Open

9 tasks

Stypox force-pushed the media-session-ui branch from 096c2f9 to 8d0cea4 Compare July 26, 2022 14:36

Stypox mentioned this pull request Jul 26, 2022

Keep strong references to Picasso notification icon loading targets #8677

Merged

5 tasks

litetex approved these changes Aug 1, 2022

View reviewed changes

Stypox mentioned this pull request Aug 4, 2022

Fix surface view not resizing video correctly + other player fixes #8731

Merged

5 tasks

This comment was marked as off-topic.

Sign in to view

opusforlife2 mentioned this pull request Aug 8, 2022

Tablet layout encountered after full screen video ends, despite Tablet mode being off #8646

Open

5 tasks

Isira-Seneviratne mentioned this pull request Aug 18, 2022

Use picture-in-picture (PIP) for the popup player on Android >= 8.0 #8750

Closed

5 tasks

litetex force-pushed the media-session-ui branch from 8d0cea4 to d402ed7 Compare August 25, 2022 14:39

litetex approved these changes Aug 25, 2022

View reviewed changes

Stypox added 8 commits August 25, 2022 17:00

Create MediaSessionPlayerUi

c054ea0

Remove useless MediaSessionCallback

bc33322

The player is now passed directly, it made no sense to wrap around it in a callback that was not really a callback but rather, actually, a wrapper.

Fix thumbnail sometimes not set to media session metadata

3cc43e9

The thumbnail was not being updated in the media session metadata after it was loaded, since there was no metadata update in that case, only a notification update.

Fix sometimes seeing outdated thumbnail in notification

f3a9b81

Before the thumbnail finishes loading for the new video the player is now playing, the old thumbnail was being used, leading to wrong thumbnails set in the media session and the notification.

Even when thumbnails should not be shown, set it to null in notification

d73ca41

This makes sure the thumbnail is removed from the notification if the user disables thumbnails

Let exoplayer decide when to update metadata

f80d1dc

Though still make sure metadata is updated after the thumbnail is loaded. This fixes the wrong seekbar properties (duration and current position) being shown in the notification sometimes.

Merge MediaSessionManager into MediaSessionPlayerUi

11bd236

Stypox and others added 3 commits August 25, 2022 17:02

Keep strong reference to Picasso thumbnail loading target

510efaa

Before the Target would sometimes be garbage collected before being called with the loaded thumbnail, since Picasso holds weak references to targets

Review suggestions

973a966

Fixed sonar detected problems

59d1ded

+ Automatically fixed code style (imports)

litetex force-pushed the media-session-ui branch from de8dcc9 to 59d1ded Compare August 25, 2022 15:03

litetex merged commit 75917c7 into TeamNewPipe:dev Aug 25, 2022

Stypox deleted the media-session-ui branch August 25, 2022 21:30

Stypox mentioned this pull request Aug 27, 2022

Release v0.24.0 (please TEST!) #8890

Closed

9 tasks

Stypox mentioned this pull request Jan 11, 2023

Crash when going fullscreen (especially Android TVs) #9023

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create media session UI and fix player notification #8678

Create media session UI and fix player notification #8678

Stypox commented Jul 22, 2022 •

edited

Loading

litetex left a comment

litetex Jul 24, 2022

Stypox Jul 26, 2022 •

edited

Loading

Stypox Jul 26, 2022

litetex Aug 1, 2022

litetex Jul 24, 2022

Stypox Jul 26, 2022

Stypox commented Jul 26, 2022

opusforlife2 commented Jul 26, 2022

opusforlife2 commented Jul 31, 2022

litetex left a comment

Stypox commented Aug 4, 2022 •

edited

Loading

This comment was marked as off-topic.

litetex left a comment •

edited by opusforlife2

Loading

sonarcloud bot commented Aug 25, 2022

Create media session UI and fix player notification #8678

Create media session UI and fix player notification #8678

Conversation

Stypox commented Jul 22, 2022 • edited Loading

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

APK testing

Due diligence

litetex left a comment

Choose a reason for hiding this comment

litetex Jul 24, 2022

Choose a reason for hiding this comment

Stypox Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Stypox Jul 26, 2022

Choose a reason for hiding this comment

litetex Aug 1, 2022

Choose a reason for hiding this comment

litetex Jul 24, 2022

Choose a reason for hiding this comment

Stypox Jul 26, 2022

Choose a reason for hiding this comment

Stypox commented Jul 26, 2022

opusforlife2 commented Jul 26, 2022

opusforlife2 commented Jul 31, 2022

litetex left a comment

Choose a reason for hiding this comment

Stypox commented Aug 4, 2022 • edited Loading

This comment was marked as off-topic.

litetex left a comment • edited by opusforlife2 Loading

Choose a reason for hiding this comment

sonarcloud bot commented Aug 25, 2022

Stypox commented Jul 22, 2022 •

edited

Loading

Stypox Jul 26, 2022 •

edited

Loading

Stypox commented Aug 4, 2022 •

edited

Loading

litetex left a comment •

edited by opusforlife2

Loading