1048335 - Chromecast support for Fennec tab mirroring

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

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

Attachment: chromecast_server.patch

Moving the chromecast server patch over from bug 1037015 to get review. Ideally we should host this somewhere other than people.m.o as well

Comment 1

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

Comment on attachment 8467122 [details] [diff] [review]
chromecast_server.patch

Given my PTO over the next 2 weeks, I am passing this to Wes to take a look. I know very little about WebRTC anyway.

Beside having Wes look at some of the Chromecast behavior, we really need UX involved too. Anthony did the UX on the Roku app, so it might be a good idea to involve him in the Chromecast app too.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sounds good to me :)

Comment 3

[Alan K [:ack]]

Comment on attachment 8467122 [details] [diff] [review]
chromecast_server.patch

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

There are a bunch of console.logs and code that's commented out. Should be fixed before landing.

::: adapter.js
@@ +1,1 @@
> +var RTCPeerConnection = null;

A comment here about where this file came from would be useful, in case it needs to be updated in the future.

::: chromecast.html
@@ +48,5 @@
> +  document.getElementById("sender_inst").style.display = "none";
> +  document.getElementById("receiver_inst").style.display = "block";
> +  document.getElementById("header").firstChild.innerHTML = "TabCast: You are the reciever";
> +
> +  var extra = {};

var extra = {
  video: true,
  audio: false,
  demo: true,
  turn_only: false,
};

