Load full stream info when enqueuing a stream #7036

Douile · 2021-09-01T00:04:14Z

Please feel free to criticize the code here, I am not sure if I am using workers correctly or if there is a better way to do this. I essentially copied the code from

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

Lines 878 to 906 in faa7a91

    
           private void runWorker(final boolean forceLoad, final boolean addToBackStack) { 
        
               final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(activity); 
        
               currentWorker = ExtractorHelper.getStreamInfo(serviceId, url, forceLoad) 
        
                       .subscribeOn(Schedulers.io()) 
        
                       .observeOn(AndroidSchedulers.mainThread()) 
        
                       .subscribe(result -> { 
        
                           isLoading.set(false); 
        
                           hideMainPlayer(); 
        
                           if (result.getAgeLimit() != NO_AGE_LIMIT && !prefs.getBoolean( 
        
                                   getString(R.string.show_age_restricted_content), false)) { 
        
                               hideAgeRestrictedContent(); 
        
                           } else { 
        
                               handleResult(result); 
        
                               showContent(); 
        
                               if (addToBackStack) { 
        
                                   if (playQueue == null) { 
        
                                       playQueue = new SinglePlayQueue(result); 
        
                                   } 
        
                                   if (stack.isEmpty() || !stack.peek().getPlayQueue().equals(playQueue)) { 
        
                                       stack.push(new StackItem(serviceId, url, title, playQueue)); 
        
                                   } 
        
                               } 
        
                               if (isAutoplayEnabled()) { 
        
                                   openVideoPlayer(); 
        
                               } 
        
                           } 
        
                       }, throwable -> showError(new ErrorInfo(throwable, UserAction.REQUESTED_STREAM, 
        
                               url == null ? "no url" : url, serviceId))); 
        
           }

I decided loading the information when adding to the queue is better than loading when the video starts playing as it allows the queue progress text to be correct as well.

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 calls getStreamInfo causing a full network fetch of stream info (I believe only if required) when adding a stream item to the queue. This should prevent UI issues of missing metadata when queuing videos that have been fast-loaded and are missing metadata.

Fixes the following issue(s)

Fixes Queued videos metadata not properly updating #7035

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.

ktprograms · 2021-09-01T03:06:34Z

I think how you're doing it now will always cause a full network fetch. The best way would probably be to check if (for example) the item.getDuration() is null, and only then doing the fetch. If it isn't null, you can just keep the old code (so have an if / else statement).

Here's the before and after of a similar case of adding the network fetch I recently implemented:

Before:

NewPipe/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

Lines 32 to 37 in 2b41f71

    
           show_channel_details(R.string.show_channel_details, (fragment, item) -> 
        
               // For some reason `getParentFragmentManager()` doesn't work, but this does. 
        
               NavigationHelper.openChannelFragment( 
        
                       fragment.requireActivity().getSupportFragmentManager(), 
        
                       item.getServiceId(), item.getUploaderUrl(), item.getUploaderName()) 
        
           ),

After:

NewPipe/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

Lines 36 to 59 in faa7a91

    
           show_channel_details(R.string.show_channel_details, (fragment, item) -> { 
        
               if (isNullOrEmpty(item.getUploaderUrl())) { 
        
                   final int serviceId = item.getServiceId(); 
        
                   final String url = item.getUrl(); 
        
                   Toast.makeText(fragment.getContext(), R.string.loading_channel_details, 
        
                           Toast.LENGTH_SHORT).show(); 
        
                   ExtractorHelper.getStreamInfo(serviceId, url, false) 
        
                           .subscribeOn(Schedulers.io()) 
        
                           .observeOn(AndroidSchedulers.mainThread()) 
        
                           .subscribe(result -> { 
        
                               NewPipeDatabase.getInstance(fragment.getContext()).streamDAO() 
        
                                       .setUploaderUrl(serviceId, url, result.getUploaderUrl()) 
        
                                       .subscribeOn(Schedulers.io()).subscribe(); 
        
                               openChannelFragment(fragment, item, result.getUploaderUrl()); 
        
                           }, throwable -> Toast.makeText( 
        
                                   // TODO: Open the Error Activity 
        
                                   fragment.getContext(), 
        
                                   R.string.error_show_channel_details, 
        
                                   Toast.LENGTH_SHORT 
        
                           ).show()); 
        
               } else { 
        
                   openChannelFragment(fragment, item, item.getUploaderUrl()); 
        
               } 
        
           }),

