1129785 - Send videos from Fennec to Firefox OS via Presentation API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect, P1)

Description

[Jonathan Hao (inactive) [:jhao]]

The connected device team plans to demonstrate the Presentation API in the upcoming MWC. One of the use cases will be sending videos from Fennec to the Panasonic TV which runs Firefox OS. The code for the demonstration may not be checked-in to the code base.

I am responsible for the Fennec part.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

From what SC told me and what I've studied, the starting point

Comment 2

[Jonathan Hao (inactive) [:jhao]]

From what SC told me and what I've studied (please correct me if I'm wrong), the starting point would be
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/CastingApps.js#582

In the prompt method, there will be a list of all the discovered devices to send to. We should get the available Firefox OS devices by https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationDeviceManager.idl

The interface of devices is https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationDevice.idl

We should add the Firefox OS devices to the list, and if the user selects one of them, we should start a session, and sends the video url and commands such as play, pause, seek through the session object.

Example: https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/fling-sender/js/fling_sender.js

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: demo-device-protocol.patch

You can use this patch to test device add/remove/list.

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Part 1: Show devices in prompt

Comment 5

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

Thanks for working to integrate the Presentation API into Fennec. I am not familiar with how the API is implemented, so bear with me.

Looks like the patches are injecting the Presentation API into the prompt display code in CastingApps.js, but I think it would be better to integrate the API as a new service, similar to how we integrated Chromecast (MediaPlayer) and other devices.

In the long run, it would be nice to replace SimpleServiceDiscovery with the Presentation API itself. In order to do that, we'd need to move all of the current device support (Roku, Chromecast, MatchStick and others into the Presentation API discovery. I don't know how to do that, but it kinda looks like your patch is making a "device" for MatchStick (I think), or maybe some other device.

The one thing I don't want to do is have two discovery services glued together in CastingApps.

Just some early feedback.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Hi Mark,

Thanks very much for your feedback. I totally agree with you that it would be a mess to have two discovery services.

For now we just want to have a working patch to demo, but if we want to land this feature someday, we'll definitely need to take some time to design the structure of service discovery.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Attachment: (WIP) Could play video on receiver

Besides showing the prompt in the last patch, I implemented the PresentationDevicePrompt required by Presentation API. When user chooses a device of Presentation API, a message will be sent to PresentationDevicePrompt to set the chosen device. Then call startSession and send the url over.

Now we can successfully send the url to a Flame running TV apps, and play successfully. What's left is to pause the video, and establish the control such as play/pause,...

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Attachment: Send URL + play/pause control

Now play/pause controls are working. What's left is to detect the end of stream.

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Attachment: Ready to demo

Did well sending to tablets. Haven't tested on TV.
Joe wanted to change the icon of Fennec to Nightly during demo, so there should be an |ac_add_options --with-branding=mobile/android/branding/nightly| in the mozconfig.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tested successfully on TV

This patch is tested on TV. The previous one is not the latest.

Comment 11

[Josh Cheng [:josh]]

Pending UX for TV 2.5 design on Fennec.

Comment 12

[Tori Chen [:tori]]

Hi, this is initial version spec for video cast. Still need review by Fennec team.
https://drive.google.com/open?id=0B4dMhI4hp32OZDBaR2FtMEhTb3c

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: patch-all-for-presentation-fennec-sender.patch

This is a patch for running Fennec as a sender to send presentation request to a receiver.
Because it contains many WIP patches, this may become invalid in a short time. We are using it for experiment.

Also, since Fennec is not able to get IP of itself, we need to hardcode it for now.

Comment 14

[Mahendra Potharaju [:mahe]]

Not a blocker for FxOS for phone.

Comment 15

[Josh Cheng [:josh]]

Hi Mahe,
TV team need blocking-2.5+ to identify TV blockers for 2.5. Can you try revise your triage query to remove TV bugs which with whiteboard '[ft:conndevices]'?
I add all TV bugs with it on whiteboard. FYR.
Thanks!

