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

Youtube api improvements #2277

Merged
merged 16 commits into from
Aug 3, 2021

Conversation

SamantazFox
Copy link
Member

@SamantazFox SamantazFox commented Jul 25, 2021

This PR is intended to unify the implementation of the next endpoint. The authors of #2217, #2220, #2227 and #2257 will have to rebase their changes against this PR.

Notables changes:

  • The youtube API functions have been grouped under the YoutubeAPI namespace and function names have been simplified, for code clarity.
  • The next endpoint takes data in Hash or NamedTuple form, so it can be extended in the future, without requiring new function overrides. See documentation in the code for more details.

Comments and search result aren't returned by the browse()
endpoint but by the next() and search() endpoints, respectively.
@unixfox
Copy link
Member

unixfox commented Jul 25, 2021

Should we also implement the others endpoints in this PR and allow to set others clients?

@SamantazFox
Copy link
Member Author

Should we also implement the others endpoints in this PR and allow to set others clients?

I was wondering that, too. You don't mind if that ends up rewriting half of your PRs?

@unixfox
Copy link
Member

unixfox commented Jul 25, 2021

Should we also implement the others endpoints in this PR and allow to set others clients?

I was wondering that, too. You don't mind if that ends up rewriting half of your PRs?

No and that's a good thing.

@SamantazFox SamantazFox force-pushed the youtube-api-improvements branch 2 times, most recently from 59c36d5 to 6577c55 Compare July 26, 2021 15:11
And accept gzip compressed data, to help save on bandwidth
@SamantazFox
Copy link
Member Author

Here we go, I'm done here.

syeopite
syeopite previously approved these changes Jul 27, 2021
@unixfox
Copy link
Member

unixfox commented Jul 27, 2021

Related to #2189 (comment), we will need to manipulate the context.

@syeopite syeopite dismissed their stale review July 27, 2021 07:05

Context needs to be manipulatable as shown by recent information

@SamantazFox
Copy link
Member Author

Related to #2189 (comment), we will need to manipulate the context.

Yeah, I saw that, however, I don't know if that can be provided in any request (We're always using the embed player, so it might fit) or is it required only for age bypass?

@syeopite
Copy link
Member

syeopite commented Jul 27, 2021

Afaik it should only be required for the age restriction bypass though as shown by recent investigations, using different clients and contexts does provide slightly different functionality.

Either way, the context should be configurable through ClientConfig.

@SamantazFox
Copy link
Member Author

Either way, the context should be configurable through ClientConfig.

That's harder than expected, lol

@unixfox
Copy link
Member

unixfox commented Jul 27, 2021

We will probably need android embed too in order to solve the throttling issues and directly decrypt the encrypted video streams.

@SamantazFox
Copy link
Member Author

We will probably need android embed too in order to solve the throttling issues and directly decrypt the encrypted video streams.

Sure! I don't know how/where to get those, and probably can't anyway (old, ungoogled phone), so any help is appreciated.

@syeopite
Copy link
Member

@SamantazFox
Copy link
Member Author

But you should be able to directly execute the decryption js.

the problem is that it requires an entire Js engine dependency to be added to invidious, which is something I'd prefer to avoid. See #2222.

@SamantazFox SamantazFox marked this pull request as ready for review July 31, 2021 17:52
@SamantazFox
Copy link
Member Author

SamantazFox commented Jul 31, 2021

@syeopite regarding the ability to modify the context, I tried to implement that, but it's a hell of a nightmare. It seems that crystal (1.0.0, for now) have troubles with merging Hash structures.

I.e, the basic merge()/merge!() function only provide 1 level of merging, so merging {"client": {"foo":1} } and {"client": {"bar":1} } will return {"client": {"bar":1} }, and using a block (merge() { |key,v1,v2| # do_something}) to handle 2nd/3rd level merge will make the compiler complain that the data type doesn't match (the previous example is a Hash(String, String | Hash(String, String), but at level 2 it becomes String | Hash(String, String), which can't be merged.

So for now, I think it's better to keep things like that, and I'll have to find a way of making better data structures (maybe with a JSON::builder?) to hold the context data and allow modification.

@unixfox
Copy link
Member

unixfox commented Jul 31, 2021

@syeopite regarding the ability to modify the context, I tried to implement that, but it's a hell of a nightmare. It seems that crystal (1.0.0, for now) have troubles with merging Hash structures.

I.e, the basic merge()/merge!() function only provide 1 level of merging, so merging {"client": {"foo":1} } and {"client": {"bar":1} } will return {"client": {"bar":1} }, and using a block (merge() { |key,v1,v2| # do_something}) to handle 2nd/3rd level merge will make the compiler complain that the data type doesn't match (the previous example is a Hash(String, String | Hash(String, String), but at level 2 it becomes String | Hash(String, String), which can't be merged.

So for now, I think it's better to keep things like that, and I'll have to find a way of making better data structures (maybe with a JSON::builder?) to hold the context data and allow modification.

But you can technically merge the second level this way:

hash1 = {"client" => {"foo" => 1} }
hash2 = {"client" => {"bar" => 1} }
hash1["client"].merge!(hash2["client"])
puts hash1 #{"client" => {"foo" => 1, "bar" => 1}}

Copy link
Member

@syeopite syeopite left a comment

Choose a reason for hiding this comment

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

It seems that crystal (1.0.0, for now) have troubles with merging Hash structures.

In that case, this is fine for now.

ClientType::WebAgeBypass => {
name: "WEB",
version: "2.20210721.00.00",
api_key: "AIzaSyAO_FJ2SlqU8Q4STEHLGCilw_Y9_11qcW8",
screen: "EMBED",
},

Everything else looks pretty good!

@SamantazFox
Copy link
Member Author

But you can technically merge the second level this way:

hash1 = {"client" => {"foo" => 1} }
hash2 = {"client" => {"bar" => 1} }
hash1["client"].merge!(hash2["client"])
puts hash1 #{"client" => {"foo" => 1, "bar" => 1}}

I've tried that, and the compiler's not hapy :(

 165 | client_context["client"].merge!(
                                ^-----
Error: undefined method 'merge!' for String (compile-time type is (Hash(String, String) | String))

@SamantazFox
Copy link
Member Author

@syeopite I added more clients. Still approved for you?

@unixfox anything to say?

src/invidious/helpers/youtube_api.cr Show resolved Hide resolved
src/invidious/helpers/youtube_api.cr Outdated Show resolved Hide resolved
@SamantazFox SamantazFox merged commit 5b020e8 into iv-org:master Aug 3, 2021
@SamantazFox SamantazFox deleted the youtube-api-improvements branch February 7, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants