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

Add MixInfoItem and extract YouTube mixes in related items #788

Merged
merged 13 commits into from
Mar 19, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Feb 2, 2022

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • [no need] I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Replaces #685, thank you @opusforlife2 for the ping there ;-)

Changes

  • This PR adds a mix subpackage that contains a MixInfoItem, i.e. a new type of InfoItem separated from PlaylistInfoItem that adds an abstract getMixType function and nothing else to the plain InfoItem (since mixes do not have an uploader or a stream count). The mix type can be one of: stream, music, channel (as described here). The mix subpackage also contains the MixInfoItemCollector and the base MixInfoItemExtractor. See Add MixInfoItem and extract YouTube mixes in related items #788 (comment)
  • I renamed InfoItemsSearchCollector into MultiInfoItemsCollector and added support for also collecting MixInfoItemExtractors. I could do this since the essence of that class was just to combine some collectors into one, so it had nothing to do with "search" but could be useful everywhere we want to return a list with multiple types of items.
  • [YouTube] I implemented mixes in related searches in YouTube. Since some code in YouTubeMixInfoItemExtractor would have been duplicated with YoutubeStreamInfoItemExtractor and classes related to mixes, I created some common functions in YouTubeParsingHelper (the TODO there is unrelated to this PR and was just copied over). Related videos have compactVideoRenderer as a key in the json object, while related mixes have compactRadioRenderer (note: still untested for channel, music and my mixes)

@XiangRongLin what do you think about this? :-)

@AudricV AudricV added enhancement New feature or request multiservice Issues related to multiple services youtube service, https://www.youtube.com/ labels Feb 2, 2022
@Stypox
Copy link
Member Author

Stypox commented Feb 3, 2022

Actually, I'm not so sure whether MixInfoItem should be completely separated from PlaylistInfoItem: maybe we should make it so that MixInfoItem extends PlaylistInfoItem? This way backward compatibility would be better (no need to instantly adapt to a new InfoItem type, since MixInfoItem could just be used as a PlaylistInfoItem) and maybe some services (will) have an uploader and/or a video count (which can just be set to default values in case they don't exist). What do you think?

@opusforlife2
Copy link
Collaborator

Yay! Youtube Music mixes! 💃

@Stypox
Copy link
Member Author

Stypox commented Feb 17, 2022

I made some changes:

  • Removed the mix subpackage and followed @XiangRongLin's suggestion of just adding a getPlaylistType() method to PlaylistInfoItem along with the related enum
  • Also added getPlaylistType() to PlaylistInfo and implemented it for YouTube mixes
  • Added the genre playlist type
  • Improved the test with the streams API and fixed the flaky case as requested (I still ensure that there is exactly one test of the specific type though)
  • Add full test for genre mix (note: I just copied one the other tests and changed the data, so code and TODOs are duplicated but this will not be solved in this PR)
  • Changed some mix tests that were referencing an invalid video id

Testing apk: app-debug.zip

@Stypox
Copy link
Member Author

Stypox commented Feb 27, 2022

As TiA4f8R pointed out I need to change the videos being tested to non-copyrighted ones

@opusforlife2
Copy link
Collaborator

Every Mix gives me this error:

Exception

Crash log

java.lang.NullPointerException: Attempt to invoke interface method 'java.util.Iterator java.util.List.iterator()' on a null object reference
	at org.schabi.newpipe.extractor.services.youtube.YoutubeParsingHelper.extractCookieValue(YoutubeParsingHelper.java:1090)
	at org.schabi.newpipe.extractor.services.youtube.extractors.YoutubeMixPlaylistExtractor.onFetchPage(YoutubeMixPlaylistExtractor.java:88)
	at org.schabi.newpipe.extractor.Extractor.fetchPage(Extractor.java:54)
	at org.schabi.newpipe.extractor.playlist.PlaylistInfo.getInfo(PlaylistInfo.java:65)
	at org.schabi.newpipe.util.ExtractorHelper.lambda$getPlaylistInfo$9(ExtractorHelper.java:171)
	at org.schabi.newpipe.util.ExtractorHelper$$ExternalSyntheticLambda11.call(Unknown Source:4)
	at io.reactivex.rxjava3.internal.operators.single.SingleFromCallable.subscribeActual(SingleFromCallable.java:43)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleDoOnSuccess.subscribeActual(SingleDoOnSuccess.java:35)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeFromSingle.subscribeActual(MaybeFromSingle.java:41)
	at io.reactivex.rxjava3.core.Maybe.subscribe(Maybe.java:5330)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.drain(MaybeConcatArray.java:153)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray$ConcatMaybeObserver.request(MaybeConcatArray.java:78)
	at io.reactivex.rxjava3.internal.operators.flowable.FlowableElementAtMaybe$ElementAtSubscriber.onSubscribe(FlowableElementAtMaybe.java:66)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeConcatArray.subscribeActual(MaybeConcatArray.java:42)
	at io.reactivex.rxjava3.core.Flowable.subscribe(Flowable.java:15868)
	at io.reactivex.rxjava3.internal.operators.flowable.FlowableElementAtMaybe.subscribeActual(FlowableElementAtMaybe.java:36)
	at io.reactivex.rxjava3.core.Maybe.subscribe(Maybe.java:5330)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeToSingle.subscribeActual(MaybeToSingle.java:46)
	at io.reactivex.rxjava3.core.Single.subscribe(Single.java:4813)
	at io.reactivex.rxjava3.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
	at io.reactivex.rxjava3.core.Scheduler$DisposeTask.run(Scheduler.java:644)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:65)
	at io.reactivex.rxjava3.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:56)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at java.lang.Thread.run(Thread.java:919)


