1078796 - Matchstick code no longer works

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Wesley Johnston (:wesj)]

With the release of the Matchstick device, we don't work anymore :(

I've found two problems:

1.) We filter the device on server: null. The server returned from the discovery phases isn't null anymore. We should update or remove that check.

2.) The API's has been changed pretty dramatically to a WebSocket based one (at least, that is what is used here https://github.com/fling2flint/fling_player_firefox/). Simplified code:

let host = "10.252.34.215";
let port = "8008";

function startApp() {
   // These ports are hardcoded...
   var url = "http://" + host + ":" + 9001 + "/open_app?url=http://game.majijia.com/player/play.html?v=0";
   var wsIP = "ws://" + host + ":" + 9000 + "/sender";

   // Send an http request to start the app
   var req = new XMLHttpRequest();
   req.onload = function(data) {
     let ws = new WebSocket(wsIP);
     ws.onopen = function (evt) {
         // play/pause actions for playing and pausing
         ws.send('{"action":"info"}');
     };

     ws.onmessage = function (evt) {
       var msg = JSON.parse(evt.data);
       // When we get the connected info, start video playback
       if (msg.action === "receiver_online") {
             var data = {"src":["http://video.blendertestbuilds.de/download.blender.org/peach/trailer_480p.mov"], "action":"init"};
             ws.send(JSON.stringify(data));
         }
     };

     ws.onerror = function (evt) {
       console.log('Sender error occured: ' + evt.data);
     };   
   };
   req.open("get", url, true);
   req.send();
}

startApp();

We could actually probably use our Chroemcast app instead (and unify some code, and hopefully lessen future bustage...)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #0)
> 
> We could actually probably use our Chroemcast app instead (and unify some
> code, and hopefully lessen future bustage...)

I want to move the matchstick support to toolkit and support it on desktop.

Comment 2

[Caspy7]

Hopefully we can keep things as easy as possible for supporting other casting protocols in the future should they arise.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: nofilter-matchstick v0.1

Matchstick no longer advertises itself as a clone of Chromecast. We no longer need the "server" filter hack to identify it. We can use the | manufacturer: "openflint" | filter.

This patch switches to that filter and removes the "server" filter hack from SSDP.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fixup-matchstick v0.1

This patch completely replaces the previous MatchstickApp.jsm implementation. It might take a few passes, but I have been testing it for a few days to iron out the wrinkles. It's ready for review.

Notes:
1. Matchstick now uses a mostly normal DIAL discovery system. That means we can use simple XHR for the discovery and launch. This is the same pattern used in pre-Playservices Chromecast and other DIAL based JSMs.
2. Matchstick does support a built-in video player, appID = flintplayer. This patch uses it.
3. Once launched, Matchstick uses a documented WebSocket control API. The RemoteMedia wraps this to handle the basic control.
4. The protocol requires a heartbeat ping, otherwise it kills your session.

I used docs and sample code to help build this:
1. Spec: https://github.com/openflint/openflint.github.io/wiki/Flint%20Protocol%20Docs
2. flintplayer code: https://github.com/openflint/flint-player/blob/gh-pages/assets/player.js
3. MediaPlayer: https://github.com/openflint/flint-receiver-sdk/blob/gh-pages/v1/libs/mediaplayer.js
4. Receiver: https://github.com/openflint/flint-receiver-sdk/blob/d6b9ecbd66e09ed778596824d12bd1657702893a/v1/receiver.js
5. Server Daemon: https://github.com/openflint/flingd-coffee/blob/cf0f3f2b9fefa09fa0b270dbc47d81ea67e07779/lib/dial/handler/FlingAppControlHandler.coffee
6. Fabrice's example: https://github.com/fabricedesre/matchstick/blob/645b156a88e2dacd323a21c575b4c62bf56fb3ea/js/main.js

I also hooked the USB'd the Matchstick to be able to debug with logcat.

You do need to make sure your matchstick is updated to the most recent Firmware version. I used http://office.infthink.com/cm/matchstick/ and the protocol seemed to match what Fabrice's code was expecting.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Once this lands for Fennec, we can port it to Desktop without much work.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8534827 [details] [diff] [review]
fixup-matchstick v0.1