Comment 16

[Mahendra Potharaju [:mahe]]

Sure Josh. I put the blocking 2.5+ flag back. Will revise my query accordingly. 

Thanks

Comment 17

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

Comment on attachment 8657035 [details] [diff] [review]
patch-all-for-presentation-fennec-sender.patch

I can't comment too much on the core Presentation code, but I would like to see the code you added to CastingApps.js removed (as much as possible) and placed in a JSM like RokuApp.jsm (and recently removed, MatchstickApp.jsm). We try to encapsulate as much of the device/protocol specific code into modules.

You could try to make a PresentationApp.jsm that wraps the presentation API notifications and the play/pause methods. That should make the code "pluggable" into the existing framework.

MediaPlayerApp.jsm is an example of wrapping a very different type of API (Chromecast on Android) into the current casting framework. It's a very lightweight file and let's Android do most of the work.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/MediaPlayerApp.jsm

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'll follow up the review process from now. Thanks to @jhao for providing the initial patch!

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #18)
> I'll follow up the review process from now. Thanks to @jhao for providing
> the initial patch!

Thank you SC for taking this!

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-presentation-device.patch

rebase and clean up previous patch. will do code reorganization in next version.

Comment 21

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-presentation-device.patch

latest patch for TV 2.5 integration test.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: generalize-device-discovery.patch

Merge presentation device into CastingApps.js architecture.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-video-sharing-via-presentation-api.patch

Comment 24

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

Comment on attachment 8707377 [details] [diff] [review]
support-video-sharing-via-presentation-api.patch


>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+// Enable Presentation API
>+pref("dom.presentation.enabled", true);
>+pref("dom.presentation.discovery.enabled", true);

What is the impact on networking and power from enabling these features?

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js


