Send videos from Fennec to Firefox OS via Presentation API

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jhao, Assigned: schien)

Tracking

unspecified
Firefox 46
x86_64
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox47 fixed)

Details

(Whiteboard: [ft:conndevices][fennec])

Attachments

(1 attachment, 12 obsolete attachments)

14.75 KB, patch
schien
: review+
Details | Diff | Splinter Review
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.
From what SC told me and what I've studied, the starting point
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
Posted patch demo-device-protocol.patch (obsolete) — Splinter Review
You can use this patch to test device add/remove/list.
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.
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.
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,...
Attachment #8560512 - Attachment is obsolete: true
Posted patch Send URL + play/pause control (obsolete) — Splinter Review
Now play/pause controls are working. What's left is to detect the end of stream.
Attachment #8561998 - Attachment is obsolete: true
Posted patch Ready to demo (obsolete) — Splinter Review
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.
Attachment #8563185 - Attachment is obsolete: true
Posted patch Tested successfully on TV (obsolete) — Splinter Review
This patch is tested on TV. The previous one is not the latest.
Attachment #8563920 - Attachment is obsolete: true

Updated

4 years ago
blocking-b2g: --- → 2.5+
Whiteboard: [ft:conndevices]

Updated

4 years ago
Whiteboard: [ft:conndevices] → [ft:conndevices][fennec]
Pending UX for TV 2.5 design on Fennec.

Updated

4 years ago
Flags: needinfo?(tchen)
Hi, this is initial version spec for video cast. Still need review by Fennec team.
https://drive.google.com/open?id=0B4dMhI4hp32OZDBaR2FtMEhTb3c
Flags: needinfo?(tchen)

Updated

4 years ago
Flags: needinfo?(jocheng)
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.
Not a blocker for FxOS for phone.
blocking-b2g: 2.5+ → ---
tracking-b2g: --- → +
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!
Flags: needinfo?(jocheng) → needinfo?(mpotharaju)
Sure Josh. I put the blocking 2.5+ flag back. Will revise my query accordingly. 

Thanks
blocking-b2g: --- → 2.5+
tracking-b2g: + → ---
Flags: needinfo?(mpotharaju)
No longer depends on: 1206223
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

Updated

4 years ago
Priority: -- → P3
I'll follow up the review process from now. Thanks to @jhao for providing the initial patch!
Assignee: jhao → schien
(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!

Updated

4 years ago
Status: NEW → ASSIGNED

Updated

4 years ago
No longer blocks: 1187861

Updated

4 years ago
Priority: P3 → P1

Updated

4 years ago
Blocks: TV_P1
rebase and clean up previous patch. will do code reorganization in next version.
Attachment #8559614 - Attachment is obsolete: true
Attachment #8564021 - Attachment is obsolete: true
Attachment #8657035 - Attachment is obsolete: true

Updated

4 years ago
blocking-b2g: 2.5+ → ---

Updated

4 years ago
No longer blocks: TV_P1
latest patch for TV 2.5 integration test.
Attachment #8674163 - Attachment is obsolete: true
Merge presentation device into CastingApps.js architecture.
Attachment #8679420 - Attachment is obsolete: true
Attachment #8687160 - Flags: feedback?(mark.finkle)

Updated

4 years ago
Target Milestone: --- → Firefox 46
Attachment #8687160 - Attachment is obsolete: true
Attachment #8687160 - Flags: feedback?(mark.finkle)
Attachment #8707377 - Flags: review?(mark.finkle)
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!

Updated

3 years ago
Blocks: TV_P1

Updated

3 years ago
blocking-b2g: --- → 2.5+
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!
Hi Mark,
Could you help to review the patch when you are available? 
Thanks!
Flags: needinfo?(mark.finkle)
@mfinkle, I've updated the patch and integrate the periodic discovery to SimpleServiceDiscovery.jsm.
Attachment #8707377 - Attachment is obsolete: true
Attachment #8707377 - Flags: review?(mark.finkle)
Attachment #8714271 - Flags: review?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
Attachment #8714271 - Flags: review?(mark.finkle) → review+
update according to review comment, carry r+.
Attachment #8714271 - Attachment is obsolete: true
Attachment #8715618 - Flags: review+

Comment 33

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/734225676b79
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.