Review of attachment 8534827 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a good start. I'm a little nervous about the shutdown lifecycle. I'd like to get that fixed before we share this with desktop.

::: mobile/android/modules/MatchstickApp.jsm
@@ +98,5 @@
> +
> +    let data = {
> +      type: "launch",
> +      app_info: {
> +        url: "http://openflint.github.io/flint-player/player.html",

This should probably be a const.

@@ +135,5 @@
> +  },
> +
> +  remoteMedia: function remoteMedia(aCallback, aListener) {
> +    this.status((aStatus) => {
> +      if ("serviceURL" in aStatus && aStatus.serviceURL) {

This seems a bit redundant. i.e. I'd prefer if (aStatus.serviceURL)

@@ +139,5 @@
> +      if ("serviceURL" in aStatus && aStatus.serviceURL) {
> +        log("serviceURL: " + aStatus.serviceURL);
> +        if (aCallback) {
> +          aCallback(new RemoteMedia(aStatus.serviceURL, aListener, this));
> +        }

You could just return in here, and remove the else clause.

@@ +141,5 @@
> +        if (aCallback) {
> +          aCallback(new RemoteMedia(aStatus.serviceURL, aListener, this));
> +        }
> +      } else {
> +        log("no serviceURL")

Remove or make more descriptive. Same for other logging. Maybe log() should just prepend "Matchstick" or something to make it clear what these are coming from. Also ';'

@@ +195,5 @@
> +    xhr.addEventListener("load", () => {
> +      if (xhr.status == 200) {
> +        this.pingTimer.initWithCallback(() => {
> +          this._ping();
> +        }, this.app.pingInterval - 200, Ci.nsITimer.TYPE_ONE_SHOT);

What is this 200?

@@ +362,5 @@
>  
> +  onServerClose: function(aContext, aStatusCode, aReason) {
> +    // This will be fired from _teardown when we close the websocket, but it
> +    // can also be called for other internal socket failures and timeouts. We
> +    // make sure the _teardown bails on reentry.

Is this comment right? We don't close the websocket in teardown (we do in shutdown). If this happens though, it will apparently not be closed.... i.e. should we null it out here? We need these systems to actually be more robust to losing and re-establishing contact, so actually _teardown in here might be a bit extreme....

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

i suppose once this lands, i can grab a nightly fennec and make it work with matchstick again? :) thanks

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #6)

> > +      app_info: {
> > +        url: "http://openflint.github.io/flint-player/player.html",
> 
> This should probably be a const.

Done

> > +      if ("serviceURL" in aStatus && aStatus.serviceURL) {
> 
> This seems a bit redundant. i.e. I'd prefer if (aStatus.serviceURL)

Done

> > +        log("serviceURL: " + aStatus.serviceURL);
> > +        if (aCallback) {
> > +          aCallback(new RemoteMedia(aStatus.serviceURL, aListener, this));
> > +        }
> 
> You could just return in here, and remove the else clause.

Done. And I removed the debug logging.

> > +        this.pingTimer.initWithCallback(() => {
> > +          this._ping();
> > +        }, this.app.pingInterval - 200, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> What is this 200?

A simple "backoff" so we make sure we ping before the timeout happens and the device disconnects. I made it a const (PING_INTERVAL_BACKOFF).

> > +  onServerClose: function(aContext, aStatusCode, aReason) {
> > +    // This will be fired from _teardown when we close the websocket, but it
> > +    // can also be called for other internal socket failures and timeouts. We
> > +    // make sure the _teardown bails on reentry.
> 
> Is this comment right? We don't close the websocket in teardown (we do in
> shutdown). If this happens though, it will apparently not be closed.... i.e.
> should we null it out here? We need these systems to actually be more robust
> to losing and re-establishing contact, so actually _teardown in here might
> be a bit extreme....

The comment came from a copy/paste of other WIP patches. The code evolved in this patch to the point where the comment is no longer correct. _teardown does not close the websocket anymore. The shutdown path is cleaner in this code, so I updated the comment.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/76d9f3288e08
remote:   https://hg.mozilla.org/integration/fx-team/rev/96a80d4aa037

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76d9f3288e08
https://hg.mozilla.org/mozilla-central/rev/96a80d4aa037

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "Added support for casting to Matchstick devices"