>+var fxOSTVDevice = {
>+  id: "app://fling-player.gaiamobile.org",
>+  target: "app://fling-player.gaiamobile.org/index.html",

This is a wrapper around the "presentation API", which is very generic, but it only works with a fxos TV?

>+  factory: function(aService) {
>+    Cu.import("resource://gre/modules/PresentationApp.jsm");
>+    let request = new window.PresentationRequest(this.target);
>+    return new PresentationApp(aService, request);
>+  },
>+  init: function() {
>+    Services.obs.addObserver(this, "presentation-device-change", false);
>+  },

This part looks good. Fits the expected model of a shim.

>+  startSearch: function(interval) {
>+    if (this._intervalId) {
>+      this.stopSearch();
>+    }
>+
>+    (function _search() {
>+      window.navigator.mozPresentationDeviceInfo.forceDiscovery();
>+
>+      // need to update the lastPing time for known device.
>+      window.navigator.mozPresentationDeviceInfo.getAll()
>+      .then(function(devices) {
>+        for (let device of devices) {
>+          let service = fxOSTVDevice.toService(device);
>+          SimpleServiceDiscovery.addService(service);
>+        }
>+        fxOSTVDevice._intervalId = window.setTimeout(_search, interval);
>+      });
>+    })();
>+
>+  },
>+  stopSearch: function() {
>+    window.clearTimeout(this._intervalId);
>+    delete this._intervalId;
>+  },
>+  observe: function(subject, topic, data) {
>+    let device = subject.QueryInterface(Ci.nsIPresentationDevice);
>+    let service = this.toService(device);
>+    switch (data) {
>+      case "add":
>+        SimpleServiceDiscovery.addService(service);
>+        break;
>+      case "update":
>+        SimpleServiceDiscovery.updateService(service);
>+        break;
>+      case "remove":
>+        if(SimpleServiceDiscovery.findServiceForID(device.id)) {
>+          SimpleServiceDiscovery.removeService(device.id);
>+        }
>+        break;
>+    }
>+  },

I am less in love with this part. It look more like a peer to SimpleServiceDiscovery. It seems a bit heavyweight for the shim.

> var CastingApps = {

>     // MediaPlayerDevice will notify us any time the native device list changes.
>     mediaPlayerDevice.init();
>     SimpleServiceDiscovery.registerDevice(mediaPlayerDevice);
> 
>+    // Presentation Device will notify us any time the available device list changes.
>+    fxOSTVDevice.init();
>+    SimpleServiceDiscovery.registerDevice(fxOSTVDevice);
>+

Again, this makes sense.

>     // Search for devices continuously
>     SimpleServiceDiscovery.search(this._interval);
>+    fxOSTVDevice.startSearch(this._interval);

This duplication worries me. 

What if we added a way to add other discovery code to SimpleServiceDiscovery?

Something like SimpleServiceDiscovery.addDiscovery(JS Object that has startSearch(interval) and stopSearch() methods)

Then SimpleServiceDiscovery could make sure polling is coordinated and call startSearch and stopSearch at the right times.

let me know if you think this is reasonable. I could be swayed to land the code as-is, and make changes in a followup too.


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      InitLater(() => Cu.import("resource://gre/modules/PresentationDeviceInfoManager.jsm"));

Can this also be imported in CastingApps where you import "resource://gre/modules/PresentationApp.jsm" ?

>diff --git a/toolkit/modules/secondscreen/PresentationApp.jsm b/toolkit/modules/secondscreen/PresentationApp.jsm

This file looks great!

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks for the review and here is my reply.

(In reply to Mark Finkle (:mfinkle) from comment #24)
> Comment on attachment 8707377 [details] [diff] [review]
> support-video-sharing-via-presentation-api.patch
> 
> 
> >diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js
> 
> >+// Enable Presentation API
> >+pref("dom.presentation.enabled", true);
> >+pref("dom.presentation.discovery.enabled", true);
> 
> What is the impact on networking and power from enabling these features?
I'm going to provide a JS implemented mDNS service, which lifecycle can be controlled by gecko (see bug 1241368).
> 
> >diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js
> 
> 
> >+var fxOSTVDevice = {
> >+  id: "app://fling-player.gaiamobile.org",
> >+  target: "app://fling-player.gaiamobile.org/index.html",
> 
> This is a wrapper around the "presentation API", which is very generic, but
> it only works with a fxos TV?
Presentation API is generic but it still need a corresponding receiver app on TV side, so it is fxos TV only.
> 
> >+  factory: function(aService) {
> >+    Cu.import("resource://gre/modules/PresentationApp.jsm");
> >+    let request = new window.PresentationRequest(this.target);
> >+    return new PresentationApp(aService, request);
> >+  },
> >+  init: function() {
> >+    Services.obs.addObserver(this, "presentation-device-change", false);
> >+  },
> 
> This part looks good. Fits the expected model of a shim.
> 
> >+  startSearch: function(interval) {
> >+    if (this._intervalId) {
> >+      this.stopSearch();
> >+    }
> >+
> >+    (function _search() {
> >+      window.navigator.mozPresentationDeviceInfo.forceDiscovery();
> >+
> >+      // need to update the lastPing time for known device.
> >+      window.navigator.mozPresentationDeviceInfo.getAll()
> >+      .then(function(devices) {
> >+        for (let device of devices) {
> >+          let service = fxOSTVDevice.toService(device);
> >+          SimpleServiceDiscovery.addService(service);
> >+        }
> >+        fxOSTVDevice._intervalId = window.setTimeout(_search, interval);
> >+      });
> >+    })();
> >+
> >+  },
> >+  stopSearch: function() {
> >+    window.clearTimeout(this._intervalId);
> >+    delete this._intervalId;
> >+  },
> >+  observe: function(subject, topic, data) {
> >+    let device = subject.QueryInterface(Ci.nsIPresentationDevice);
> >+    let service = this.toService(device);
> >+    switch (data) {
> >+      case "add":
> >+        SimpleServiceDiscovery.addService(service);
> >+        break;
> >+      case "update":
> >+        SimpleServiceDiscovery.updateService(service);
> >+        break;
> >+      case "remove":
> >+        if(SimpleServiceDiscovery.findServiceForID(device.id)) {
> >+          SimpleServiceDiscovery.removeService(device.id);
> >+        }
> >+        break;
> >+    }
> >+  },
> 
> I am less in love with this part. It look more like a peer to
> SimpleServiceDiscovery. It seems a bit heavyweight for the shim.
> 
> > var CastingApps = {
> 
> >     // MediaPlayerDevice will notify us any time the native device list changes.
> >     mediaPlayerDevice.init();
> >     SimpleServiceDiscovery.registerDevice(mediaPlayerDevice);
> > 
> >+    // Presentation Device will notify us any time the available device list changes.
> >+    fxOSTVDevice.init();
> >+    SimpleServiceDiscovery.registerDevice(fxOSTVDevice);
> >+
> 
> Again, this makes sense.
> 
> >     // Search for devices continuously
> >     SimpleServiceDiscovery.search(this._interval);
> >+    fxOSTVDevice.startSearch(this._interval);
> 
> This duplication worries me. 
> 
> What if we added a way to add other discovery code to SimpleServiceDiscovery?
> 
> Something like SimpleServiceDiscovery.addDiscovery(JS Object that has
> startSearch(interval) and stopSearch() methods)
> 
> Then SimpleServiceDiscovery could make sure polling is coordinated and call
> startSearch and stopSearch at the right times.
> 
> let me know if you think this is reasonable. I could be swayed to land the
> code as-is, and make changes in a followup too.
I was worried about making SimpleServiceDiscovery do thing more than SSDP protocol. I can do it in a follow-up bug if you think the current patch is in land-able quality.
> 
> 
> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+      InitLater(() => Cu.import("resource://gre/modules/PresentationDeviceInfoManager.jsm"));
> 
> Can this also be imported in CastingApps where you import
> "resource://gre/modules/PresentationApp.jsm" ?
This JSM is the backend for Presentation API but not for video sharing. I'll suggest to leave it here. Is any other reason you suggest to move it to CastingApps.js?
> 
> >diff --git a/toolkit/modules/secondscreen/PresentationApp.jsm b/toolkit/modules/secondscreen/PresentationApp.jsm
> 
> This file looks great!

Comment 26

[Josh Cheng [:josh]]

Hi Mark,
Could you help to review the patch when you are available? 
Thanks!

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-video-sharing-via-presentation-api.patch

@mfinkle, I've updated the patch and integrate the periodic discovery to SimpleServiceDiscovery.jsm.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c6efe1e48f2

Comment 29

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

Comment on attachment 8714271 [details] [diff] [review]
support-video-sharing-via-presentation-api.patch


>diff --git a/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm b/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm

>+  _discoverys: [],

>+  addExternalDiscovery: function(discovery) {
>+    this._discoverys.push(discovery);
>+  },
>+
>+  _startExternalDiscovery: function() {
>+    for (let discovery of this._discoverys) {
>+      discovery.startDiscovery();
>+    }
>+  },
>+
>+  _stopExternalDiscovery: function() {
>+    for (let discovery of this._discoverys) {
>+      discovery.stopDiscovery();
>+    }
>+  },
> }

Let's rename _discoverys -> _discoveryMethods


----


Looks good. Thanks for making the changes.

We should think about refactoring SSDP into a more generic discovery service as we add new types of discovery. We could even consider renaming SimpleServiceDiscovery.jsm to ServiceDiscovery.jsm and pull out the SSDP protocol code into a separate JSM. ServiceDiscovery.jsm could auto-register mDNS and SSDP itself. Anyway, that's a different bug.

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: support-video-sharing-via-presentation-api.patch

update according to review comment, carry r+.

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

File bug 1245729 as the follow-up of comment #29.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/734225676b79

Comment 33

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/734225676b79