Playback resume by Koitharu · Pull Request #1968 · TeamNewPipe/NewPipe

Koitharu · 2018-12-27T14:09:50Z

I carefully read the contribution guidelines and agree to them.

Store playing position in history table in database and resume it. Also show current position in VideoDetailFragment.

Closes #1970

TobiGr

We need someone who can help us with code reviews :D Thanks for implementing this. I have no time to review the code now, but I quickly tested your changes.

I think we might need a setting to turn resume on and off / make it clearer that this is done by turning the watch history off. Maybe some additional words are needed in the summary of the watch history setting. Btw. can you please correct "History & Cache" to "Watch history"?

NewPipe/app/src/main/res/values/strings.xml

Line 94 in e0b8d7c

<string name="enable_watch_history_title">History & Cache</string>

What do you think of showing the player interface instead of the toast? When the users sees that the current position is not at the beginning, they should understand that the last position has been restored. At the same time, they are able to hit the double back icon, to go to 0:00.

How do we want to handle exits which are right before the end? Many YouTube videos have these cards/ end-screens where you can select a recommended video to play next. In NewPipe these links are not shown and these end-cards have no value. I can only speak about my workflow, but I guess many other people hit next as soon this last image appears, too.

Koitharu · 2018-12-27T17:32:42Z

a setting to turn resume on and off

Yes, I will implement it also

showing the player interface instead of the toast

ok 👌

these cards/ end-screens

This feature needs extractor changes. As far as I understand, it needs some time for implementation. I'll think about it.

TobiGr · 2018-12-27T20:37:10Z

This feature needs extractor changes. As far as I understand, it needs some time for implementation. I'll think about it.

Sorry, you got me wrong :) One possible problem could be the saving of the position when you exit the video right before the end (~10 seconds).
Let's say, I stopped watching a video and left 5 seconds before it was finished. When I want to watch this video again, I click on it and have only 5 seconds to realize that I need to jump to the beginning. Otherwise the next video will start.
My question was about how to handle this problem: we can either say, that a video was finished, although there are some seconds left, or store that the user stopped to watch the video and is going to resume it later.

Koitharu · 2018-12-28T05:47:01Z

Ok, good note. Maybe, ignore saved position if left less then 10 seconds

TobiGr · 2018-12-29T14:54:40Z

Just found a little bug: Watch a video for the first time when watch history and resume playback are disabled. The player interface shows the loading spinner the whole time and changing the playback position does not work. I guess the state has not been changed at some point.

Koitharu · 2018-12-30T08:04:37Z

I think, it is done, aren't?)

TobiGr · 2018-12-30T13:34:43Z

I think, it is done, aren't?)

Almost :) When upgrading from 0.14.2 or current dev, I get following crashes at app start:

Exception

User Action: ui error
Request: App crash, UI failure
Content Language: GB
Service: none
Version: 0.14.2
OS: Linux ,release-keys 6.0 - 23

Crash log

java.lang.IllegalStateException: attempt to re-open an already-closed object: SQLiteDatabase: /data/user/0/org.schabi.newpipe.debug/databases/newpipe.db
	at android.database.sqlite.SQLiteClosable.acquireReference(SQLiteClosable.java:55)
	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1312)
	at android.database.sqlite.SQLiteDatabase.rawQueryWithFactory(SQLiteDatabase.java:1291)
	at android.arch.persistence.db.framework.FrameworkSQLiteDatabase.query(FrameworkSQLiteDatabase.java:161)
	at android.arch.persistence.db.framework.FrameworkSQLiteDatabase.query(FrameworkSQLiteDatabase.java:150)
	at android.arch.persistence.room.RoomOpenHelper.hasRoomMasterTable(RoomOpenHelper.java:151)
	at android.arch.persistence.room.RoomOpenHelper.checkIdentity(RoomOpenHelper.java:123)
	at android.arch.persistence.room.RoomOpenHelper.onOpen(RoomOpenHelper.java:115)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onOpen(FrameworkSQLiteOpenHelper.java:151)
	at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:266)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:163)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableSupportDatabase(FrameworkSQLiteOpenHelper.java:96)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.java:54)
	at android.arch.persistence.room.RoomDatabase.query(RoomDatabase.java:233)
	at org.schabi.newpipe.database.playlist.dao.PlaylistRemoteDAO_Impl$6.call(PlaylistRemoteDAO_Impl.java:274)
	at org.schabi.newpipe.database.playlist.dao.PlaylistRemoteDAO_Impl$6.call(PlaylistRemoteDAO_Impl.java:271)
	at android.arch.persistence.room.RxRoom$4.apply(RxRoom.java:111)
	at android.arch.persistence.room.RxRoom$4.apply(RxRoom.java:108)
	at io.reactivex.internal.operators.flowable.FlowableMap$MapConditionalSubscriber.tryOnNext(FlowableMap.java:123)
	at io.reactivex.internal.operators.flowable.FlowableObserveOn$ObserveOnConditionalSubscriber.runAsync(FlowableObserveOn.java:649)
	at io.reactivex.internal.operators.flowable.FlowableObserveOn$BaseObserveOnSubscriber.run(FlowableObserveOn.java:176)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
	at java.lang.Thread.run(Thread.java:818)