(In the after, openChannelFragment is a helper method that calls NavigationHelper.openChannelFragment):

NewPipe/app/src/main/java/org/schabi/newpipe/util/StreamDialogEntry.java

Lines 209 to 216 in faa7a91

    
           private static void openChannelFragment(final Fragment fragment, 
        
                                                   final StreamInfoItem item, 
        
                                                   final String uploaderUrl) { 
        
               // For some reason `getParentFragmentManager()` doesn't work, but this does. 
        
               NavigationHelper.openChannelFragment( 
        
                       fragment.requireActivity().getSupportFragmentManager(), 
        
                       item.getServiceId(), uploaderUrl, item.getUploaderName()); 
        
           }

GneHHHc233 · 2021-09-01T17:18:25Z

Hey, brother, thank you for solving the problem that my device YouTube can't get

Stypox · 2021-09-01T18:51:46Z

The best way would probably be to check if (for example) the item.getDuration() is null, and only then doing the fetch. If it isn't null, you can just keep the old code (so have an if / else statement).

Yes, good idea. The stream info would anyway be fetched when the stream is loaded and played, but not fetching it beforehand is a good idea nonetheless.

Douile · 2021-09-01T18:55:44Z

getStreamInfo does check the cache but it is simple enough to check whether we need to call it so I will add that check.

Douile · 2021-09-01T19:04:30Z

Changes made in #6872 will conflict with this PR so I will wait for it to be merged before continuing here.

Stypox · 2021-09-01T19:08:55Z

getStreamInfo does check the cache

There are "three" "stages" of stream loading:

stream info item loaded with fast feed, with some metadata fields, thus missing duration field
stream info item loaded with any other method (search, trending, ... including slow feed), with all metadata fields
stream info with all of the data needed to play

Stream info items are those loaded alongside other stream info items (i.e. when fetching a list, e.g. search results, trending, feed, ...), while stream info is loaded e.g. when a video is opened or played.

If when calling getStreamInfo the 3rd type of stream loading was already done before, then the cache kicks in, otherwise it doesn't, since getStreamInfo aims to collect all of the data needed to play a video. So preventing an early call to getStreamInfo would be nice, as the cache would not kick in for most stream info items, unless they have already been opened.

Stypox · 2021-09-01T19:10:36Z

_{GitHub added some keybinding to fast close PRs, and I seem to accidentally keep to press it}

Changes made in #6872 will conflict with this PR so I will wait for it to be merged before continuing here.

That's not a problem, git rebase is your friend ;-) (and I can help if needed)

This commit calls getStreamInfo causing a full network fetch of stream info (I believe only if required) when adding a stream item to the queue. This should prevent UI issues of missing metadata when queueing videos that have been fast-loaded and are missing metadata. Fixes TeamNewPipe#7035

Douile · 2021-09-18T23:18:13Z

When new info is fetched the feed item still doesn't update, I'm not sure where to trigger this update.

Douile · 2021-09-19T04:34:04Z

Ayyy found it, https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java#L975

Douile · 2021-09-20T13:59:25Z

Nvm that was the wrong thing...

Douile · 2021-10-28T00:13:28Z

Working now :)

Douile · 2021-11-04T16:49:29Z

lmk if this needs a refactor because I'm not sure if it the best way of doing.

Stypox

I can confirm it works when enqueueing a stream whose duration is unknown, thank you!

I get this crash when I long press an item in the fast feed, press on "Start playing in the background" and just after that I long press on another item. (both items are without duration before long pressing).

Exception

User Action: ui error
Request: ACRA report
Content Language: en
Service: none
Version: 0.21.13
OS: Linux Android 10 - 29

Crash log

java.lang.NullPointerException: Attempt to invoke virtual method 'int org.schabi.newpipe.player.playqueue.PlayQueue.size()' on a null object reference
	at org.schabi.newpipe.player.helper.PlayerHolder.getQueueSize(PlayerHolder.java:74)
	at org.schabi.newpipe.local.feed.FeedFragment.showStreamDialog(FeedFragment.kt:336)
	at org.schabi.newpipe.local.feed.FeedFragment.access$showStreamDialog(FeedFragment.kt:80)
	at org.schabi.newpipe.local.feed.FeedFragment$listenerStreamItem$1.onItemLongClick(FeedFragment.kt:395)
	at com.xwray.groupie.GroupieViewHolder$2.onLongClick(GroupieViewHolder.java:32)
	at android.view.View.performLongClickInternal(View.java:7339)
	at android.view.View.performLongClick(View.java:7297)
	at android.view.View.performLongClick(View.java:7315)
	at android.view.View$CheckForLongPress.run(View.java:27850)
	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)

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

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

