Closed Bug 752352 Opened 12 years ago Closed 12 years ago

Define and implement chrome extensions to getUserMedia and MediaStreams for privileged callers

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp -

People

(Reporter: anant, Assigned: anant)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [getUserMedia], [blocking-gum+], [qa-])

Attachments

(1 file, 6 obsolete files)

In Bug 750943 we discussed reusing getUserMedia to allow privileged callers from JS to perform media related actions (eg: build a preview UI, provide streams via getUserMedia to content).

This bug will track the extensions needed to getUserMedia and MediaStreams in order to provide privileged callers from JS the same functionality that C++ callers have via the MediaEngine.
This includes functionality such as CameraControl (https://wiki.mozilla.org/WebAPI/CameraControl) on the MediaStream object. On Desktop this will be chrome privileged (but may form the basis of the {picture:true} implementation, for instance), while on B2G it will be exposed directly to certain types of trusted content (camera app).
We're scoping this down to B2G since B2G is the initial target and driver here.  I believe the plan for camera control is to return an object that implements both nsIDOMMediaStream and nsICameraControl.  Camera control has not yet been discussed in the W3C so there is no spec to implement at this point.
Summary: Define and implement chrome extensions to getUserMedia and MediaStreams for privileged callers → Define and implement chrome extensions to getUserMedia and MediaStreams for privileged callers in B2G
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
Maire, how does this relate to bug 749886?
(In reply to Dietrich Ayala (:dietrich) from comment #3)
> Maire, how does this relate to bug 749886?

IMO there is really very little connection.  Here's how I would summarize things: This bug (752352) is for Camera Control features, which are needed for the default camera app that ships with the phone.  Bug 749886 is needed for the camera api used by 3rd party apps in the Marketplace that want to take a snapshot ("still image support").  

For background: I summarized in an email to clee about a week ago that both of these bugs are needed for the B2G camera work (which includes both the default B2G camera app and the camera api to support 3rd party apps wants to take snapshots), but self-contained and not blocking any non-B2G work.  There was one bug (bug 740997) that was blocking bug 752353, but we resolved that dependency (and bug 752353 has already landed).  So in that email I was categorizing B2G bugs as those that were blocking other (cross-platform getUserMedia) work (needed to support camera features), and those that weren't.  This bug and bug 749886 fell into the "non-blocking" category.  

Hope this is all clear and helps.  If you have more questions, let me know.
Assigning to Mike H per Fabrice.
Assignee: nobody → mhabicher
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Does this still block basecamp? I noticed a lot of WebRTC bugs were getting basecamp-, so why is this one a +?
blocking-basecamp: + → ?
This is basecamp+ because the camera app in B2G depends on this.

The bug was already marked blocking-basecamp+, why did you remove the flag?
(In reply to Anant Narayanan [:anant] from comment #7)
> This is basecamp+ because the camera app in B2G depends on this.
> 
> The bug was already marked blocking-basecamp+, why did you remove the flag?

Because bug 752353 (the DOM bindings for getUserMedia) was a - for basecamp.
(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Anant Narayanan [:anant] from comment #7)
> > The bug was already marked blocking-basecamp+, why did you remove the flag?
> 
> Because bug 752353 (the DOM bindings for getUserMedia) was a - for basecamp.

That's irrelevant because the bug has been fixed and landed. I would really appreciate it if you didn't change flags once they've already been marked without any prior discussion.
(In reply to Anant Narayanan [:anant] from comment #9)
> (In reply to Jason Smith [:jsmith] from comment #8)
> > (In reply to Anant Narayanan [:anant] from comment #7)
> > > The bug was already marked blocking-basecamp+, why did you remove the flag?
> > 
> > Because bug 752353 (the DOM bindings for getUserMedia) was a - for basecamp.
> 
> That's irrelevant because the bug has been fixed and landed. I would really
> appreciate it if you didn't change flags once they've already been marked
> without any prior discussion.

No. It is relevant because we analyze bugs for basecamp blockers that include fixed bugs, as QA prioritizes those bugs for testing.
(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Anant Narayanan [:anant] from comment #9)
> > That's irrelevant because the bug has been fixed and landed. I would really
> > appreciate it if you didn't change flags once they've already been marked
> > without any prior discussion.
> 
> No. It is relevant because we analyze bugs for basecamp blockers that
> include fixed bugs, as QA prioritizes those bugs for testing.

That bug has also been marked verified. What further testing does QA need to do?

Nevertheless, I also don't understand the argument that because bug 752353 was not a basecamp blocker, this one shouldn't be either. These bugs have nothing to do with each other, and we have already discussed whether or not this bug should be a blocker previously. Revisiting decisions after they've been made unless absolutely required is a waste of time.
(In reply to Anant Narayanan [:anant] from comment #11)
> (In reply to Jason Smith [:jsmith] from comment #10)
> > (In reply to Anant Narayanan [:anant] from comment #9)
> > > That's irrelevant because the bug has been fixed and landed. I would really
> > > appreciate it if you didn't change flags once they've already been marked
> > > without any prior discussion.
> > 
> > No. It is relevant because we analyze bugs for basecamp blockers that
> > include fixed bugs, as QA prioritizes those bugs for testing.
> 
> That bug has also been marked verified. What further testing does QA need to
> do?

A lot actually. The testing that has been done on that bug was making sure that pieces were landed and at a proof of concept, was working. I'm still burning through getting a test plan together, running various test cases, and logging bugs (which you'll see a bunch get logged every so often in the WebRTC components).

> 
> Nevertheless, I also don't understand the argument that because bug 752353
> was not a basecamp blocker, this one shouldn't be either. These bugs have
> nothing to do with each other, and we have already discussed whether or not
> this bug should be a blocker previously. Revisiting decisions after they've
> been made unless absolutely required is a waste of time.

Because generally in basecamp triage, the precedent being followed was that "if a webrtc bug is seen, it's an automatic minus." They are related to each other, as implementing underlying API to be able to make use of it here is a prerequisite to implementing this piece (there's other bugs like this, which were also a minus).

It's actually important to revisit if this blocks or not because we're past a feature complete deadline (July 20th), and this landing pieces toward a feature, not a bug. It also affects decisions that may be made in the webrtc meetings (like the need to unprefix rather quickly, as I'd caution taking a risk on the web compat side of the world with relying on prefixed APIs in the gecko mobile space). There's also general confusion on whether this blocks or not, which is why I put it back in the nomination queue (one said yes, 2 said no, one said maybe, about 4 others said I don't know from the b2g team that I talked to).

If there's one thing that would be helpful to provide here is to provide context into the use cases of what the camera app is doing to make use of getUserMedia. Talking with Dietrich, this only blocks if the camera app needs this API on the critical path to basic functionality in the camera app.
The camera control API for b2g just landed on m-c (bug 740997) - I think it covers what this bug was for.

Now we have all the missing pieced to fix bug 749886, which may or not be a basecamp blocker.
Mike H's response on this on an email thread (for context):

"getUserMedia() support is _not_ required for the B2G camera to function.  We have implemented a low-level camera control API (which landed last night: https://hg.mozilla.org/integration/mozilla-inbound/rev/d59f932deea9 and https://hg.mozilla.org/integration/mozilla-inbound/rev/b13039139d7b ) that exposes detailed features of the camera, beyond gUM().  The intent is that gUM() can evolve as per the WebRTC requirements, and camera control can change as we need it to for the B2G camera."

This sounds like this should be a basecamp-, unless I'm misreading something.
Whiteboard: [blocked-on-input]
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=749886#c23, we are no longer blocked on input. This should be a basecamp-, unless I'm misinterpreting something.
Whiteboard: [blocked-on-input]
This is no longer a priority for B2G, but is needed to implement the camera API on desktop. Rescoping the bug again.
Summary: Define and implement chrome extensions to getUserMedia and MediaStreams for privileged callers in B2G → Define and implement chrome extensions to getUserMedia and MediaStreams for privileged callers
Blocks: 748835
Based on comment 16, basecamp-'ing.
blocking-basecamp: ? → -
Whiteboard: [getUserMedia], [blocking-gum+]
Is Mike Habicher actually working on this?
QA Contact: jsmith
Assignee: mhabicher → anant
This patch adds a navigator.mozGetUserMediaDevices function that will be passed an array of nsIDOMMediaDevice objects via the success callback. Additionally, navigator.mozGetUserMedia now takes a "device" property in the options to specify what device to use to create a stream. Both of these are meant for access from chrome only, content must not be able to do either of these!

For Dao and others, an example usage of this functionality can be found here: https://gist.github.com/3744555 I suspect the chrome UI will be built in roughly the same way.

The big problem with this patch is the memory leak in MediaManager.cpp:427. I'd appreciate some input on how to pass an array from C++ to JS, I was hoping to avoid creating a wrapper object just for that.
Attachment #662220 - Flags: review?(mhabicher)
Attachment #662220 - Flags: review?(bugs)
Why do you hook this on the navigator object if it's chrome only? In your patch web content can still see the new function and call it but it's always useless.
(In reply to Fabrice Desré [:fabrice] from comment #20)
> Why do you hook this on the navigator object if it's chrome only? In your
> patch web content can still see the new function and call it but it's always
> useless.

I originally wanted a XPCOM interface, but roc and others recommended reusing the content JS API (See bug 750943). The problem is reusing the same getUserMedia function will be tricky (roc had suggested representing each track as one device), but I couldn't make it work with that approach.

Is there a way we can hide this function from regular content?
Comment on attachment 662220 [details] [diff] [review]
Add getUserMediaDevices functionality - v1


> Navigator::MozGetUserMedia(nsIMediaStreamOptions* aParams,
>                            nsIDOMGetUserMediaSuccessCallback* onSuccess,
>                            nsIDOMGetUserMediaErrorCallback* onError)
Um, onSuccess and onError should be
aOnSuccess and aOnError.
Same also elsewhere in the patch where onSuccess and onError are params.
(I wouldn't use onFoo for anything else but onFoo event handlers, but maybe the aOnSuccess and aOnError is ok here, 
if the spec has onSuccess and onError too as parameters)


>+  /* Check if the caller is chrome privileged */
>+  nsresult rv;
>+  bool privileged;
>+  nsCOMPtr<nsIScriptSecurityManager> securityManager =
>+    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsIPrincipal* principal = win->GetExtantDoc()->NodePrincipal();
>+  rv = securityManager->IsSystemPrincipal(principal, &privileged);
>+  NS_ENSURE_SUCCESS(rv, rv);

nsContentUtils::IsChromeDoc(win->GetExtantDoc());
is quite a bit simpler ;)



>+{
>+  nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>+  if (!win || !win->GetOuterWindow() ||
>+      win->GetOuterWindow()->GetCurrentInnerWindow() != win) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  /* Check if the caller is chrome privileged */
>+  nsresult rv;
>+  bool privileged;
>+  nsCOMPtr<nsIScriptSecurityManager> securityManager =
>+    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsIPrincipal* principal = win->GetExtantDoc()->NodePrincipal();
>+  rv = securityManager->IsSystemPrincipal(principal, &privileged);
>+  NS_ENSURE_SUCCESS(rv, rv);

nsContentUtils::IsChromeDoc(win->GetExtantDoc());



> /**
>+ * Invoke the GetUserMediaDevices success callback. Wrapped in a runnable
>+ * so that it may be called on the main thread.
>+ */
>+class DeviceSuccessCallbackRunnable: public nsRunnable
>+{
>+public:
>+  DeviceSuccessCallbackRunnable(nsIDOMGetUserMediaDevicesSuccessCallback* aSuccess,
>+    nsIDOMMediaDevice** aDevices, int aCount)
Really, int? not int32_t ?

>+  NS_IMETHOD
>+  Run()
>+  {
>+    mSuccess->OnSuccess(mDevices, mCount);
>+    return NS_OK;
>+  }
>+
>+private:
>+  nsCOMPtr<nsIDOMGetUserMediaDevicesSuccessCallback> mSuccess;
>+  nsIDOMMediaDevice** mDevices;
This would most probably lead to sg:critical error in some cases.
Please keep objects alive .
nsTArray<nsCOMPtr<nsIDOMMediaDevice> > mDevices


>+  int mCount;
int32_t



>+
>+    total = videoCount + audioCount;
>+    // FIXME: This leaks. Is the only other way to create an array wrapper?
>+    nsIDOMMediaDevice** devices =
>+      (nsIDOMMediaDevice**) PR_Calloc(total, sizeof(nsIDOMMediaDevice*));
Uh, don't use arrays of raw pointers to interfaces unless there is some *really *
good reason.
You really want nsTArray<nsCOMPtr<nsIDOMMediaDevice> > for devices.


>+class MediaDevice : public nsIDOMMediaDevice {
{ should be in the next line

>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMMEDIADEVICE
>+
>+  MediaDevice(MediaEngineVideoSource* aSource) {
ditto

>+    mSource = aSource;
>+    mType.Assign(NS_LITERAL_STRING("video"));
>+    mSource->GetName(mName);
>+  };

>+  MediaDevice(MediaEngineAudioSource* aSource) {
ditto



>+[scriptable, builtinclass, uuid(6de854f9-acf8-4383-b464-4803631ef309)]
>+interface nsIDOMMediaDevice : nsISupports
>+{
>+  readonly attribute DOMString type;
>+  readonly attribute DOMString name;
>+};
>+
>+[scriptable, function, uuid(24544878-d35e-4962-8c5f-fb84e97bdfee)]
>+interface nsIDOMGetUserMediaDevicesSuccessCallback : nsISupports
>+{
>+  void onSuccess([array, size_is(count)] in nsIDOMMediaDevice devices,
>+    in unsigned long count);
>+};
Any chance to not use nsIDOM prefix for the interfaces. Just nsI
What way the interfaces wouldn't be automatically in the global scope
of each web page.
Attachment #662220 - Flags: review?(bugs) → review-
Bunch of changes thanks to smaug. Hopefully nothing leaks now, and mozGetUserMediaDevices is only available to Chrome-JS.
Attachment #662220 - Attachment is obsolete: true
Attachment #662220 - Flags: review?(mhabicher)
Attachment #662736 - Flags: review?(bugs)
Comment on attachment 662736 [details] [diff] [review]
Add getUserMediaDevices functionality - v2


>+  MediaManager *manager = MediaManager::Get();
Nit,
MediaManager* manager =  MediaManager::Get();

>+
>+  MediaManager *manager = MediaManager::Get();
ditto
>+
>+  nsIMediaDevice* device;
>+  rv = aParams->GetDevice(&device);
>+  NS_ENSURE_SUCCESS(rv, rv);
This looks wrong.
GetDevice should addref the out param and device is then leaked.
use nsCOMPtr<nsIMediaDevice> device;
and getter_AddRefs




>+  if (device) {
>+    gUMRunnable = new GetUserMediaRunnable(
>+      audio, video, picture, aOnSuccess, aOnError, listeners, windowID,
>+      (MediaDevice*) device
C++ casting, please.

>+MediaManager::GetUserMediaDevices(nsPIDOMWindow* aWindow,
>+  nsIGetUserMediaDevicesSuccessCallback* aOnSuccess,
>+  nsIDOMGetUserMediaErrorCallback* aOnError)
>+{
>+  nsCOMPtr<nsIRunnable> gUMDRunnable = new GetUserMediaDevicesRunnable(
>+    aOnSuccess, aOnError
>+  );
>+
>+  nsCOMPtr<nsIThread> deviceThread;
>+  nsresult rv = NS_NewThread(getter_AddRefs(deviceThread));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  deviceThread->Dispatch(gUMDRunnable, NS_DISPATCH_NORMAL);
>+  return NS_OK;
>+}
Hmm, what runs in which thread?
DOM stuff _must_not_ be addrefed/released in non-main-thread.

Have you tested this all with a debug build?

This code could use some assertions about the right thread.
Attachment #662736 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #24)
> >+  nsCOMPtr<nsIThread> deviceThread;
> >+  nsresult rv = NS_NewThread(getter_AddRefs(deviceThread));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  deviceThread->Dispatch(gUMDRunnable, NS_DISPATCH_NORMAL);
> >+  return NS_OK;
> >+}
> Hmm, what runs in which thread?
> DOM stuff _must_not_ be addrefed/released in non-main-thread.

Right, the DOM objects are actually created and passed to JS in the DevicesSuccessRunnable which runs on the main thread.

> Have you tested this all with a debug build?

Yes, it works fine, except for some leak messages specific to OS X, may or may not be related to this code. I just started a Linux build to see if something happens there. XPCOM_MEM_LEAK_LOG doesn't report any leaks.
 
> This code could use some assertions about the right thread.

Added!
Attachment #662736 - Attachment is obsolete: true
Attachment #663081 - Flags: review?(bugs)
GetUserMediaRunnable and GetUserMediaDevicesRunnable run off the main thread, but were holding references to callbacks provided by JS. These references should only be AddRef'ed or Released on the main thread, so we switched to passing around  already_AddRefed objects.
Attachment #663081 - Attachment is obsolete: true
Attachment #663081 - Flags: review?(bugs)
Attachment #663100 - Flags: review?(bugs)
Attachment #663100 - Attachment is obsolete: true
Attachment #663100 - Flags: review?(bugs)
Attachment #663101 - Flags: review?(bugs)
3rd time's the charm!
Attachment #663101 - Attachment is obsolete: true
Attachment #663101 - Flags: review?(bugs)
Attachment #663110 - Flags: review?(bugs)
Comment on attachment 663110 [details] [diff] [review]
Add getUserMediaDevices functionality - v5

Someone more familiar with MediaManager should look at this too.
Attachment #663110 - Flags: review?(bugs) → review+
Comment on attachment 663110 [details] [diff] [review]
Add getUserMediaDevices functionality - v5

Shouldn't we always pass error and success callbacks back to main thread so
that they are released properly.
Attachment #663110 - Flags: review+ → review-
Nice catch! Only the success or error callbacks were getting released, not both. Adding jesup for a secondary review on the MediaManager parts.
Attachment #663110 - Attachment is obsolete: true
Attachment #663119 - Flags: review?(rjesup)
Attachment #663119 - Flags: review?(bugs)
Attachment #663119 - Flags: review?(bugs) → review+
Comment on attachment 663119 [details] [diff] [review]
Add getUserMediaDevices functionality - v6

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

::: dom/media/MediaManager.cpp
@@ +57,5 @@
>    }
>  
>  private:
> +  already_AddRefed<nsIDOMGetUserMediaSuccessCallback> mSuccess;
> +  already_AddRefed<nsIDOMGetUserMediaErrorCallback> mError;

Didn't know you could do that...  Seems handy

@@ +341,5 @@
>  
> +    if (mPicture) {
> +      ProcessGetUserMediaSnapshot(mDevice->GetSource(), 0);
> +      return NS_OK;
> +    }

thanks

::: dom/media/nsIDOMNavigatorUserMedia.idl
@@ +37,2 @@
>  
>  [scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)]

Do we need to change this UUID?
Attachment #663119 - Flags: review?(rjesup) → review+
I don't believe this is testable from web content from an end-user perspective, so marking as such. If I'm not right here, please let me know.
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa-]
https://hg.mozilla.org/mozilla-central/rev/2fed77bd195f
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Jason Smith [:jsmith] from comment #34)
> I don't believe this is testable from web content from an end-user
> perspective, so marking as such. If I'm not right here, please let me know.

Uh oh. Looks like this accidentally "did" get exposed to web content, as checking the navigator property in the 9/22 build reveals that the mozGetUserMediaDevices function exists on the navigator object. Follow-up fix is needed here - this shouldn't be exposed to web content.
We have really no easy way in  xpidl to not expose those to web content.
But yes, we need tests that those methods return null or throw exception for web content.
(In reply to Olli Pettay [:smaug] from comment #37)
> We have really no easy way in  xpidl to not expose those to web content.
> But yes, we need tests that those methods return null or throw exception for
> web content.

Okay. I'll change the context of the follow bug (bug 793436) to reflect that.
Depends on: 793436
Oh, hmm, nsINavigatorUserMedia shouldn't be in the classinfo.
Chrome code should be forced to QI to nsINavigatorUserMedia.
(Hoping that approach works)
Can we please get documentation on this on developer.mozilla.org?

mozGetUserMediaDevices is completely undocumented.
Keywords: dev-doc-needed
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.