Exception

User Action: something
Request: Subscriptions
Content Language: GB
Service: none
Version: 0.14.2
OS: Linux ,release-keys 6.0 - 23

Crash log

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.
	at android.arch.persistence.room.RoomOpenHelper.checkIdentity(RoomOpenHelper.java:135)
	at android.arch.persistence.room.RoomOpenHelper.onOpen(RoomOpenHelper.java:115)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onOpen(FrameworkSQLiteOpenHelper.java:151)
	at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:266)
	at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:163)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableSupportDatabase(FrameworkSQLiteOpenHelper.java:96)
	at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.java:54)
	at android.arch.persistence.room.RoomDatabase.query(RoomDatabase.java:233)
	at org.schabi.newpipe.database.subscription.SubscriptionDAO_Impl$6.call(SubscriptionDAO_Impl.java:299)
	at org.schabi.newpipe.database.subscription.SubscriptionDAO_Impl$6.call(SubscriptionDAO_Impl.java:296)
	at android.arch.persistence.room.RxRoom$4.apply(RxRoom.java:111)
	at android.arch.persistence.room.RxRoom$4.apply(RxRoom.java:108)
	at io.reactivex.internal.operators.flowable.FlowableMap$MapConditionalSubscriber.tryOnNext(FlowableMap.java:123)
	at io.reactivex.internal.operators.flowable.FlowableObserveOn$ObserveOnConditionalSubscriber.runAsync(FlowableObserveOn.java:649)
	at io.reactivex.internal.operators.flowable.FlowableObserveOn$BaseObserveOnSubscriber.run(FlowableObserveOn.java:176)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
	at java.lang.Thread.run(Thread.java:818)

Koitharu · 2018-12-31T05:58:01Z

Hm

karyogamy · 2019-01-01T14:36:03Z

Hi @nv95. Thank you for you work.

I just want to quickly mention the playback position saving and loading is already available, so there is no need to update the database and create another migration. This is done on a separate StreamState table, so the stream metadata and user-generated data for streams can be kept and updated separately.

Koitharu · 2019-01-01T16:56:05Z

It is unused now, and I don't understand how to use StreamState. Besides my solution is simple and ready to use now

TobiGr · 2019-01-08T20:00:38Z

@karyogamy welcome back! It's good to hear from you. Do you have time to review the code further?
@nv95 I agree with @karyogamy that using the StreamState table might be a better approach. Unfortunately, I can't help you with that until the semester is over and my exams written.

@karyogamy @theScrabi It might be good to have a model of our db in the wiki, so devs can work with it easier. What do you think?

Koitharu · 2019-01-10T07:40:20Z

Ok?

theScrabi · 2019-02-19T14:03:21Z

@karyogamy @theScrabi It might be good to have a model of our db in the wiki, so devs can work with it easier. What do you think?

Sounds good.

theScrabi · 2019-02-19T15:06:49Z

Documented the database. See here:
#2136

The files need to be enroled in a wiki/documentation though.

app/src/main/java/org/schabi/newpipe/util/Constants.java

app/src/main/res/xml/history_settings.xml

TobiGr · 2019-03-07T14:55:36Z

Thanks for fixing that. One minor thing is left:

When resuming or re-starting and finishing a video/stream, we should reset the last playback position instead of doing nothing

I think we should do the same with pop-up and background player.

Koitharu · 2019-03-07T15:43:42Z

with pop-up and background player

I think it must works in any player

TobiGr · 2019-03-07T17:20:18Z

I tested and it didn't work for some reason. This is what I did:

open a video and play it with the main player
switch to background and remember the current playback position
let the stream finish and wait until the next element in the queue starts to play
open the video detail page of the stream which just finished. Notice the time for playback resume is the same at which we switched to background.

…yback_resume

TobiGr

There are some problems with the popup player:

start a video in popup mode by clicking on a timestep-link in a video description or comment
Notice the video repeating the first miliseconds again and again until some more of the video stream is buffered.
Let the video play for some seconds
Pause the video. Notice the current playback position is reset back to the point from where we started in 1.

In some cases this problem is even bigger.

