-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 support for an external signature server #4772
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I don't really see anything wrong with the logic here. I just have a couple questions to make sure I understand everything correctly.
Also, what do you think about splitting the signature communication logic into its own shard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some Ameba nitpicks. Feel free to ignore.
"referer" => "https://www.youtube.com/watch?v=#{video_id}", | ||
} of String => String | Int64 | ||
|
||
if {"WEB", "TVHTML5"}.any? { |s| client_config.name.starts_with? s } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[C] Naming/BlockParameterName: Disallowed block parameter name found
Can you pick a more descriptive name than s
here?
Why though? Sound harder to maintain and I don't see anyone using this code except us. |
Ah right. I did not think about the additional maintenance burden. I requested it to potentially open the opportunity for other YouTube projects within the Crystal ecosystem though the possibility of that is pretty low... Let's just keep it within Invidious. |
Important note: with the current state of the PR, invidious can't start if the socket is not present. I will add some logic to fallback to another client in that case. |
I don't see the way to configure to which address the inv_sig_helper is listening on. Is it done on purpose or an oversight? I'm thinking about docker, where each service has its separate private IP address. |
Currently, it's listening on a UNIX socket (aka. on a path, by default: |
Ah, yes that would be a great improvement because we have people in the community that run Invidious over multiple servers, and you can only communicate through TCP if you have multiple servers, a UNIX socket won't work. I'm not sure if the sig parameter can be shared between multiple public IP addresses or not, though. |
That would make the current design, of Invidious sending a
The JS code doesn't require network access, so I would hope so |
Continuing the discussion about signature helper server here: iv-org/inv_sig_helper#1 |
There will be a need to add the support for TCP/IP, as such that it can be used from multiple invidious processes. Issue opened on inv_sig_helper side: iv-org/inv_sig_helper#2 |
i've been running this and #4789 on |
Co-authored-by: Brahim Hadriche <[email protected]>
Missing feature:
|
File is set to be removed with iv-org#4772
17ef3f8
to
3592858
Compare
To also write it here: |
Yeah, I should rewrite the URLs during parsing (i.e before caching) not during use. |
Closes #4649
This PR adds support for inv_sig_helper, which offloads the player fetching, function extraction and signature parsing, which in turn allows to use the web client to watch videos.
Side note: I had to update the crystal overrides because the stdlib changed quite a while ago (See crystal-lang/crystal#11049) and it was required to properly use TCP/unix sockets.
TODO:
inv_sig_helper_url: XX
is filled. This way, this functionality is only enabled when the user needs it. And the user can specify a TCP endpoint in case of Docker, for example.Additionally, if this is not activated, ANDROID_TEST_SUITE will still be used. (As long as it still works fine for "unblocked IP addresses")This will be addressed when this and Add ability to set po_token and visitor data #4789 will be merged