Closed
Bug 1129785
Opened 10 years ago
Closed 9 years ago
Send videos from Fennec to Firefox OS via Presentation API
Categories
(Firefox for Android Graveyard :: Screencasting, defect, P1)
Tracking
(blocking-b2g:2.5+, firefox47 fixed)
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: jhao, Assigned: schien)
References
Details
(Whiteboard: [ft:conndevices][fennec])
Attachments
(1 file, 12 obsolete files)
(deleted),
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.
Reporter | ||
Comment 1•10 years ago
|
||
From what SC told me and what I've studied, the starting point
Reporter | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
You can use this patch to test device add/remove/list.
Reporter | ||
Comment 4•10 years ago
|
||
Comment 5•10 years ago
|
||
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.
Reporter | ||
Comment 6•10 years ago
|
||
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.
Reporter | ||
Comment 7•10 years ago
|
||
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
Reporter | ||
Comment 8•10 years ago
|
||
Now play/pause controls are working. What's left is to detect the end of stream.
Attachment #8561998 -
Attachment is obsolete: true
Reporter | ||
Comment 9•10 years ago
|
||
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
Reporter | ||
Comment 10•10 years ago
|
||
This patch is tested on TV. The previous one is not the latest.
Attachment #8563920 -
Attachment is obsolete: true
Updated•9 years ago
|
blocking-b2g: --- → 2.5+
Whiteboard: [ft:conndevices]
Updated•9 years ago
|
Whiteboard: [ft:conndevices] → [ft:conndevices][fennec]
Comment 11•9 years ago
|
||
Pending UX for TV 2.5 design on Fennec.
Updated•9 years ago
|
Flags: needinfo?(tchen)
Comment 12•9 years ago
|
||
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•9 years ago
|
Flags: needinfo?(jocheng)
Comment 13•9 years ago
|
||
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•9 years ago
|
||
Not a blocker for FxOS for phone.
blocking-b2g: 2.5+ → ---
tracking-b2g:
--- → +
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
Sure Josh. I put the blocking 2.5+ flag back. Will revise my query accordingly.
Thanks
Comment 17•9 years ago
|
||
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•9 years ago
|
Priority: -- → P3
Assignee | ||
Comment 18•9 years ago
|
||
I'll follow up the review process from now. Thanks to @jhao for providing the initial patch!
Assignee: jhao → schien
Comment 19•9 years ago
|
||
(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•9 years ago
|
Status: NEW → ASSIGNED
Updated•9 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
Blocks: TV_Seamless_FennecSide
Updated•9 years ago
|
blocking-b2g: 2.5+ → ---
Assignee | ||
Comment 21•9 years ago
|
||
latest patch for TV 2.5 integration test.
Attachment #8674163 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
Merge presentation device into CastingApps.js architecture.
Attachment #8679420 -
Attachment is obsolete: true
Attachment #8687160 -
Flags: feedback?(mark.finkle)
Updated•9 years ago
|
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8687160 -
Attachment is obsolete: true
Attachment #8687160 -
Flags: feedback?(mark.finkle)
Attachment #8707377 -
Flags: review?(mark.finkle)
Comment 24•9 years ago
|
||
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•9 years ago
|
blocking-b2g: --- → 2.5+
Assignee | ||
Comment 25•9 years ago
|
||
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•9 years ago
|
||
Hi Mark,
Could you help to review the patch when you are available?
Thanks!
Flags: needinfo?(mark.finkle)
Assignee | ||
Comment 27•9 years ago
|
||
@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)
Assignee | ||
Comment 28•9 years ago
|
||
Comment 29•9 years ago
|
||
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+
Assignee | ||
Comment 30•9 years ago
|
||
update according to review comment, carry r+.
Attachment #8714271 -
Attachment is obsolete: true
Attachment #8715618 -
Flags: review+
Assignee | ||
Comment 31•9 years ago
|
||
File bug 1245729 as the follow-up of comment #29.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 32•9 years ago
|
||
Keywords: checkin-needed
Comment 33•9 years ago
|
||
bugherder |
Blocks: 1347755
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•