Skip to content

Allow a BasePlayer to start paused#2917

Merged
TobiGr merged 3 commits intoTeamNewPipe:devfrom
raphj:patch-1
Jan 26, 2020
Merged

Allow a BasePlayer to start paused#2917
TobiGr merged 3 commits intoTeamNewPipe:devfrom
raphj:patch-1

Conversation

@raphj
Copy link
Contributor

@raphj raphj commented Jan 1, 2020

This commit fixes the situation where NewPipe is set up to play in the background when minimizing its window.

  1. A video is playing
  2. The video is paused
  3. NewPipe is minimized (of the screen is turned of)

In this situation, without this pull request, the video is incorrectly resumed in the background.

I hesitated whether I would set the default value to true of false. I chose to pick true to be closer to the current behavior, please let me know if it would be better to use false or fill free to edit.

[X] I carefully read the contribution guidelines and agree to them.

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Jan 1, 2020
@Stypox
Copy link
Member

Stypox commented Jan 5, 2020

Could you provide an apk to test?

@raphj
Copy link
Contributor Author

raphj commented Jan 5, 2020

app-debug.zip
Please rename as .apk

@Stypox
Copy link
Member

Stypox commented Jan 6, 2020

There is something wrong with the apk... I have "Minimize on app switch to background player" active and when I switch away from a playing video in the main player, I get a strange-looking background player notification (i.e. the progress bar is missing).

@raphj
Copy link
Contributor Author

raphj commented Jan 6, 2020

Ok, I'm seeing another bug as well that I didn't encounter during my tests: Playing a video and then minimizing does not resume playing. I'm going to look into it.

@TobiGr TobiGr added this to the 0.18.2 milestone Jan 13, 2020
@raphj
Copy link
Contributor Author

raphj commented Jan 15, 2020

I am investigating.

On the version published on F-Droid, I am seeing this exception when playing a fullscreen video, and then clicking on the background button:

01-15 17:01:19.490  5954  5954 W MessageQueue: Handler (android.os.Handler) {5830631} sending message to a Handler on a dead thread
01-15 17:01:19.490  5954  5954 W MessageQueue: java.lang.IllegalStateException: Handler (android.os.Handler) {5830631} sending message to a Handler on a dead thread
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.MessageQueue.enqueueMessage(MessageQueue.java:546)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Handler.enqueueMessage(Handler.java:745)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Handler.sendMessageAtTime(Handler.java:697)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Handler.sendMessageDelayed(Handler.java:667)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Handler.sendMessage(Handler.java:604)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Message.sendToTarget(Message.java:436)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.google.android.exoplayer2.ExoPlayerImplInternal.stop(ExoPlayerImplInternal.java:208)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.google.android.exoplayer2.ExoPlayerImpl.stop(ExoPlayerImpl.java:414)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.google.android.exoplayer2.SimpleExoPlayer.stop(SimpleExoPlayer.java:1006)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.google.android.exoplayer2.BasePlayer.stop(BasePlayer.java:80)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at org.schabi.newpipe.player.BasePlayer.destroyPlayer(BasePlayer.java:334)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at org.schabi.newpipe.player.BasePlayer.destroy(BasePlayer.java:352)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at org.schabi.newpipe.player.VideoPlayer.destroy(VideoPlayer.java:587)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at org.schabi.newpipe.player.MainVideoPlayer.onStop(MainVideoPlayer.java:261)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.Instrumentation.callActivityOnStop(Instrumentation.java:1432)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.Activity.performStop(Activity.java:7375)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.ActivityThread.callActivityOnStop(ActivityThread.java:4181)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.ActivityThread.performStopActivityInner(ActivityThread.java:4159)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.ActivityThread.handleStopActivity(ActivityThread.java:4234)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.servertransaction.TransactionExecutor.performLifecycleSequence(TransactionExecutor.java:192)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.servertransaction.TransactionExecutor.cycleToPath(TransactionExecutor.java:165)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.servertransaction.TransactionExecutor.executeLifecycleState(TransactionExecutor.java:142)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:70)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1816)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Handler.dispatchMessage(Handler.java:106)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.os.Looper.loop(Looper.java:193)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at android.app.ActivityThread.main(ActivityThread.java:6718)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at java.lang.reflect.Method.invoke(Native Method)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
01-15 17:01:19.490  5954  5954 W MessageQueue:  at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

@raphj
Copy link
Contributor Author

raphj commented Jan 16, 2020

All right so I didn't understand well the meaning of the RESUME_PLAYBACK field. There are two places it is set to true across the code base, and it is in fragments/detail/VideoDetailFragment. I should not touch this for this pull request.

It seems to me that a new field in the intent is needed: isPaused. I am going to experiment with this, but feel free to stop me if it does not seems to be a good solution.

This allows fixing spurious playback resume when minimizing to the background player.
@raphj
Copy link
Contributor Author

raphj commented Jan 16, 2020

I think I got it fixed.

I tested switching:

  • from the main player to the background player, while playing and while paused
  • from the main player to the popup player, while playing and while paused
  • from the popup player to the main player, while playing and while paused
  • from the background player to the main player, while playing and while paused
  • from the background player to the popup player, while playing and while paused

I don't know how to switch from the popup player to the background player.

To be completely honest, I did spot situations where when playing in the main player and switching to the background player, playback is not resumed, but I didn't find a way to replicate this and I am not sure the issue comes from my code.

I think the fields of the intent need to be documented, though I would need help for this.

@Stypox does it look good to you?

app-debug.zip

@raphj raphj changed the title Respect RESUME_PLAYBACK when creating a BasePlayer Allow a BasePlayer to start paused Jan 17, 2020
@raphj
Copy link
Contributor Author

raphj commented Jan 17, 2020

We will probably have to rebase this on top of #2907.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thanks!
You do not only fix a bug, but also improve code quality. Nice job!

@TobiGr TobiGr merged commit 609855f into TeamNewPipe:dev Jan 26, 2020
@raphj raphj deleted the patch-1 branch January 26, 2020 17:17
This was referenced Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

player Issues related to any player (main, popup and background)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants