Support to Cast Any Website to Chromecast (via DashCast) #102

marcosdiez · 2018-05-14T04:41:24Z

In another step to make catt cast ALL the things, here is a simple PR to make it cast any website to Chromecast.

The heavy lifting is done by DashCast ( http://stestagg.github.io/dashcast/ ), which was easier than I thought to implement on catt because pychromecast already supports it.

I decided not to change the UI by falling back to dastcast every time youtube-dl can not handle a URL.

That means you just needs to type:

catt cast https://www.github.com/

to see github on your chromecast!

catt info shows the title of the webpage that is being displayed.

skorokithakis · 2018-05-17T12:37:07Z

Thank you for the PR! I'm a bit apprehensive about the mode of failure where the URL mistakenly doesn't return a video and the site itself gets cast. Maybe this should be an explicit switch that would force casting a site?

@theychx, what do you think?

mdiez-modus · 2018-05-17T12:39:52Z

I though a lot about that. Maybe have a completely different command, like catt cast_url URL
I don't like heuristics either.
Since this is a delicate topic in which is very easy for the 3 of us to have at least 4 different opinions, I let you guys decide what the best interface should be.

I can easily implement it after a conclusion is achieved!

skorokithakis · 2018-05-17T12:42:58Z

Yes, I think a dedicated command might be much better. I think this also fits with the spirit of catt, generally, and the code doesn't seem very complicated. Let's wait for @theychx to chime in as well.

mdiez-modus · 2018-05-17T13:04:15Z

my code is broken. please don't merge. I am adding the cast_url command as well. please do not merge yet

theychx · 2018-05-17T13:09:09Z

I'm not keen on the current approach. Making a separate command seems like the obvious solution, as this would mean that you could just call setup_cast with controller="dashcast", prep="app" and not need to modify StreamInfo or setup_cast.

skorokithakis · 2018-05-17T13:10:41Z

Yes, I'm keen on that for the same reason, it would remove much of the if is_website code duplication that smells bad.

theychx · 2018-05-17T13:23:37Z

@mdiez-modus cast_url is not a good name, as it's not distinct enough from the cast command and it's functionality. I thought of cast_site, but that's perhaps not a very good name either. @skorokithakis ?

skorokithakis · 2018-05-17T13:24:51Z

mirror_site?

theychx · 2018-05-17T13:27:03Z

Yeah, or just mirror.

skorokithakis · 2018-05-17T13:28:08Z

I think mirror_site is a bit more explicit as to what you're mirroring, but I have no strong preference either way.

mdiez-modus · 2018-05-17T18:52:20Z

mirror_site ? I would prefer display_website ...

skorokithakis · 2018-05-17T19:07:05Z

The official terminology that chrome uses is "tab mirroring", so this kind of keeps in line with that.

marcosdiez · 2018-05-17T20:19:48Z

Sorry @skorokithakis but that's completely different. What Chrome does is open the tab in itself and mirror what it has rendered to Chromecast.
What I plan to do is tell Chromecast to open a URL. There is no mirroring. The website is even accessed using Chromecast's own user agent.

skorokithakis · 2018-05-18T01:19:37Z

Hmm, yes, good point. Do you want to make the changes first, and decide on the name later? Renaming the command is a one-line change anyway, and I think we have consensus that this is a useful feature to include.

marcosdiez · 2018-05-18T04:31:59Z

The idea of making this a separate URL really made everything easier.

I did not like to have to change StreamInfo and the changes I did to setup_cast were minimal.

I also made sure we can reload URLs, something that was previously not possible.

We now have only to decide how the command will be called.

I let to you guys decide. I am happy with the functionality!

Thanks for the code review

theychx

This is working nicely, although it's a real shame we can't utilize the reload functionality of the controller.

theychx · 2018-05-18T08:10:31Z

catt/controllers.py

@@ -334,11 +343,13 @@ def _prep_app(self):
    def _prep_control(self):
        """Make shure chromecast is in an active state."""

+        if self._cast.app_id == DASHCAST_APP_ID:
+            return


This is a hack, please remove.

This is not a hack. With this line, it's possible to type catt info and see the title of the current loaded webpage. Without it, we get a boring Error: Nothing is currently playing..

It also allows the user to use catt ffwd 60 while a webpage is loaded, and then get the irrelevant error message Error: Stream is not seekable.
info and status displays video/audio info (as descibed in the command helptexts).

ok... you convinced me!

theychx · 2018-05-18T08:11:17Z

catt/controllers.py

        if self._cast.app_id == BACKDROP_APP_ID or not self._cast.app_id:
            raise CattCastError("Chromecast is inactive.")
        self._cast.media_controller.block_until_active(1.0)
        if self._cast.media_controller.status.player_state in ["UNKNOWN", "IDLE"]:
-            raise CattCastError("Nothing is currently playing.")
+            raise CattCastError("Nothing is currently playing. [APP_ID={}]".format(self._cast.app_id))


The app id is not useful to the regular user (not in this context at least).

That was useful to me, but I don't mind removing it.

theychx · 2018-05-18T09:34:17Z

catt/controllers.py

+        self._load_url(url)  # this will actually take us to the "Waiting for URL" screen
+        while not self._is_waiting_for_url():
+            time.sleep(1)
+        self._load_url(url)  # this will finally load the URL


I have found a way to make this work without going through these hoops:
Override _prep_app in your dashcast controller, so that it always kills the running app.
Add a backdrop_ready flag to CastStatusListener.
In the overridden prep_app, kill the running app and wait for the flag to be set.

theychx · 2018-05-18T08:31:02Z

catt/cli.py

@@ -181,6 +181,15 @@ def process_subtitle(ctx, param, value):
    return value


+@cli.command(short_help="Makes your Chromecast display any webpage")


Please use the imperative mood, like in the other command help texts.

theychx · 2018-05-18T09:36:49Z

catt/controllers.py

@@ -110,8 +116,11 @@ def setup_cast(device_name, video_url=None, prep=None, controller=None):



We need to fail here, if the type of the chosen cast device is not in supported_device_types.

Instead of failing, I decided to just print a warning for two reasons:

this is what catt currently do for other stuff

warnings are more future proof than failures

A warning is not going to cut it. The warning logic in setup_cast is currently used to make sure that the default cast controller is returned to cli.py when the selected device is not supported by a custom controller. In cli.py there is then a fallback when trying to cast a playlist with the default castcontroller. With your functionality, there is no fallback, so we need to fail.
(and you print a warning, and then return the dashcast castcontroller anyway.)

theychx

A few more notes.

theychx · 2018-05-20T07:52:27Z

catt/controllers.py

@@ -110,8 +116,11 @@ def setup_cast(device_name, video_url=None, prep=None, controller=None):



A warning is not going to cut it. The warning logic in setup_cast is currently used to make sure that the default cast controller is returned to cli.py when the selected device is not supported by a custom controller. In cli.py there is then a fallback when trying to cast a playlist with the default castcontroller. With your functionality, there is no fallback, so we need to fail.
(and you print a warning, and then return the dashcast castcontroller anyway.)

theychx · 2018-05-20T08:02:39Z

catt/controllers.py

        self.app_ready = threading.Event()
+        self.set_app_id(app_id, active_app_id)


Please just add a backdrop_ready flag of the threading.Event kind, and then initiate it like app_ready, and expand the logic in new_cast_status so the flag is set/cleared in the same manner as app_ready.

theychx · 2018-05-20T08:10:15Z

catt/controllers.py

+            self._cast_listener.set_app_id(DASHCAST_APP_ID)
+
+        self._cast.start_app(DASHCAST_APP_ID)
+        self._cast_listener.app_ready.wait()


I imagine _prep_app being as simple as this:

self.kill() self._cast_listener.backdrop_ready.wait() self._cast.start_app(self._cast_listener.app_id) self._cast_listener.app_ready.wait()

marcosdiez · 2018-05-20T20:08:20Z

@theychx I did all the changes you requested. _prep_app has one extra line than what you had in mind, because there is no need to kill the current app unless it's dashcast itself.

I believe it should be ready to be merged now!

theychx

One more note.

theychx · 2018-05-20T20:56:39Z

catt/controllers.py

        self.app_ready = threading.Event()
+        self.set_app_id(app_id, active_app_id)


What I wanted was this:

class CastStatusListener: def __init__(self, app_id, active_app_id): self.app_id = app_id self.app_ready = threading.Event() self.backdrop_ready = threading.Event() if active_app_id == app_id: self.app_ready.set() elif active_app_id == BACKDROP_APP_ID: self.backdrop_ready.set() def new_cast_status(self, status): if status.app_id == self.app_id: self.app_ready.set() self.backdrop_ready.clear() elif status.app_id == BACKDROP_APP_ID: self.backdrop_ready.set() self.app_ready.clear() else: self.app_ready.clear() self.backdrop_ready.clear()

You might want to know that after the listener has been registered in the constructor of the CastController baseclass, pychromecast automatically calls the new_cast_status method of the listener object whenever there is a change.
Also, threading.Event flags are initially set to false.

marcosdiez · 2018-05-21T01:29:25Z

I see. It all makes sense. By the way I was reading pychromecast's source code and I think there is a way to bypass backdrop completelly. I will test tomorrow after work

marcosdiez · 2018-05-21T11:55:23Z

@theychx now it's G R E A T !
No more going to backdrop and going back to DashCast! Pages load a few seconds faster because of that.
Of course I did not like the internal API call I had to do. But that's what we have for today.

I will soon write a patch to pychromecast.

skorokithakis · 2018-05-21T11:56:23Z

That's pretty fantastic. Hopefully your patch will stabilize the API so it doesn't change from under our feet.

marcosdiez · 2018-05-21T12:06:52Z

My PR to pychromecast is here: home-assistant-libs/pychromecast#237
Now it's up to them to merge it!

theychx

Great find!

theychx · 2018-05-21T18:05:28Z

catt/controllers.py

+        # becomes unresponsive after a website is loaded
+        self._cast.socket_client.receiver_controller.launch_app(self._cast_listener.app_id, force_launch=True)
+        self._cast.start_app(self._cast_listener.app_id)
+        self._cast_listener.app_ready.wait()


If you clear the app_ready flag before force launching the app, it would not be necessary to modify CastStatusListener or use the class variable. Also the self._cast.start_app call is not needed anymore.

Wouldn't not modifying CastStatusListener make things rely on a race condition ?
The modified version of CastStatusListener guarantees that the app is only ready when it should be.

The DashCast app has an "Application Is Starting" state in which it can not yet receive URLs: https://github.com/stestagg/dashcast/blob/master/receiver.html#L163

This is one of the reasons, actually, that I liked that catt info showed the state of the DashCast app.

Wouldn't not modifying CastStatusListener make things rely on a race condition ?

Hmm, I guess thats true, although it's extremely unlikely that this would ever be a real problem. I've tested my own suggestion and could not get it to fail.
I'm keen on the code needed to circumvent the odd behaviour of dashcast, being contained within the overridden _prep_app. And I am honestly not keen on the class variable and the use of it in CastStatusListener.

@skorokithakis I'm not completely sure I'm in the right here, any input?

I did not like the change either on CastStatusListener but either we leave things as is or we patch DashCast to behave as expected. Race conditions are must always be avoided because debugging them is a real pain. My vote is to do nothing and push the merge button.

I'll have to agree with @marcosdiez here, if eliminating a race condition means that the code will be a bit uglier, I can live with that, since it will be really hard to debug something like "Sometimes catt doesn't work" or "catt doesn't work on my new Chromecast".

I would, however, like to have comments here and on CastStatusListener explaining exactly which race condition this guards against, and how it does that. I can definitely see us coming back in a year, not remembering this discussion and saying "Oh we don't actually need this, we can just clear the app_ready flag" and fall into the race condition trap.

Also, it looks like your suggestion sets the app_ready flag if the app is not DashCast, but don't we need a way to tell that DashCast is ready? Sorry to be such a pain, I just don't understand the flow here exactly.

Yep, but he also implemented the extra status text check, and it is the necessity of this I am contesting.

@theychx Can we discuss somewhere more real-time? I've created a Gitter room and invited you there (you can just log in with Github).

@marcosdiez You're welcome too, of course.

Hey @marcosdiez, @theychx and I discussed this on Gitter, and we agree that eliminating the race condition makes the most sense. However, could you change the code in the check to this?:

def _is_app_ready(self, status): if status.app_id == self.app_id == DASHCAST_APP_ID: # Big-ass comment about why this is here here. return status.status_text == "Application ready" return status.app_id == self.app_id

That way, we get rid of the confusingly-named class variable that's only used in one place, and we have a comment on the special case explaining exactly why it happens.

Also, could you add a comment on the check in __init__ on the CastStatusListener about why the DashCast check is necessary? I think this is a good compromise between clearly documenting the special-case code and eliminating any potential race conditions.

skorokithakis · 2018-05-21T22:25:40Z

catt/controllers.py

+        # becomes unresponsive after a website is loaded
+        self._cast.socket_client.receiver_controller.launch_app(self._cast_listener.app_id, force_launch=True)
+        self._cast.start_app(self._cast_listener.app_id)
+        self._cast_listener.app_ready.wait()


I'll have to agree with @marcosdiez here, if eliminating a race condition means that the code will be a bit uglier, I can live with that, since it will be really hard to debug something like "Sometimes catt doesn't work" or "catt doesn't work on my new Chromecast".

I would, however, like to have comments here and on CastStatusListener explaining exactly which race condition this guards against, and how it does that. I can definitely see us coming back in a year, not remembering this discussion and saying "Oh we don't actually need this, we can just clear the app_ready flag" and fall into the race condition trap.

theychx

@marcosdiez Thank you for this contribution! We can merge once you have adopted @skorokithakis final suggestion.

marcosdiez · 2018-05-23T02:09:18Z

Hi guys,
sorry for not participating in the Gitter discussion. I was busy with my real job :/

@theychx you were right, self._cast_listener.app_ready.wait() was redundant and therefore was removed

@skorokithakis Yet on the race condition topic, I did check on the YouTube and Default Media Player app and they both already start in a ready status.

I did describe the race condition in the code as best as I could. Feel free to request further changes or modify yourself.

And by the way, the triple comparison was a nice trick that I was not aware of. Thanks for teaching me that one!

skorokithakis · 2018-05-23T06:33:34Z

Thank you, @marcosdiez! The comment looks good to me. However, you seem to have removed start_app() instead of wait()? Is that a mistake?

theychx · 2018-05-23T06:41:22Z

It's cool. He just quoted the wrong line, the code is correct.

skorokithakis · 2018-05-23T06:43:49Z

Ah alright, great. Are you good to merge this, @theychx?

theychx · 2018-05-23T06:46:07Z

Yep

skorokithakis · 2018-05-23T06:47:12Z

Fantastic! There are a few conflicts, it seems, which is a bit odd since we didn't do much to master. Is either of you at a computer and able to resolve those?

theychx · 2018-05-23T06:49:40Z

It was just the gitter badge.

skorokithakis · 2018-05-23T06:49:40Z

Thanks, I realized there was a button but it didn't show on mobile.

skorokithakis · 2018-05-23T07:04:33Z

Merged, thank you both!

marcosdiez changed the title ~~dashcast~~ Support to Cast Any Website to ChromeCast (via DashCast) May 14, 2018

marcosdiez changed the title ~~Support to Cast Any Website to ChromeCast (via DashCast)~~ Support to Cast Any Website to Chromecast (via DashCast) May 14, 2018

Add Support for DashCast

962f062

marcosdiez force-pushed the dashcast branch from 0c2d7c1 to 962f062 Compare May 17, 2018 12:28

One commit solely honoring flake8

0a9b7b1

it works but the code looks awful

0cfbb0c

marcosdiez added 5 commits May 18, 2018 03:47

it's looking better

b8a2329

the code looks good now

17c45e9

oops

2bfa6e9

minor changes to setup_cast

c87839f

minor comment, for clarity

2fc92f8

theychx requested changes May 18, 2018

View reviewed changes

marcosdiez added 3 commits May 20, 2018 00:35

we now make good use of CastStatusListener

65e29c2

_prep_control will again consider a webpage an inactive chromecast

7ccf207

better way to use CastStatusListener

a922b96

updated readme and comments

54bdc0a

theychx requested changes May 20, 2018

View reviewed changes

final changes?

1af34b8

small changes

f4e283a

theychx requested changes May 20, 2018

View reviewed changes

DashCast does not need to fiddle with backdrop anymore

479c2ed

Minor text fixes

54e2bb5

remove unused lines

9a87bae

theychx requested changes May 21, 2018

View reviewed changes

skorokithakis requested changes May 21, 2018

View reviewed changes

theychx approved these changes May 22, 2018

View reviewed changes

Big-ass comment about why this is here here.

f173373

skorokithakis approved these changes May 23, 2018

View reviewed changes

Merge branch 'master' into dashcast

6cabfe6

skorokithakis merged commit b66f205 into skorokithakis:master May 23, 2018

theychx mentioned this pull request Sep 6, 2018

Dashcast update #123

Merged

		@@ -181,6 +181,15 @@ def process_subtitle(ctx, param, value):
		return value


		@cli.command(short_help="Makes your Chromecast display any webpage")

		@@ -110,8 +116,11 @@ def setup_cast(device_name, video_url=None, prep=None, controller=None):

		self.app_ready = threading.Event()
		self.set_app_id(app_id, active_app_id)

Support to Cast Any Website to Chromecast (via DashCast) #102

Support to Cast Any Website to Chromecast (via DashCast) #102

Conversation

marcosdiez commented May 14, 2018 • edited Loading

skorokithakis commented May 17, 2018

mdiez-modus commented May 17, 2018

skorokithakis commented May 17, 2018 • edited Loading

mdiez-modus commented May 17, 2018

theychx commented May 17, 2018

skorokithakis commented May 17, 2018

theychx commented May 17, 2018

skorokithakis commented May 17, 2018

theychx commented May 17, 2018

skorokithakis commented May 17, 2018

mdiez-modus commented May 17, 2018

skorokithakis commented May 17, 2018

marcosdiez commented May 17, 2018

skorokithakis commented May 18, 2018

marcosdiez commented May 18, 2018

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

theychx May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theychx May 18, 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

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

marcosdiez commented May 20, 2018

theychx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosdiez commented May 21, 2018

marcosdiez commented May 21, 2018

skorokithakis commented May 21, 2018

marcosdiez commented May 21, 2018

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

skorokithakis May 21, 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

skorokithakis May 22, 2018 • edited Loading

Choose a reason for hiding this comment

skorokithakis May 21, 2018 • edited Loading

Choose a reason for hiding this comment

theychx left a comment

Choose a reason for hiding this comment

marcosdiez commented May 23, 2018

skorokithakis commented May 23, 2018

theychx commented May 23, 2018

skorokithakis commented May 23, 2018

theychx commented May 23, 2018

skorokithakis commented May 23, 2018

theychx commented May 23, 2018

skorokithakis commented May 23, 2018

skorokithakis commented May 23, 2018

marcosdiez commented May 14, 2018 •

edited

Loading

skorokithakis commented May 17, 2018 •

edited

Loading

theychx May 18, 2018 •

edited

Loading

theychx May 18, 2018 •

edited

Loading

skorokithakis May 21, 2018 •

edited

Loading

skorokithakis May 22, 2018 •

edited

Loading

skorokithakis May 21, 2018 •

edited

Loading