feat: Cast file with local or remote subtitles #93

marcosdiez · 2018-05-03T04:43:25Z

fixes #46

skorokithakis · 2018-05-03T08:35:20Z

Daaamn, this is amazing, thank you! @theychx, do you want to review this with me?

theychx · 2018-05-03T11:30:37Z

Yep.

skorokithakis · 2018-05-03T13:19:11Z

catt/cli.py

@@ -103,28 +105,98 @@ def write_config(settings):
        raise CattCliError("No device specified.")


+def hunt_subtitle(video):
+    dot_pos = video.rfind(".")


It would be nice to use Pathlib here, and also to write a more robust algorithm. My subtitle names aren't always videofile.srt, sometimes they are videofile.en.srt and videofile.el.srt, so it would be good to do some globbing and fetch the first file alphabetically, if nothing is selected.

theychx

This works really well. I've left a few notes.

theychx · 2018-05-03T12:15:19Z

catt/cli.py

 @cli.command(short_help="Send a video to a Chromecast for playing.")
 @click.argument("video_url", callback=process_url)
+@click.argument("subtitle", required=False, default=None, callback=process_subtitle)


default=None is not necessary, as the value becomes None if the argument is not supplied.

theychx · 2018-05-03T12:21:33Z

catt/cli.py

    controller = "default" if force_default else None
    cst, stream = setup_cast(settings["device"], video_url=video_url,
                             prep="app", controller=controller)

    if stream.is_local_file:
        click.echo("Casting local file %s..." % video_url)
        click.echo("Playing %s on \"%s\"..." % (stream.video_title, cst.cc_name))
+        subtitle_url = load_subtitle_if_exists(subtitle, video_url, stream)


I would prefer if you supplied port and local_ip to load_subtitle_if_exists instead of the entire stream object.

theychx · 2018-05-03T12:26:17Z

catt/cli.py

+        # it's an URL
+        return subtitle
+
+    if len(subtitle) > 4 and subtitle.lower()[-4:] == ".srt":


if subtitle.lower().endswith(".srt") would be more concise.

theychx · 2018-05-03T12:40:57Z

catt/cli.py

+
+def load_subtitle_if_exists(subtitle, video, stream):
+    if subtitle is None:
+        subtitle = hunt_subtitle(video)


subtitle = subtitle or hunt_subtitle(video) would be more concise.

theychx · 2018-05-03T14:34:15Z

catt/cli.py

+
+    if "://" in subtitle:
+        # it's an URL
+        return subtitle


You're not converting remote srt subtitles.

marcosdiez · 2018-05-04T00:21:54Z

Hi! Thanks for the nice review. I agree 100% with all the mentioned items.
I am not really sure if anybody will ever try to load a remote subtitle, but converting it to WEBVTT was not hard anyway.

Congratulations once more for catt. It's really useful.

marcosdiez · 2018-05-04T00:23:28Z

Sorry, I was not clear in my last message. I implemented everything you mentioned and also added an extra parameter --dont-load-subtitles-automatically, which could sometimes (readonly) be necessary

theychx · 2018-05-04T12:47:47Z

catt/cli.py

+            continue
+        if entry_path.stem.lower().startswith(video_path_stem_lower) and \
+                entry_path.suffix.lower() in [".vtt", ".srt"]:
+            return entry


This is not working, as you're not returning the absolute path. You could do this:
for entry_path in video_path.parent.iterdir()
(and thus, no longer need os)
And then let the function return str(entry_path.resolve())

theychx · 2018-05-04T12:52:13Z

catt/cli.py

    controller = "default" if force_default else None
    cst, stream = setup_cast(settings["device"], video_url=video_url,
                             prep="app", controller=controller)

    if stream.is_local_file:
        click.echo("Casting local file %s..." % video_url)
        click.echo("Playing %s on \"%s\"..." % (stream.video_title, cst.cc_name))
+        subtitle_url = load_subtitle_if_exists(subtitle, dont_load_subtitles_automatically, video_url, stream.local_ip, stream.port + 1)


I would prefer if the flag-handling logic was here, instead of the flag being passed on to the function. Like:

if subtitle is None and dont_load_subtitles_automatically: subtitle_url = None else: subtitle_url = load_subtitle_if_exists(subtitle, video_url, stream.local_ip, stream.port + 1)

marcosdiez · 2018-05-05T12:10:31Z

Sorry for the delay.
I was annoyed catt was not working on WSL and it took me a while to fix that.
It turns out the problem was in python-zeroconf ( python-zeroconf/python-zeroconf#130 ) so as soon as that get merged into pychromecast I'll be back here and ask you for another release :)

marcosdiez · 2018-05-05T12:14:30Z

By the way, @theychx I don't know what's wrong with travis and flake8 today... it's not my fault. sorry for that. I had the same issue on the python zeroconf PR :(

skorokithakis · 2018-05-05T12:16:32Z

I've had issues with them being flaky as well. Usually just rerunning the test works.

marcosdiez · 2018-05-05T13:27:05Z

@skorokithakis I rebased to make sure travis would run again and it failed again. My other PR also failed again, for the same reason...

any ideas ?

theychx · 2018-05-05T20:40:53Z

@marcosdiez The travis fails do not seem to be caused by anything in your code. I have no idea whats going on.

theychx

@marcosdiez This is really cool! Thanks!

skorokithakis · 2018-05-05T21:14:32Z

I have fixed the Travis failure. Let me see if I can update this branch with the fix.

skorokithakis · 2018-05-06T09:08:50Z

catt/cli.py

+    video_path = Path(video)
+    video_path_stem_lower = video_path.stem.lower()
+    for entry in video_path.parent.iterdir():
+        entry_path = Path(entry)


Is this necessary? Aren't the returned objects paths already?

skorokithakis · 2018-05-06T09:10:31Z

catt/cli.py

+
+
+def convert_srt_to_webvtt_helper(content):
+    content = re.sub(r'([\d]+)\,([\d]+)', r'\1.\2', content)


This seems a bit too general, it will catch things like internationalized numbers in the subtitles and mistakenly convert them. Can we make it more specific?

This works, if there aren't more SRT formats:

# Find all lines that contain `-->` (timing lines) and replace commas in them with periods. re.sub(r"^(.*? \-\-\> .*?)$", lambda m: m.group(1).replace(",", "."), content, flags=re.MULTILINE)

skorokithakis · 2018-05-06T09:23:57Z

catt/cli.py

+
+
+def convert_srt_to_webvtt(filename):
+    # print("Converting {} to WebVTT".format(filename))


The comment here should be removed.

skorokithakis · 2018-05-06T09:24:17Z

catt/cli.py

+
+def convert_srt_to_webvtt(filename):
+    # print("Converting {} to WebVTT".format(filename))
+    for possible_encoding in ['utf-8', 'iso-8859-15']:


We can use chardet here later on.

skorokithakis · 2018-05-06T09:25:12Z

catt/cli.py

+
+
+def load_subtitle_if_exists(subtitle, video, local_ip, port):
+    subtitle = subtitle or hunt_subtitle(video)


Sorry to change this again, but the ternary if is more readable here:

subtitle = subtitle if subtitle else hunt_subtitle(video)

skorokithakis · 2018-05-06T09:26:40Z

catt/cli.py

+            return subtitle
+
+    if subtitle.lower().endswith(".srt"):
+        subtitle = convert_srt_to_webvtt(subtitle)


Shouldn't this give an error unless the extension is .srt or .vtt instead? We wouldn't want to continue with any arbitrary file.

According to https://developers.google.com/cast/docs/media , Chromecast supports:

TTML - Timed Text Markup Language

(example, https://gist.github.com/anotherhale/676a72edc84ca3a37c0c )
WebVTT - Web Video Text Tracks
(example: https://github.com/brenopolanski/html5-video-webvtt-example/blob/master/MIB2-subtitles-pt-BR.vtt
Line 21 Data Services
example: https://en.wikipedia.org/wiki/EIA-608

I confess I never heard about any of these 3 and I bet nobody will ever use cast with the latter. And XML is a bitch extension, so I think we are fine doing what the user tells us to instead of complaining.

What do you think ?

The problem is that your code is not failing if the specified subtitle file has neither an ".srt" or ".vtt" extension.
(you are right that we should not concern ourselves with the other two obscure formats)

My point is there may be somebody who would like to use one of the obscure formats and if we arbitrarily forbid it because the extensions we are restricting for restriction's sake.

In the end of the day there is no downside in sending the wrong file to chromecast as an extension, so I think we should not restrict it

Also, we don't check the extension for the main files we cast, so we should not bother with the subtitles either.

Hmm, okay, this makes sense to me, thanks.

skorokithakis

Almost done, I just have a few comments and then we're good to merge!

skorokithakis · 2018-05-06T09:27:31Z

catt/cli.py

 @cli.command(short_help="Send a video to a Chromecast for playing.")
 @click.argument("video_url", callback=process_url)
+@click.argument("subtitle", required=False, callback=process_subtitle)


I think this would be better served as an option, rather than as an argument.

I agree, but that would also change the applicability of --dont-load-subtitles-automatically. Because if this was an option, you might as well just say that --subtitle without any argument means "try to automatically load subtitle".

Yes, exactly. And --no-subtitle would mean "don't load, even automatically".

Maybe NOT specifying --subtitle would suffice 😃 (I'll butt out of your review now. Sorry).

The problem is that there are three states, file/auto/none. So we need --subtitle/nothing/--no-subtitle.

Not specifying --subtitle means none.
Specifying --subtitle without an argument means auto.
Specifying --subtitle <subtitle> means file.
What am I missing?

I would prefer if auto loading subtitles was the default option. That's why I wrote this way.

We both agree with that, we're just discussing the interface, basically. It's a bit tricky. Maybe --subs/--no-subs and it automatically discovers if you specify neither?

So:
Nothing specified means "auto".
Specifying --subs <subtitle-file> means "file".
Specifying --no-subs means "none".

Having given this a bit of thought, I can see that this is probably the behaviour most users would expect.

good, we all agreed on the same thing. I just got somebody visiting me, so I'll continue later.

skorokithakis · 2018-05-06T09:28:24Z

catt/cli.py

 @click.option("-f", "--force-default", is_flag=True,
              help="Force use of the default Chromecast app (use if a custom app doesn't work).")
 @click.option("-r", "--random-play", is_flag=True,
              help="Play random item from playlist, if applicable.")
+@click.option("--dont-load-subtitles-automatically", is_flag=True, default=False,


Maybe this should be renamed to --no-subs? It's kind of long right now.

skorokithakis · 2018-05-06T09:29:30Z

catt/cli.py

 @click.pass_obj
-def cast(settings, video_url, force_default, random_play):
+def cast(settings, video_url, subtitle, force_default, random_play, dont_load_subtitles_automatically):


Having subtitle be an option would work better here as well, as I don't like changing function signatures that drastically. However, we don't have default arguments for this, so it'd break anyway.

Oh, I misread "subtitle without any argument". I think loading the associated sub should be the default behaviour, without needing an argument...

I.e. without needing to specify --subtitle, I mean.

skorokithakis · 2018-05-06T09:30:25Z

Thanks again for this PR, @marcosdiez, this is extremely useful. I'll release 0.7.0 as soon as we merge.

skorokithakis · 2018-05-06T09:31:36Z

catt/cli.py

    controller = "default" if force_default else None
    cst, stream = setup_cast(settings["device"], video_url=video_url,
                             prep="app", controller=controller)

    if stream.is_local_file:
        click.echo("Casting local file %s..." % video_url)
        click.echo("Playing %s on \"%s\"..." % (stream.video_title, cst.cc_name))
+        if subtitle is None and dont_load_subtitles_automatically:


Is there any reason we can't cast remote files with local subtitles as well? It's going to be a bit more tricky, but if it's easy to do, why not?

This would not work with save/restore, as we would face the same problem as with save/restore for local files. And if remote files + local subtitles was implemented, users would probably expect this to work.

Ah, alright.

theychx · 2018-05-06T09:51:54Z

@skorokithakis

I'll release 0.7.0 as soon as we merge

Please wait a few days before releasing.

marcosdiez · 2018-05-06T23:39:58Z

I added "requests>=2.0", to setup.py. I got this version because it's the same on pychromecast, so this way the requirement is there but won't cause an extra download

This reverts commit 4ef5c5b.

theychx · 2018-05-07T06:19:58Z

@marcosdiez What is the 7653225 commit doing in this PR?

skorokithakis · 2018-05-07T11:39:33Z

.travis.yml

@@ -9,4 +9,5 @@ install: "pip install -r requirements_dev.txt"
 # command to run tests, e.g. python setup.py test
 script:
  - python setup.py test
+  - flake8 --version


This is probably unnecessary, the version is printed when installing.

I can take it out, but for me it was very useful.
I was doing some other development here and flake was telling me other stuff. If you check it out I have PR called flake8 is happy again and another one called Revert "flake8 is happy again", because different flake versions give me different output, so that's the easy way to know what sublibraries flake8 has.

I applied the same trick here https://github.com/jstasiak/python-zeroconf/pulls and that's why I am the only one with a passing build. I would keep it. It does not affect the end user and make the devs happier.

Yeah, that's true, I just generally dislike putting too much stuff in PRs. This is fine though, thanks.

skorokithakis · 2018-05-07T11:43:20Z

catt/http_server.py

    class FileHandler(BaseHTTPRequestHandler):
+
+        def format_size(self, size):


What is this for?

Very useful debugging information.
Now we get this kind of logs:
10.1.1.3 - - [07/May/2018 11:53:56] "GET /?loaded_from_catt HTTP/1.1" 200 - video/mp4 - 39.80 MB
10.1.1.3 - - [07/May/2018 11:53:56] "GET //tmp/tmpmnfqm1im.vtt HTTP/1.1" 200 - text/vtt;charset=utf-8 - 125.97 KB

I backported from my other PR.

I like knowing how big the file and the mime is useful for the other one anyway.

skorokithakis · 2018-05-07T12:40:26Z

Alright, this looks good to me! @theychx, any feedback/objections?

theychx · 2018-05-07T13:24:11Z

Looks cool! (and passes my testing scheme)

skorokithakis · 2018-05-07T13:24:58Z

Great, approved! Thank you, @marcosdiez!

mdiez-modus · 2018-05-07T13:37:42Z

NIIIIIIIIIIIIIIIIIICE !

skorokithakis reviewed May 3, 2018

View reviewed changes

theychx requested changes May 3, 2018

View reviewed changes

theychx reviewed May 4, 2018

View reviewed changes

marcosdiez added 7 commits May 5, 2018 13:19

we can now cast localfile with local or remote subtitles

141f345

hopefully we are safe against encoding errors

1dc527a

easy changes

493a831

we search harder for subtitles

5335007

option not to load subtitles automatically

9688d19

we now convert remote subtitles from SRT to WEBVTT

f292d8f

fix loading subtitles from a different path

42dd55d

marcosdiez force-pushed the subtitle_support branch from 0bba65d to 42dd55d Compare May 5, 2018 13:20

theychx approved these changes May 5, 2018

View reviewed changes

skorokithakis changed the title ~~we can now cast localfile with local or remote subtitles~~ feat: Cast local file with local or remote subtitles May 5, 2018

skorokithakis added 2 commits May 6, 2018 00:17

Avoid broken pep8-naming version

8333f14

Merge branch 'master' into subtitle_support

89672cb

skorokithakis reviewed May 6, 2018

View reviewed changes

skorokithakis requested changes May 6, 2018

View reviewed changes

skorokithakis changed the title ~~feat: Cast local file with local or remote subtitles~~ feat: Cast file with local or remote subtitles May 6, 2018

skorokithakis reviewed May 6, 2018

View reviewed changes

marcosdiez added 4 commits May 6, 2018 19:25

minor code changes

3bfd30b

better way to convert subtitles from SRT to WEBTT

6e352b5

make subtitle an option instead of an argument

ba38ffa

flake8 is happy again

4ef5c5b

marcosdiez added 4 commits May 6, 2018 23:43

HttpServer now shows the file size and the mime

7653225

small refactor

0a6a57c

detect flake8 version

d03fbe1

Revert "flake8 is happy again"

1f7ce75

This reverts commit 4ef5c5b.

skorokithakis reviewed May 7, 2018

View reviewed changes

skorokithakis approved these changes May 7, 2018

View reviewed changes

skorokithakis merged commit edd22b4 into skorokithakis:master May 7, 2018



		def convert_srt_to_webvtt_helper(content):
		content = re.sub(r'([\d]+)\,([\d]+)', r'\1.\2', content)



		def convert_srt_to_webvtt(filename):
		# print("Converting {} to WebVTT".format(filename))



		def load_subtitle_if_exists(subtitle, video, local_ip, port):
		subtitle = subtitle or hunt_subtitle(video)

		class FileHandler(BaseHTTPRequestHandler):

		def format_size(self, size):

feat: Cast file with local or remote subtitles #93

feat: Cast file with local or remote subtitles #93

Conversation

marcosdiez commented May 3, 2018

skorokithakis commented May 3, 2018

theychx commented May 3, 2018

Choose a reason for hiding this comment

theychx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosdiez commented May 4, 2018

marcosdiez commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosdiez commented May 5, 2018

marcosdiez commented May 5, 2018

skorokithakis commented May 5, 2018

marcosdiez commented May 5, 2018

theychx commented May 5, 2018 • edited Loading

theychx left a comment

Choose a reason for hiding this comment

skorokithakis commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skorokithakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theychx May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theychx May 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skorokithakis commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theychx commented May 6, 2018

marcosdiez commented May 6, 2018

theychx commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skorokithakis commented May 7, 2018

theychx commented May 7, 2018

skorokithakis commented May 7, 2018

mdiez-modus commented May 7, 2018

theychx commented May 5, 2018 •

edited

Loading

theychx May 6, 2018 •

edited

Loading

theychx May 6, 2018 •

edited

Loading