Did no one else encounter this? I didn't bother reporting because I thought you would have come across it instantly.

}
}

public static class GenreMix {
Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find another video which suggests me a genre mix, so I won't change these, also because they are autogenerated playlists, not actual possible copyrighted videos


public class YoutubeStreamExtractorRelatedMixTest extends DefaultStreamExtractorTest {
private static final String RESOURCE_PATH = DownloaderFactory.RESOURCE_PATH + "services/youtube/extractor/stream/";
static final String ID = "UtF6Jej8yb4";
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead I found an NCS video with a stream mix and a yt music mix in the suggestions, I will use this one: https://www.youtube.com/watch?v=K4DyBUG242c

@Stypox
Copy link
Member Author

Stypox commented Feb 28, 2022

I changed the copyrighted video to https://www.youtube.com/watch?v=K4DyBUG242c as requested and, since that video also incuded a music mix, I found out music mixes were not being extracted. They were a still different type of recommended item, compactPlaylistRenderer, so I added support for that, too. I did so by changing the MixPlaylistInfoItemExtractor to MixOrPlaylistInfoItemExtractor and by parsing the uploader name and the stream count automatically (this let me remove the hardcoded "YouTube" uploader). I pushed a commit for that.
Note that the YoutubeStreamExtractorRelatedMixTest only works reliably if the downloader is MOCK, since YouTube does not always send the same mixes each time. Should I do something for this? I don't think there is any way to test reliably this part other than only using MOCK, since suggested items alongside videos are autogenerated by YouTube.
@XiangRongLin please review again ;-)
Testing apk: app-debug.zip

@opusforlife2
Copy link
Collaborator

@Stypox The latest APK is from the delete-large-land-player branch (just FYI). And Mixes still give the same error.

@Stypox
Copy link
Member Author

Stypox commented Mar 1, 2022

@opusforlife2 I can't reproduce your error, for me they work fine. Also, I'd say it's unrelated to this PR, but I will try to fix it later today and open another PR.

@opusforlife2
Copy link
Collaborator

Progress! Tested Adele's Hello. It's the only video where I got two mixes in related videos instead of just one. One is the regular "Mix - [video name]", which still doesn't work. For all videos that showed me a Mix in related videos, this was the title format.

But the other one does! I don't know if you'll get the same suggestion, but it's called "Hip Hop Essentials" and the uploader is shown as "YouTube Music". There's no "Mix" in the title.

@Stypox
Copy link
Member Author

Stypox commented Mar 1, 2022

@opusforlife2 yep, the one you found is a YouTube Music Mix and is among the ones I added support for in 098acf2

@XiangRongLin XiangRongLin self-requested a review March 6, 2022 14:15
@XiangRongLin

This comment was marked as outdated.

Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

I just did a cursory review. Since the branch was force pushed I have no idea what exactly is new and old.
Codewise it looks good to me.

Testing apk: app-debug.zip

What exactly can be tested and how? API compatibility?

@Stypox
Copy link
Member Author

Stypox commented Mar 12, 2022

What exactly can be tested and how? API compatibility?

Well, you can open music videos on youtube in NewPipe and you can verify that mixes now appear as related items (which is what this PR is almost all about)

@Stypox
Copy link
Member Author

Stypox commented Mar 15, 2022

If yes one could link it here:

Done

@XiangRongLin have you understood what/how you should test?

Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

have you understood what/how you should test?

Yes. Then LGTM

@Stypox Stypox mentioned this pull request Mar 16, 2022
1 task
Stypox added 12 commits March 19, 2022 10:44
It is a collector that can handle many extractor types, to be used when a list contains items of different types (e.g. search). It was renamed from InfoItemsSearchCollector so that it can now be used not just for search but for any extractor needing it. It supports, streams, channels, playlists and *mixes*.
Note: genre mixes already worked, now they are just considered as such in various video id extraction and in related items
Note 2: now extracting a mix id from a *normal* youtube mix id will fail if the video id wouldn't be exactly 11 characters long
Replaces mix tests based on a strange mix type RDQM{videoId} (only reference I could find is ytdl-org/youtube-dl#26228) and with an invalid video id of 13 characters (the first two characters were QM, but even after removing QM there still wasn't a video available at that id).
Also updates mocks.
@Stypox
Copy link
Member Author

Stypox commented Mar 19, 2022

@XiangRongLin rebased and added @MockOnly to testRelatedItems, please review again and then merge :-)

@XiangRongLin XiangRongLin merged commit 7f2ea13 into TeamNewPipe:dev Mar 19, 2022
@opusforlife2
Copy link
Collaborator

Woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request multiservice Issues related to multiple services youtube service, https://www.youtube.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants