-
Notifications
You must be signed in to change notification settings - Fork 59
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
Mitigate error 429: too many requests #122
Mitigate error 429: too many requests #122
Conversation
My guess would be that Soundcloud don't make many changes and although this is scraping, it's stable enough. Would that be a fair assessment, do you think? Are public streams common? What decides if they are public or not? I'm just curious what sort of improvement we can expect from this? |
P. S. Thanks very much for these PRs. I will look closer later. |
Two other packages implement similar ways of streaming using public client ids: https://pypi.org/project/youtube_dl/ and https://pypi.org/project/soundcloud-lib/. Not sure what fraction of streaming is public, so hard to say what impact it has. https://github.com/ytdl-org/youtube-dl/blob/04d4a3b136060158438c3f2c1b31c884c6961712/youtube_dl/extractor/soundcloud.py#L277-L289 The ‘sharing’ attribute in the track’s json shows if the track is available publicly. This is used to decide if the public client id will be used to stream the track. This json is obtained through the apps standard client id. The scraping is done a bit provisionally, but does the work for the streams I’ve tested. |
Seems it does break but we can see how it goes. If it does break maybe we can look at depending on soundcloud-lib and relying on those fixes. |
Yes sounds reasonable. Ill look into passing the tests and adding a few for the new code. |
57dc070
to
d5642d3
Compare
I noticed that tracks only available with "SoundCloud Go+" now play a preview whereas previously we considered them unplayable and they were skipped (which could easily eat up our quota). We could avoid this by filtering out "/preview/progressive" strings but maybe this behaviour is better. Maybe we should mark them as previews if we keep this. EDIT: actually, now I've gone back to the master branch it looks like the old method also plays the preview... I must have just remembered it wrong so never mind. We should consider marking these tracks as previews but that is out of scope here. |
26f86b6
to
9cc707c
Compare
…use of standard app client id
4a198c8
to
a3d8acb
Compare
a3d8acb
to
e011cf1
Compare
Pulled down your fix @Laurentww. Works great, thanks for your effort on this. |
@Laurentww is this ready for merging? I recommend going through all of the comments above and clicking the "resolve conversation" button for each thread that's no longer relevant. |
I do think it is ready to merge. Thanks for the suggestions! PS: I have a newer branch which makes use of API-v2 and includes images. That branch has more rigorous code changes and might therefore be better to merge later on. For now this PR should get this plugin up and running. |
Merged in 89bac5b |
Streams publicly available tracks using a public client id to unload use of standard app client id