::: mozdemo.js
@@ +28,5 @@
> +}
> +
> +
> +
> +function ajax(params) {

Can this have a better name?

@@ +52,5 @@
> +var CallingClient = function(config_, username, peer, divs, start_call, other_params) {
> +    console.log("Calling client constructor");
> +    var poll_timeout = 1000; // ms
> +    var config = config_;
> +    var state = "INIT";

Comment needed; What are the other states?

@@ +82,5 @@
> +		    process_offer(sdp);
> +	        } else if (sdp.type === "answer") {
> +		    process_answer(sdp);
> +	        }
> +	    } else {

Test for sdp.candidate here?

@@ +155,5 @@
> +
> +    var set_remote_answer_success= function() {
> +    	log("Successfully applied answer.");
> +        if (other_params.demo) {
> +//            divs.remote_video.mozRequestFullScreen();

Why is this commented out?

@@ +248,5 @@
> +        // Start polling
> +        poll();
> +    };
> +
> +    var config = {};

var config = ... on line 55 above?

@@ +281,5 @@
> +    
> +    log("Calling client: user=" + username + " peer = " + peer);
> +//    var config = {};
> +    log("Config = " + JSON.stringify(config));
> +    var pc = new RTCPeerConnection(config, {});

Second (empty object) parameter is unnecessary, no?

@@ +287,5 @@
> +    if (pc) {
> +        log("Created PC object");
> +    }
> +    else {
> +        log("Failure creating Webrtc object");

Throw/return here? Probably don't want to call pc.onaddstream() below.

@@ +304,5 @@
> +    // Start.
> +    log("Calling get user media");
> +    // Get the video stream
> +    
> +    var params = {};

var params = {
  video: extra.video ? true : false,
  audio: extra.audio ? true : false,
  fake: extra.fake ? true : false,
};

@@ +317,5 @@
> +    }
> +
> +    if (start_call) {
> +    
> +    getUserMedia(params, function(stream){

Indent branch code block for clarity.

Comment 4

[Darren Newman]

Is Mirror Tab broken at the moment? Using latest Nightly for Android. Tried on my Nexus 7 and Nexus 10 both running Android 4.4.2. Chromecast gives blank white screen, Roku does nothing. Both still workm fine casting videos from Nightly.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8467122 [details] [diff] [review]
chromecast_server.patch

Alan reviewed this. I'm not going to. You want a look finkle?

Comment 6

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

Comment on attachment 8467122 [details] [diff] [review]
chromecast_server.patch

>diff -uN a/adapter.js b/adapter.js

>+if (navigator.mozGetUserMedia) {
>+  console.log("This appears to be Firefox");

Do we care about any other browsers? If not, let's reduce the complexity.

>diff -uN a/chromecast.html b/chromecast.html

>+    <title>Example minimum receiver</title>

We could probably come up with a better title. Maybe even remove this.

>+  <body>
>+    <span>hello</span>
>+    <span style="color:red">world 5</span>
>+    <div id="header"><h2>TabCast: You are the sender</h2></div>
>+    <div id="status"></div>
>+    <div id="sender_inst">
>+      <div>Open this link in another tab or browser</div>
>+      <div> <a id="link">right click here</a></div>
>+      <div>That other tab or browser will have text in a red box that you'll paste here:</div>
>+      <textarea id="answer"></textarea>
>+      <button onclick="answer_click()">submit answer</button>
>+    </div>

Can we remove all this "sender" stuff?

>+  <video style="border:thin blue solid" id="video" width="320" height="240" autoplay="TRUE"></video><br/>

"TRUE" -> "true"

>+<div style="color:green; font-size: xx-small;" id="logwindow"></div>

Do we need a log window? Sounds useful for a demo, but probably not needed.

>+  //window.location.reload(true);

Remove it

>+  var me = "castee";
>+  var them = "caster";

Needed?

>+  if (true ) {
>+  extra.video=true;
>+  }
>+  if (false ) {
>+  extra.audio=true;
>+  }
>+  if (true ) {
>+  extra.demo=true;
>+  }
>+  if (false ) {
>+  extra.turn_only=true;
>+  }

Indents, but really... why are these hard coded? Remove the if blocks and just make the extra object in place:

var extra = {
  video: true,
  demo: true,  <-- really?
};

>+  var divs = {
>+  local_video :  null,
>+  remote_video : document.getElementById("video") ,
>+  };

Indents

>diff -uN a/debugstyle.css b/debugstyle.css

Remove this file since it doesn't seem to be used

>diff -uN a/mozdemo.js b/mozdemo.js

Best name ever? No!

>+function ui_log(msg) {
>+    color_log(msg, "green");
>+}
>+var color_log = function(msg, color) {
>+    var log = msg.replace("\\r\\n", "<br/>");
>+
>+    var d = document.createElement("div");
>+    d.style.color = color;
>+    d.innerHTML = log;
>+
>+    var node = document.getElementById("logwindow");
>+
>+    if (node.firstChild) {
>+      node.insertBefore(d, node.firstChild);
>+    } else {
>+      node.appendChild(d);
>+    }
>+};

Remove this logging stuff

>+}
>+
>+
>+
>+function ajax(params) {

One line is enough

>+    var set_remote_answer_success= function() {
>+    	log("Successfully applied answer.");
>+        if (other_params.demo) {
>+//            divs.remote_video.mozRequestFullScreen();
>+        }

Remove the block, and maybe the "demo" param?
>+	send_sdpdescription(sdp);
>+        if (other_params.demo) {
>+//            divs.remote_video.mozRequestFullScreen();
>+        }

Same

Comment 7

[Randall Barker [:rbarker]]

Attachment: chromecast server patch v2

Updated to address review comments.

Comment 8

[Randall Barker [:rbarker]]

Attachment: chromecast server patch v3

Addressed a few remaining review comments.

Comment 9

[Randall Barker [:rbarker]]

Attachment: chromecast server patch v4

Comment 10

[Randall Barker [:rbarker]]

Comment on attachment 8502592 [details] [diff] [review]
chromecast server patch v4

One of the fixes will require an update to fennec as well, I will post that patch next.

Comment 11

[Randall Barker [:rbarker]]

Code is currently hosted:

https://github.com/mozilla/chromecast-server

Comment 12

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

Comment on attachment 8502592 [details] [diff] [review]
chromecast server patch v4

>diff --git a/../empty/adapter.js b/adapter.js

>+var RTCPeerConnection = null;
>+var getUserMedia = null;

These two are only used once (RTCPeerConnection) and none (getUserMedia). Since this is only going to be run in a Chromecast, I'd be fine with going full "webkit" prefix. Up to you.

>diff --git a/../empty/player.css b/player.css

>+#header {
>+    display:none;
>+}

No "header" in the HTML anymore

>+html, body {
>+  margin:0;
>+  padding:0;
>+}

You could move this into the HTML file where a smal CSS block exists for HTML and BODY already

>+#remote{
>+     width:100%;
>+     margin-left:250px;
>+     margin-right:auto;
>+}
>+
>+#remote-caption{
>+     display:none;
>+}
>+
>+#local {
>+     display:none;
>+     width:0px;
>+}
>+
>+#logwindow{
>+     clear:both;
>+     display:none;
>+}
>+
>+#status{
>+     clear:both;
>+     display:none;
>+}

These do not exist in the HTML anymore

>+#video {
>+    width:100%;
>+    height:100%;
>+    margin-left: auto;
>+    margin-right: auto;
>+}

