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

Extractor for youtube mix (auto-generated playlist) #280

Merged
merged 14 commits into from
Dec 14, 2020

Conversation

XiangRongLin
Copy link
Collaborator

@XiangRongLin XiangRongLin commented Mar 7, 2020

  • I carefully read the contribution guidelines and agree to them.
  • I did test the API against NewPipe.
  • I agree to ASAP create a PULL request for NewPipe for making in compatible when I changed the api.

closes TeamNewPipe/NewPipe#2895

Problem

As describes in the issue in the NewPipe repo, when sharing a youtube mix to newpipe, it crashes. Reason for that is that the linkhandler receives (1) https://www.youtube.com/watch?v=QXCE3Betyug&list=RDQXCE3Betyug and converts it to (2) https://www.youtube.com/playlist?list=RDQXCE3Betyug , which loads a basicly empty page.

Changes

  • The LinkHandler doesn't convert mix urls.
  • New PlaylistExtractor to extract playlist information from the url (1).
  • The YoutubeService uses the new extractor if it is a mix.

Testing

I tested the changes on my android 10 and 7.1 devices and a 5.0 emulator, by:

  • sharing a link from my browser to NewPipe
  • saving the playlist and opening it from the bookmarks fragment
  • playling the playlist (didn't play on 5.0 emulator, but i can't play ANY video here)

@Iamdeadlyz could you test the apk.
app-debug.zip

What's missing

  • Mixes are generated based on the user. Meaning if I open the site once while logged in and once while in incognito mode, it gives me different playlists.
  • The size of a mix is infinity, but getStreamCount returns a long. So i don't really know how to handle that. I would let that method return -1 and let the frontend interpret that as infinity.
  • Check if "My Mix" is getting handled correctly. Here the list id starts with "RDMM".

Notes

  • In the mix url (1) above you can see the playlist id is just the video id with "RD" at the start.
    All mix urls that i found so far are like that, where the playlist id is "RD" + video id of the first video. I assume RD stands for radio (Playlist ids start with "PL" instead)
  • Since i can't reopen PR (Extractor for youtube mix (auto-generated playlist) #253) because i force pushed, i'll add the comments from @B0pol here:
  1. Uploader name should be YouTube
  2. Uploader avatar url should point to the YouTube logo
  • I noticed that there are quite a few duplicates if you continue scrolling down in a mix, especially the first video of the mix is repeated often.

Related PR: #276
Depends on: TeamNewPipe/NewPipe#3243

@Iamdeadlyz
Copy link

Hi @XiangRongLin

Thanks for the ping! The debug app works well with the auto-generated playlist. I can confirm that the issue is now fixed.

@PeterHindes
Copy link

Hey @XiangRongLin isn't "my mix" based on your youtube profile? How would that work in new-pipe at all?

@XiangRongLin
Copy link
Collaborator Author

@PeterHindes It just queues videos in the same genre. It's not very good at selecting them but it still works.

And currently you can only share mix urls to newpipe and not start a mix from newpipe itself, so you will always have a starting video on which the mix is based on.

I also just realized that i can't install my debug apk for whatever reason. I'll look into it on the weekend.

@Stypox Stypox linked an issue Mar 13, 2020 that may be closed by this pull request
@Stypox
Copy link
Member

Stypox commented Mar 13, 2020

I also just realized that i can't install my debug apk for whatever reason. I'll look into it on the weekend

I don't know why, but sometimes it happens. You can fix this by clicking "Clean Project" and rebuilding normally.

@wb9688
Copy link
Contributor

wb9688 commented Mar 13, 2020

@Stypox: I think this happens when you compile the app to run the activity again on your phone, if you just do a full build (no need to clean), it'll work again.

@Stypox
Copy link
Member

Stypox commented Mar 16, 2020

The size of a mix is infinity, but getStreamCount returns a long. So i don't really know how to handle that. I would let that method return -1 and let the frontend interpret that as infinity.

On IRC we are discussing if it would be a good idea to return -2 in these cases, that would mean "unknown number bigger than 100" (in this case infinity). -1 already means "unknown".

@XiangRongLin
Copy link
Collaborator Author

@Stypox
Thanks for the update.

In that case we could use static constants, so we don't have magic numbers floating around.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you for the effort! :-D
Code looks good


In that case we could use static constants, so we don't have magic numbers floating around.

Yeah, you could add a constant, something like "MORE_THAN_100", equal to -2. Then you would have to open another PR in the app to handle that correctly and show "100+" to the user. If you have no time I could do that.


Uploader name should be YouTube
Uploader avatar url should point to the YouTube logo

I think this is the way to go. The mix creator is Youtube, actually. So even though there is support for null/empty uploaders, it is better to return an hardcoded "YouTube" anyway. Also, for the uploader image, it is ok if you return a hardcoded youtube image url.


I noticed that there are quite a few duplicates if you continue scrolling down in a mix, especially the first video of the mix is repeated often.

Nothing to worry about, this is YouTube's fault ;-)

@XiangRongLin
Copy link
Collaborator Author

@Stypox
I applied your suggestions and updated to apk.
I'll leave the changes in the frontend to you. I have no clue how to integrate the "+" in "100+", so that it works in all languages.