Stypox · 2021-11-21T12:29:45Z

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

    enqueue(R.string.enqueue_stream, (fragment, item) -> {
-        NavigationHelper.enqueueOnPlayer(fragment.getContext(), new SinglePlayQueue(item));
+        fetchItemInfoIfSparse(fragment, item,
+                fullItem -> NavigationHelper.enqueueOnPlayer(fragment.getContext(), fullItem)
+        );
    }),

    enqueue_next(R.string.enqueue_next_stream, (fragment, item) -> {
-        NavigationHelper.enqueueNextOnPlayer(fragment.getContext(), new SinglePlayQueue(item));
+        fetchItemInfoIfSparse(fragment, item,
+                fullItem -> NavigationHelper.enqueueNextOnPlayer(fragment.getContext(), fullItem)
+        );
    }),


Why did you only do it for enqueue? I also get the problem of the missing duration field in the play queue when I press on "Start playing on ..."

Ah, for some reason I neglected to check those options :)

Co-authored-by: Stypox <stypox@pm.me>

Stypox · 2021-12-07T15:58:49Z

In order to fix the exception I posted above I think you just need to alter the PlayerHolder.getQueueSize() method to something along these lines

if (!isPlayerOpen()) return 0;
final PlayQueue playQueue = player.getPlayQueue();
return playQueue == null ? 0 : playQueue.size();

Stypox

Code now looks good, the formatting just needs to be fixed

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

Co-authored-by: Stypox <stypox@pm.me>

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

Co-authored-by: Stypox <stypox@pm.me> Replace the unecessary callback interface InfoCallback in favour of the standard type Consumer<SinglePlayQueue>

sonarcloud · 2022-01-03T18:03:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

Stypox

Works perfectly, thank you!

TobiGr

While rebasing #7570 I ran into a few smaller things which I would like to get an answer / opinion on.

TobiGr · 2022-01-15T22:07:56Z

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

+    // helper functions                        //
+    /////////////////////////////////////////////
+
+    private static void fetchItemInfoIfSparse(final Fragment fragment,


Why is the fragment used as param here? The only usage for fragment is fragment.getContext() in this method.

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

TobiGr · 2022-01-15T22:12:14Z

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

+                                .doOnError(throwable -> Log.e("StreamDialogEntry",
+                                        throwable.toString()))
+                                .subscribe();
+
+                        callback.accept(new SinglePlayQueue(result));
+                    }, throwable -> Log.e("StreamDialogEntry", throwable.toString()));


I think we should report these error via the UI. Otherwise the user gets no feedback on his action if something fails. However, I am not sure how to handle network related errors here. @Stypox Any idea?

Maybe we toast the user and fallback to returning the sparse info item then, although that still has its issues.

You could use an error notification, just like https://github.com/TeamNewPipe/NewPipe/pull/7538/files#diff-212f79b4111281cdfbffed5f79bca8922f99be14642ef90d524e6c9de9a436f7R66

Stypox closed this Sep 1, 2021

Stypox reopened this Sep 1, 2021

Douile force-pushed the fix/queue-metadata branch from 54d8f2e to 62d3612 Compare September 18, 2021 23:16

Update stream state in database after loading

bc2f0f9

Stypox requested changes Nov 21, 2021

View reviewed changes

Douile and others added 5 commits November 23, 2021 15:22

Don't fetch uneeded stream info for live streams

91611fc

Co-authored-by: Stypox <stypox@pm.me>

Remove unused code

6472e9b

Remove unused variable

3d1a360

Co-authored-by: Stypox <stypox@pm.me>

Fix StreamType check, missing import, and styling errors

ec7de2a

Fetch sparse items when playing in background or popup

7cd3603

Stypox requested changes Dec 7, 2021

View reviewed changes

Douile and others added 2 commits December 12, 2021 12:51

Remove unecessary line

baee915

Co-authored-by: Stypox <stypox@pm.me>

Fix lambda code formatting

3ff00ff

Co-authored-by: Stypox <stypox@pm.me>

Douile force-pushed the fix/queue-metadata branch from cbe986e to 3ff00ff Compare December 12, 2021 13:04

Douile requested a review from Stypox December 21, 2021 18:56

Stypox requested changes Jan 3, 2022

View reviewed changes

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

Remove unecessary interface InfoCallback

064242d

Co-authored-by: Stypox <stypox@pm.me> Replace the unecessary callback interface InfoCallback in favour of the standard type Consumer<SinglePlayQueue>

Stypox approved these changes Jan 7, 2022

View reviewed changes

Stypox merged commit 7907182 into TeamNewPipe:dev Jan 7, 2022

TobiGr reviewed Jan 15, 2022

View reviewed changes

Douile mentioned this pull request Feb 4, 2022

#7036: Fix codestyle issues #7808

Closed

5 tasks

This was referenced Feb 11, 2022

Release 0.22.0 #7857

Closed

Release 0.22.0 #7858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load full stream info when enqueuing a stream #7036

Load full stream info when enqueuing a stream #7036

Douile commented Sep 1, 2021

ktprograms commented Sep 1, 2021

GneHHHc233 commented Sep 1, 2021

Stypox commented Sep 1, 2021

Douile commented Sep 1, 2021

Douile commented Sep 1, 2021

Stypox commented Sep 1, 2021

Stypox commented Sep 1, 2021

Douile commented Sep 18, 2021

Douile commented Sep 19, 2021 •

edited

Loading

Douile commented Sep 20, 2021

Douile commented Oct 28, 2021

Douile commented Nov 4, 2021

Stypox left a comment

Stypox Nov 21, 2021

Douile Nov 23, 2021

Stypox commented Dec 7, 2021

Stypox left a comment

sonarcloud bot commented Jan 3, 2022

Stypox left a comment

TobiGr left a comment

TobiGr Jan 15, 2022

TobiGr Jan 15, 2022

Douile Jan 15, 2022

Stypox Jan 24, 2022

	private void runWorker(final boolean forceLoad, final boolean addToBackStack) {
	final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(activity);
	currentWorker = ExtractorHelper.getStreamInfo(serviceId, url, forceLoad)
	.subscribeOn(Schedulers.io())
	.observeOn(AndroidSchedulers.mainThread())
	.subscribe(result -> {
	isLoading.set(false);
	hideMainPlayer();
	if (result.getAgeLimit() != NO_AGE_LIMIT && !prefs.getBoolean(
	getString(R.string.show_age_restricted_content), false)) {
	hideAgeRestrictedContent();
	} else {
	handleResult(result);
	showContent();
	if (addToBackStack) {
	if (playQueue == null) {
	playQueue = new SinglePlayQueue(result);
	}
	if (stack.isEmpty() \|\| !stack.peek().getPlayQueue().equals(playQueue)) {
	stack.push(new StackItem(serviceId, url, title, playQueue));
	}
	}
	if (isAutoplayEnabled()) {
	openVideoPlayer();
	}
	}
	}, throwable -> showError(new ErrorInfo(throwable, UserAction.REQUESTED_STREAM,
	url == null ? "no url" : url, serviceId)));
	}

Load full stream info when enqueuing a stream #7036

Load full stream info when enqueuing a stream #7036

Conversation

Douile commented Sep 1, 2021

What is it?

Description of the changes in your PR

Fixes the following issue(s)

APK testing

Due diligence

ktprograms commented Sep 1, 2021

Before:

After:

GneHHHc233 commented Sep 1, 2021

Stypox commented Sep 1, 2021

Douile commented Sep 1, 2021

Douile commented Sep 1, 2021

Stypox commented Sep 1, 2021

Stypox commented Sep 1, 2021

Douile commented Sep 18, 2021

Douile commented Sep 19, 2021 • edited Loading

Douile commented Sep 20, 2021

Douile commented Oct 28, 2021

Douile commented Nov 4, 2021

Stypox left a comment

Choose a reason for hiding this comment

Exception

Stypox Nov 21, 2021

Choose a reason for hiding this comment

Douile Nov 23, 2021

Choose a reason for hiding this comment

Stypox commented Dec 7, 2021

Stypox left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 3, 2022

Stypox left a comment

Choose a reason for hiding this comment

TobiGr left a comment

Choose a reason for hiding this comment

TobiGr Jan 15, 2022

Choose a reason for hiding this comment

TobiGr Jan 15, 2022

Choose a reason for hiding this comment

Douile Jan 15, 2022

Choose a reason for hiding this comment

Stypox Jan 24, 2022

Choose a reason for hiding this comment

Douile commented Sep 19, 2021 •

edited

Loading