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

feat: Add remote-only subtitles to the API #258

Merged
merged 5 commits into from
Aug 12, 2020
Merged

Conversation

skorokithakis
Copy link
Owner

For your consideration, a subtitle parameter to the API call.

I'm not sure whether it should be called subtitle_url, though, I'm not sure if it has to be a URL or if it can be a path.

Copy link
Collaborator

@theychx theychx left a comment

Choose a reason for hiding this comment

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

This would obviously be useful, but we would have to use SubsInfo like we do in cli.py (because of CORS and stuff).

@skorokithakis
Copy link
Owner Author

Good point, will fix, thank you sir.

@skorokithakis
Copy link
Owner Author

Hmm, I (finally) looked at this again, and it looks like we're downloading the subtitles and serving them for every case, right?

My current use case is that I serve a remote URL, and wouldn't want to keep catt running unless we have to, which for most subtitles I tried isn't the case. Maybe we can pass the remote URL through without re-serving if the subs are WebVTT already?

@theychx
Copy link
Collaborator

theychx commented May 13, 2020

which for most subtitles I tried isn't the case

Subtitles are supposed to be served once, and not forever (at least, thats how it is implemented in cli.py). Can you provide an url?
I use these for testing:

catt cast https://github.com/brenopolanski/html5-video-webvtt-example/blob/master/MIB2.mp4?raw=true -s https://github.com/brenopolanski/html5-video-webvtt-example/raw/master/MIB2-subtitles-pt-BR.vtt
catt cast https://github.com/brenopolanski/html5-video-webvtt-example/blob/master/MIB2.mp4?raw=true -s https://github.com/brenopolanski/html5-video-webvtt-example/raw/master/MIB2-subtitles-pt-BR.srt

@skorokithakis
Copy link
Owner Author

I'll just test some myself, no need to bother you about it, I just thought I'd ask because my only Chromecast is in a room far away from my development machine.

@skorokithakis
Copy link
Owner Author

Alright, I gave this a go and, because it's an API, we're not setting up the Chromecast/getting a local IP/port. I think we would be better off merging this as is (since we have the equivalent currently with url and resolve=False.

You're right, however, I would very much like us to have a better story around properly handling/serving subtitles. We just need to make the serving explicit, as I'm not a fan of libraries starting threads in the background.

We should have a second class that will expose SubsInfo and allow the user to fetch/convert/serve the subtitles, and then pass them to the play_url call.

@theychx
Copy link
Collaborator

theychx commented May 14, 2020

since we have the equivalent currently with url and resolve=False

Eh?

We just need to make the serving explicit

What exactly does "explicit" entail?

I'm not a fan of libraries starting threads in the background

I agree, but because of CORS, subtitles wont work without us serving them, and I dont see how we can do that without starting a new thread.

@theychx
Copy link
Collaborator

theychx commented May 14, 2020

We should have a second class that will expose SubsInfo and allow the user to fetch/convert/serve the subtitles, and then pass them to the play_url call

Did you mean that this new thingy should also start the server thread?

@skorokithakis
Copy link
Owner Author

Eh?

We aren't doing the whole setup/discover/etc thing if resolve=False, you just pass a URL and we send it off, so we can do the same with the subtitles.

What exactly does "explicit" entail?

That we don't start the thread ourselves, the caller does.

I agree, but because of CORS, subtitles wont work without us serving them, and I dont see how we can do that without starting a new thread.

Hmm, that might be the case for local files, but they do actually work fine in my PR. Maybe the server has set CORS headers, though.

Did you mean that this new thingy should also start the server thread?

Yeah, so the caller knows when it starts and can control it. Ideally we'd make that overridable too, for if the caller wanted to start the server on one of their own threads.

@theychx
Copy link
Collaborator

theychx commented May 14, 2020

Maybe the server has set CORS headers, though

That must the case, it wouldn't work otherwise, which is why we need to implement your suggestions.
The rest of your answer makes sense to me, thanks.

@skorokithakis
Copy link
Owner Author

I'll think a bit more about the UI and see, I don't want to force people to have to setup the stream before we get an IP and port, but they might just be able to specify that themselves. Thanks for the feedback!

@skorokithakis
Copy link
Owner Author

I've looked at this again to get it merged, and, because of a combination of presumably very few people using the API, and the complexity of supporting local subtitles (and how much I need remote subs), I think we should merge this as-is for now, and we can later add an API parameter to properly support local subtitles. Ie we'll only support remote ones (of the proper type) for now.

Is that okay with you?

@theychx
Copy link
Collaborator

theychx commented Aug 12, 2020

Sure. A comment about CORS would be nice, so people don't think it'll work with any arbitrary URL.
I'm real busy these days, so I won't be able to work on it until September/October.

@skorokithakis
Copy link
Owner Author

No problem, I'll add a comment about CORS, thanks.

@skorokithakis skorokithakis changed the title Add subtitles to the API feat: Add remote-only subtitles to the API Aug 12, 2020
@skorokithakis skorokithakis merged commit 433b7e9 into master Aug 12, 2020
@skorokithakis skorokithakis deleted the feat/api-subs branch August 12, 2020 11:21
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.

2 participants