I just saw that my CI build failed because i got a reCaptcha challange. The "getPageMultipleTimes" test may need to be removed.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Last few suggestions, thank you :)


I'll leave the changes in the frontend to you. I have no clue how to integrate the "+" in "100+", so that it works in all languages.

As soon as you are done implementing the renaming suggestion I will take this up. There is no need you upload another apk, since I will do it as soon as I implement this.


I just saw that my CI build failed because i got a reCaptcha challange. The "getPageMultipleTimes" test may need to be removed.

Don't worry, it happens sometimes ;-)

@Stypox
Copy link
Member

Stypox commented Mar 17, 2020

I opened the corresponding client PR: TeamNewPipe/NewPipe#3243
Debug apk: app-debug.zip

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Again, thank you for your work :-D

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

@wb9688 suggested in TeamNewPipe/NewPipe#3243 (comment) to separate "infinite" from "more than 100". So, sorry for asking you to change this again, but could you add another field named INFINITE_ITEMS = -2 and returning that instead? Also keep MORE_THAN_100_ITEMS = -3, since @wb9688 plans to use it. If you want you can just commit with the buttons below, or tell me to do that ;-)

@XiangRongLin
Copy link
Collaborator Author

@Stypox Feel free to just commit small stuff like that in the future.

@Stypox
Copy link
Member

Stypox commented Mar 20, 2020

@XiangRongLin I moved things around once again ;-)

@wb9688
Copy link
Contributor

wb9688 commented Mar 21, 2020

Does this support channel mixes (e.g. https://www.youtube.com/watch?v=mnk6gnOBYIo&list=RDCMUCXuqSBlHAE6Xw-yeJA0Tunw&start_radio=1&t=0)? You don't have tests for them.

Copy link
Member

@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.

Code looks good to me.
Thank you for the patience!
Can you fix the conflicts, please?

@XiangRongLin
Copy link
Collaborator Author

@TobiGr It seems like that this is already so old, that YouTube has already changed their json format. So I'll have to look into it again.

On another note, NewPipe want to be very privacy friendly and (I think) avoid any way to allow tracking. Does this feature fit in there? Because it uses cookies to allow for continuations.

@TheAssassin
Copy link
Member

On another note, NewPipe want to be very privacy friendly and (I think) avoid any way to allow tracking. Does this feature fit in there? Because it uses cookies to allow for continuations.

That's quite a good point. You'd have to make sure to limit tracking really well (ideally store different cookies for different mixes, etc.). It allows for tracking across very long periods of times, too. Sometimes a mix may run for hours.

We should discuss that in the team, I guess.

@XiangRongLin
Copy link
Collaborator Author

You'd have to make sure to limit tracking really well (ideally store different cookies for different mixes, etc.).

If i remember correctly, YouTube provides the cookie and i just send it back on continuations request. For a mix to not provide duplicate videos, it is needed for as long as the mix is running

@Stypox
Copy link
Member

Stypox commented Dec 12, 2020

If i remember correctly, YouTube provides the cookie and i just send it back on continuations request. For a mix to not provide duplicate videos, it is needed for as long as the mix is running

In my opinion this is ok, as it only tells YouTube that a random user seems to be really listening the mix, which YouTube would already know with more precision by looking at the video details request.
@XiangRongLin sorry for not merging this for a long long time. If you fix youtube mixes again I promise I'm going to review and finally merge in a short time (and ping me multiple times if I don't ;-) )

XiangRongLin and others added 14 commits December 12, 2020 20:30
-New YoutubeMixPlaylistExtractor, that extracts from a mix (auto-generated playlist).
-The url has the format of "youtube.com/watch?v=videoID&playlistID",
where playlistID always starts with "RD" and usually followed by the videoID.
-Change YoutubePlaylistLinkHandlerFactory to create a linkhandler with the given url if it is a mix.
-Change YoutubeService to return YoutubeMixPlaylistExtractor if the url is a mix.
…ID" format.

This occurs when sharing a mix from the official youtube app.
…of mix

Also fix getThumbnailUrl for "My Mix"
Move thumbnail id exctraction code to getThumbnailUrlFromId
Add test for "My mix" detection to service tests
Use ITEM_COUNT_UNKNOWN everywhere instead of -1 and add some tests
Channel mix adjusments and test
Don't accept youtube music mix urls as playlist
Don't override playlistData to keep getInitialPage()
Remove json constants
Indentation
This way youtube wont return duplicates when getting more items of the mix (but youtube can also track us)
@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Dec 12, 2020

@Stypox Problem was smaller than i expected, see the last commit of the bunch

As for the rebase, only conflicting file was YoutubeParsingHelper, where i think i got the correct stuff merged together.

@XiangRongLin
Copy link
Collaborator Author

apk at the top is already updated

@Stypox
Copy link
Member

Stypox commented Dec 14, 2020

I tested it with a couple mixes, and also with continuations, and it works as expected. Thank you! Sorry for taking so long ;-)

@Stypox Stypox merged commit 85fa006 into TeamNewPipe:dev Dec 14, 2020
@XiangRongLin XiangRongLin deleted the mixPL branch December 14, 2020 17:51
TobiGr added a commit that referenced this pull request Dec 23, 2020
Regression introduced by YouTube Mix support (#280)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement youtube service, https://www.youtube.com/
Projects
None yet