start a video in popup mode by clicking on a timestep-link in a video description or comment
start another video in popup mode by clicking on a timestep-link in a video description or comment
let the first video finish or skip it (to skip it, click on the popup notification and then press the control to get to the next video
You'll be in an endless loop of the first frames. The only way the interrupt the loop is pressing the pause button. after pressing play again, the video is played properly.

open a video and play it with the main player

switch to background and remember the current playback position

let the stream finish and wait until the next element in the queue starts to play

open the video detail page of the stream which just finished. Notice the time for playback resume is the same at which we switched to background.

Unfortunately, this is not resolved yet.

We might need to discuss the correct error handling. First of all, we should only log when we have a debug build. Apart from that, we can create a bug report and display it to the user. @theScrabi Can you please take a look at the lines below and tell us how you would handle the errors?

TobiGr · 2019-03-11T16:03:14Z

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

+                            }
+                        },
+                        error -> {
+                            Log.e(TAG, "Player resume failure: ", error);


Suggested change

Log.e(TAG, "Player resume failure: ", error);

if (DEBUG) Log.e(TAG, "Player resume failure: ", error);

TobiGr · 2019-03-11T16:03:26Z

app/src/main/java/org/schabi/newpipe/player/BasePlayer.java

+                                changeState(playWhenReady ? STATE_PLAYING : STATE_PAUSED);
+                            },
+                            error -> {
+                                Log.e(TAG, "Player resume failure: ", error);


Suggested change

Log.e(TAG, "Player resume failure: ", error);

if (DEBUG) Log.e(TAG, "Player resume failure: ", error);

TobiGr · 2019-03-11T16:03:33Z

app/src/main/java/org/schabi/newpipe/player/BasePlayer.java

-    protected void savePlaybackState(final StreamInfo info, final long progress) {
+    private void savePlaybackState(final StreamInfo info, final long progress) {
        if (info == null) return;
+        Log.d(TAG, "savePlaybackState() called");


Suggested change

Log.d(TAG, "savePlaybackState() called");

if (DEBUG) Log.d(TAG, "savePlaybackState() called");

TobiGr · 2019-03-11T16:03:34Z

app/src/main/java/org/schabi/newpipe/player/BasePlayer.java

-    private void savePlaybackState() {
+    private void resetPlaybackState(final StreamInfo info) {
+        if (info == null) return;
+        Log.d(TAG, "resetPlaybackState() called");


Suggested change

Log.d(TAG, "resetPlaybackState() called");

if (DEBUG) Log.d(TAG, "resetPlaybackState() called");

TobiGr · 2019-03-11T16:03:46Z

app/src/main/java/org/schabi/newpipe/player/BasePlayer.java

+                .onErrorComplete()
+                .subscribe(
+                        ignored -> {/* successful */},
+                        error -> Log.e(TAG, "resetPlaybackState() failure: ", error)


Koitharu · 2019-03-11T17:32:35Z

I have some troubles with understanding, where and when is rationally to save playback state. Also I think, that it is undesirable to call savePlaybackState() from main/popup/background player activities or services, aren`t? So, give me some time

Koitharu · 2019-03-12T08:13:59Z

by clicking on a timestep-link

this bug is not related to the saving playback state

Koitharu · 2019-03-12T08:46:45Z

@yausername I found a trouble with CommentTextOnTouchListener.playOnPopup. You remove start position parameter from the url and put info into cache. As a result, after opening a timestamp link we have a StreamInfo with non-zero startPosition in cache, but its key does not contains timestamp information due to
cleanUrl = factory.getUrl(factory.getId(url));
Are we really need this line?

TobiGr · 2019-03-12T12:39:45Z

So, give me some time

Sure. We all want to have the best result in the end. If this takes a bit longer - no problem :)

this bug is not related to the saving playback state

That's good to know. I didn't test the latest dev and compared the bugs of this PR with the dev branch.

yausername · 2019-03-13T01:45:58Z

cleanUrl = factory.getUrl(factory.getId(url));
Are we really need this line?

@nv95 I wanted to use cached value of streamInfo. If I do not remove the position parameter from url then it fetches streamInfo again from server.
Anyway, a fix is available here #2207
Let me know if this is ok.

Koitharu · 2019-03-13T09:08:59Z

@yausername your patch is not compatible with this pr.

I propose to merge #2207 first, then I will complete this.

yausername · 2019-04-03T17:19:38Z

I think this got closed due to github's automatic trigger. @nv95 @TobiGr please reopen.

TobiGr · 2019-04-03T17:21:12Z

Oh 😅

TobiGr · 2019-04-11T17:24:54Z

@nv95 I'll do one last round of testing. Is there anything you intend to fix or change or have you completed your work on this?

Koitharu · 2019-04-11T18:36:28Z

@TobiGr Sorry, I have not time just now, but I will do it whatever. Give me some time 👌

playest · 2019-07-30T05:34:19Z

Any news about when this feature will be implemented?

Koitharu · 2019-07-30T06:12:14Z

@playest already merged in #2288

Koitharu added 4 commits December 27, 2018 12:04

Saving streams positions and resume it

fd7a669

Show watch position on VideoDetailFragment

7fb6274

Database migration

2d1e5ab

Show toast on playback resumed

e0b8d7c

TobiGr reviewed Dec 27, 2018

View reviewed changes

Koitharu added 3 commits December 29, 2018 11:16

Dont resume playback if less then 10 seconds left

000b7b7

Playback resume preference

b86c420

Show player interface on playback resumed

83fcb1a

TobiGr mentioned this pull request Dec 29, 2018

Feature Request : Resume to last playback position #1970

Closed

Fix bug with player state

fafbd59

Increase database version

d01158b

Use StreamState

31300e0

Merge branch 'dev' into playback_resume

982d427

TobiGr reviewed Feb 27, 2019

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/Constants.java Outdated Show resolved Hide resolved

Koitharu added 2 commits February 27, 2019 18:37

Refactor: threshold constant

40570f7

Merge branch 'dev' into playback_resume

1c11b36

TobiGr reviewed Mar 5, 2019

View reviewed changes

app/src/main/res/xml/history_settings.xml Outdated Show resolved Hide resolved

Fix settings layout

3cbc84d

TobiGr mentioned this pull request Mar 10, 2019

Update ExoPlayer to 2.9.6 #2184

Merged

1 task

Koitharu added 2 commits March 11, 2019 13:43

Merge branch 'dev' of https://github.com/TeamNewPipe/NewPipe into pla…

93e6eef

…yback_resume

Reset playback state on playing complete

b73d3c7

TobiGr reviewed Mar 11, 2019

View reviewed changes

Fix logging

06cf59f

Fix playback state saving again

393cea9

Fix trouble with background player

ca103ae

yausername mentioned this pull request Mar 13, 2019

seek on clicking timestamp links in comments #2207

Merged

1 task

TobiGr closed this in #2207 Apr 3, 2019

TobiGr reopened this Apr 3, 2019

TobiGr added 2 commits April 8, 2019 13:19

Merge branch 'dev' into playback_resume

d3153d9

Merge branch 'dev' into playback_resume

41a9b44

Koitharu mentioned this pull request Apr 13, 2019

Playback resume v2 #2286

Closed

1 task

Koitharu closed this Apr 13, 2019

	Log.e(TAG, "Player resume failure: ", error);
	if (DEBUG) Log.e(TAG, "Player resume failure: ", error);

	Log.d(TAG, "savePlaybackState() called");
	if (DEBUG) Log.d(TAG, "savePlaybackState() called");

	Log.d(TAG, "resetPlaybackState() called");
	if (DEBUG) Log.d(TAG, "resetPlaybackState() called");

Uh oh!

Conversation

Koitharu commented Dec 27, 2018 • edited by TobiGr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiGr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Koitharu commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiGr commented Dec 27, 2018

Uh oh!

Koitharu commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiGr commented Dec 29, 2018

Uh oh!

Koitharu commented Dec 30, 2018

Uh oh!

TobiGr commented Dec 30, 2018

Exception

Exception

Uh oh!

Koitharu commented Dec 31, 2018

Uh oh!

karyogamy commented Jan 1, 2019

Uh oh!

Koitharu commented Jan 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiGr commented Jan 8, 2019

Uh oh!

Koitharu commented Jan 10, 2019

Uh oh!

theScrabi commented Feb 19, 2019

Uh oh!

theScrabi commented Feb 19, 2019

Uh oh!

Uh oh!

Uh oh!

TobiGr commented Mar 7, 2019

Uh oh!

Koitharu commented Mar 7, 2019

Uh oh!

TobiGr commented Mar 7, 2019 • edited by theScrabi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiGr left a comment

Choose a reason for hiding this comment

Uh oh!

TobiGr Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

TobiGr Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

TobiGr Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

TobiGr Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

TobiGr Mar 11, 2019

Choose a reason for hiding this comment

Uh oh!

Koitharu commented Mar 11, 2019

Uh oh!

Koitharu commented Mar 12, 2019

Uh oh!

Koitharu commented Mar 12, 2019

Uh oh!

TobiGr commented Mar 12, 2019

Uh oh!

yausername commented Mar 13, 2019

Uh oh!

Koitharu commented Mar 13, 2019

Uh oh!

yausername commented Apr 3, 2019

Koitharu commented Dec 27, 2018 •

edited by TobiGr

Loading

TobiGr left a comment •

edited

Loading

Koitharu commented Dec 27, 2018 •

edited

Loading

Koitharu commented Dec 28, 2018 •

edited

Loading

Koitharu commented Jan 1, 2019 •

edited

Loading

TobiGr commented Mar 7, 2019 •

edited by theScrabi

Loading