If you keep the HTML and BODY stuff in here, fine. If not, move the VIDEO block in the HTML file too.

Comment 13

[Randall Barker [:rbarker]]

Attachment: chromecast server patch v5

Addressed review comments.

Comment 14

[Randall Barker [:rbarker]]

Comment on attachment 8504155 [details] [diff] [review]
chromecast server patch v5

carry forward r+ from :mfinkle

Comment 15

[Alan K [:ack]]

Comment on attachment 8504155 [details] [diff] [review]
chromecast server patch v5

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

::: chromecast.html
@@ +33,5 @@
> +
> +window.onload = function() {
> +  try {
> +    // Uncomment to set logger level to DEBUG
> +    //cast.receiver.logger.setLevelValue(cast.receiver.LoggerLevel.DEBUG);

Can you set this by a query string parameter instead?

@@ +40,5 @@
> +      log("sender connected: " + JSON.stringify(event));
> +    }
> +    window.castReceiverManager.onReady = function(event) {
> +      log("on ready");
> +      try {

Why try block here?

::: player.js
@@ +1,3 @@
> +var log = function(msg) {
> +  // Uncomment to enable logging
> +  //console.log("LOG(CallingClient): " + msg);

Toggle this with a query string flag?

@@ +2,5 @@
> +  // Uncomment to enable logging
> +  //console.log("LOG(CallingClient): " + msg);
> +};
> +
> +var failure = function(x) {

Should errors be propagated back to code that consumes CallingClient?

@@ +10,5 @@
> +var CallingClient = function(divs) {
> +  log("Calling client constructor");
> +
> +  var config = {}
> +  config.iceServers = [];

Compress this into one line?

@@ +11,5 @@
> +  log("Calling client constructor");
> +
> +  var config = {}
> +  config.iceServers = [];
> +  config.iceServers.push({"url":"stun:stun.services.mozilla.com"});

What happens if stun.services.mozilla.com goes away?

@@ +42,5 @@
> +      return;
> +    }
> +    this.senderId = senderId;
> +    log("Received raw message " + msg);
> +    var data = JSON.parse(msg);

JSON.parse can throw if msg is not formatted correctly.

@@ +78,5 @@
> +  },
> +
> +  _setRemoteOfferSuccess: function() {
> +    log("Successfully applied offer");
> +    this.pc.createAnswer(this._createAnswerSuccess.bind(this), failure);

Does this need a bind?

@@ +81,5 @@
> +    log("Successfully applied offer");
> +    this.pc.createAnswer(this._createAnswerSuccess.bind(this), failure);
> +  },
> +
> +  _setLocalSuccessAnswer: function(sdp) {

Should this be _setLocalAnswerSuccess?

@@ +101,5 @@
> +  },
> +
> +  _createAnswerSuccess: function(sdp) {
> +    log("Successfully created answer " + JSON.stringify(sdp));
> +    this._filterNonrelayCandidates(sdp);

A comment about why we're filtering candidates here would be useful.

Comment 16

[Randall Barker [:rbarker]]

(In reply to Alan K [:ack] from comment #15)
> Comment on attachment 8504155 [details] [diff] [review]
> chromecast server patch v5
> 
> Review of attachment 8504155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chromecast.html
> @@ +33,5 @@
> > +
> > +window.onload = function() {
> > +  try {
> > +    // Uncomment to set logger level to DEBUG
> > +    //cast.receiver.logger.setLevelValue(cast.receiver.LoggerLevel.DEBUG);
> 
> Can you set this by a query string parameter instead?
> 

Unfortunately, the url sent to the chromecast device is controlled by Google.

> @@ +40,5 @@
> > +      log("sender connected: " + JSON.stringify(event));
> > +    }
> > +    window.castReceiverManager.onReady = function(event) {
> > +      log("on ready");
> > +      try {
> 
> Why try block here?
> 

Not sure, deleted.

> ::: player.js
> @@ +1,3 @@
> > +var log = function(msg) {
> > +  // Uncomment to enable logging
> > +  //console.log("LOG(CallingClient): " + msg);
> 
> Toggle this with a query string flag?
> 

Same problem as above.

> @@ +2,5 @@
> > +  // Uncomment to enable logging
> > +  //console.log("LOG(CallingClient): " + msg);
> > +};
> > +
> > +var failure = function(x) {
> 
> Should errors be propagated back to code that consumes CallingClient?
> 

Probably, this app obviously leaves much to be desired when it come to error handling.

> @@ +10,5 @@
> > +var CallingClient = function(divs) {
> > +  log("Calling client constructor");
> > +
> > +  var config = {}
> > +  config.iceServers = [];
> 
> Compress this into one line?
> 

done.

> @@ +11,5 @@
> > +  log("Calling client constructor");
> > +
> > +  var config = {}
> > +  config.iceServers = [];
> > +  config.iceServers.push({"url":"stun:stun.services.mozilla.com"});
> 
> What happens if stun.services.mozilla.com goes away?
> 

Since this all happens on the local area network, The stun server isn't required. However, the page/app can be updated at any time by us since it is not tied to a fennec release.

> @@ +42,5 @@
> > +      return;
> > +    }
> > +    this.senderId = senderId;
> > +    log("Received raw message " + msg);
> > +    var data = JSON.parse(msg);
> 
> JSON.parse can throw if msg is not formatted correctly.
> 

There is not much that can be done if it does throw, so letting the exception propagate up seems reasonable to me.

> @@ +78,5 @@
> > +  },
> > +
> > +  _setRemoteOfferSuccess: function() {
> > +    log("Successfully applied offer");
> > +    this.pc.createAnswer(this._createAnswerSuccess.bind(this), failure);
> 
> Does this need a bind?
> 

_createAnswerSuccess, I believe so. failure, no.

> @@ +81,5 @@
> > +    log("Successfully applied offer");
> > +    this.pc.createAnswer(this._createAnswerSuccess.bind(this), failure);
> > +  },
> > +
> > +  _setLocalSuccessAnswer: function(sdp) {
> 
> Should this be _setLocalAnswerSuccess?
> 

done.

> @@ +101,5 @@
> > +  },
> > +
> > +  _createAnswerSuccess: function(sdp) {
> > +    log("Successfully created answer " + JSON.stringify(sdp));
> > +    this._filterNonrelayCandidates(sdp);
> 
> A comment about why we're filtering candidates here would be useful.

I'm not sure why that filter code is there, I removed it.

Comment 17

[Randall Barker [:rbarker]]

Attachment: chromecast server patch v6

carry forward r+ from :ack and :mfinkle

Comment 18

[Richard Newman [:rnewman]]

I uploaded the current reviewed player.js and chromecast.html in Comment 17 to the CDN:

  http://mobile.cdn.mozilla.net/chromecast/1/chromecast.html

I don't know why, but HTTPS access to this doesn't work. I get a PermanentRedirect error:

  The bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.
  mobile-origin-cdn-mozilla-net.s3-us-west-1.amazonaws.com

I presume that we want HTTPS. Chris, any idea what's going on there?

Comment 19

[Chris Turra [:cturra]]

i think i know what's going on here! i had misconfigurated the https end-point on the cdn setup. this has now been updated, but will take a bit of time to propagate. will report back when that update is complete.

Comment 20

[Chris Turra [:cturra]]

should be good to go now \o/ sorry about the misconfiguration.

$ curl -I https://mobile.cdn.mozilla.net/chromecast/1/chromecast.html
HTTP/1.1 200 OK
Accept-Ranges: bytes
Content-Type: text/html
Date: Tue, 28 Oct 2014 19:36:07 GMT
Etag: "91d2c71b337ef52dcc57e69b3584e0ea"
Last-Modified: Tue, 28 Oct 2014 01:10:56 GMT
Server: ECAcc (pae/3706)
x-amz-id-2: /Ky920e0eZS5qL8wssj8DrGGXS85wCVZ+POS0gl4EQpAkXibuM0a6YHYL3QXFo5p
x-amz-request-id: E3A8B2BE1E21E6AD
X-Cache: HIT
Content-Length: 1673