Bug 752351 will have a fallback MediaEngine which can form the basis for a cross-platform DOM binding to getUserMedia. Some code from Bug 691234 can be reused. This patch will also need to be aware of the changes being made to Navigator.cpp in Bug 738528 for still image support on Android, and ensure that there is a single injection point for getUserMedia's multiple forms across all 3 platforms (B2G, Android, Desktop).

Uploading a WIP patch, it works; but needs some more cleanup.

Comment on attachment 625328 [details] [diff] [review] WIP - v0.1 Review of attachment 625328 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/Makefile.in @@ -46,5 @@ > LIBXUL_LIBRARY = 1 > -XPIDL_MODULE = content_media > - > -XPIDLSRCS = \ > - nsIDOMMediaStream.idl \ I don't see this file being moved do dom/media/interfaces in this patch ::: dom/base/Navigator.cpp @@ +940,5 @@ > + mMediaManager = new media::MediaManager(); > + } > + > + return mMediaManager->GetUserMedia(aParams, onSuccess, onError); > +} you're leaking mMediaManager ::: dom/media/interfaces/nsIDOMNavigatorUserMedia.idl @@ +16,5 @@ > + > +[scriptable, function, uuid(2614bbcf-85cc-43e5-8740-964f52bdc7ca)] > +interface nsIDOMGetUserMediaErrorCallback : nsISupports > +{ > + void onError(in DOMString error); maybe a DOMError instead of a DOMString? @@ +35,5 @@ > + in nsIMediaStreamOptions params, > + in nsIDOMGetUserMediaSuccessCallback onsuccess, > + in nsIDOMGetUserMediaErrorCallback onerror > + ); > +}; \ No newline at end of file The options interface will become much more complex so we should probably use a jsval and a dictionnary - thought that can be done in a follow up ::: dom/media/src/MediaManager.cpp @@ +87,5 @@ > + > + mozilla::MediaEngine* aBackend = GetBackend(); > + > + if (video) { > + // Pick the last device in the list for now we need to be able to specify which device we pick. I'm ok with adding another property in the options interfaces since we don't have the capabilities/constraints support yet. @@ +98,5 @@ > + )); > + return NS_OK; > + } > + > + mozilla::MediaEngineVideoSource* vSource = vSources[count-1]; nit: spaces around operator "-" @@ +127,5 @@ > + )); > + return NS_OK; > + } > + > + mozilla::MediaEngineAudioSource* aSource = aSources[count-1]; nit: ditto also, aXXX is usually used for parameter names. I know it means "audio" there, but it's still disturbing.

About the e10s support, I think we can do that at the b2g implementation level, so don't bother with it here.

Anant, What is the permissions story here? with the code as it is there, nothing prevents a page from getting a stream without prompting the user in any way.

(In reply to Fabrice Desré [:fabrice] from comment #4) > What is the permissions story here? with the code as it is there, nothing > prevents a page from getting a stream without prompting the user in any way. That's correct, we haven't implemented permissions yet because that requires platform specific UI which we haven't built yet (and may not be ready in time for tree closure). The plan was to check this in with the "media.enabled" flag set to false by default (which we are checking in Navigator.cpp), just like Chrome is doing in their Canary builds now. If that's not optimal for B2G then: a) How long will it take to build a permission prompt in B2G, i.e. is it doable by the 5th? b) It it worth defining and abstract interface (or reusing something like nsIPermissionPrompt) and use it in the DOM binding?

(In reply to Anant Narayanan [:anant] from comment #5) > (In reply to Fabrice Desré [:fabrice] from comment #4) > > What is the permissions story here? with the code as it is there, nothing > > prevents a page from getting a stream without prompting the user in any way. > > That's correct, we haven't implemented permissions yet because that requires > platform specific UI which we haven't built yet (and may not be ready in > time for tree closure). The plan was to check this in with the > "media.enabled" flag set to false by default (which we are checking in > Navigator.cpp), just like Chrome is doing in their Canary builds now. We don't need the UI to be ready, just the hooks. > If that's not optimal for B2G then: > a) How long will it take to build a permission prompt in B2G, i.e. is it > doable by the 5th? > b) It it worth defining and abstract interface (or reusing something like > nsIPermissionPrompt) and use it in the DOM binding? We have basic support for permissions using nsIContentPermissionPrompt in b2g (it mostly needs polish on the gaia side), but I'm not sure that solves the issue well. For the camera app, we just need to grant "unlimited" access (i.e no user prompt), but for a random page we would need to show some selection dialog and nsIContentPermissionPrompt is not usable for that. My proposal is to : - keep the "media.enabled" pref (true for b2g, flase everywhere else by default) - define a permission like "unlimited-media-access" to bypass any permission prompt. We'll grant it only the default apps. - file a follow-up to design and use in the DOM binding a new permission prompt that allows source selection. Also, we absolutely need to be able to specify which source we want to select in the getUM() call.

(In reply to Fabrice Desré [:fabrice] from comment #6) > We have basic support for permissions using nsIContentPermissionPrompt in > b2g (it mostly needs polish on the gaia side), but I'm not sure that solves > the issue well. For the camera app, we just need to grant "unlimited" access > (i.e no user prompt), but for a random page we would need to show some > selection dialog and nsIContentPermissionPrompt is not usable for that. > > My proposal is to : > - keep the "media.enabled" pref (true for b2g, flase everywhere else by > default) > - define a permission like "unlimited-media-access" to bypass any permission > prompt. We'll grant it only the default apps. > - file a follow-up to design and use in the DOM binding a new permission > prompt that allows source selection. Ok, sounds good to me. > Also, we absolutely need to be able to specify which source we want to > select in the getUM() call. I agree that we should just put it in the options object for now since we aren't going to implement constraints in the near future. How do you propose we specify the source? Just simple strings like "front"/"back" or are you looking for more specifics? Note that we don't have a call for content to enumerate available devices. I'll do the above in addition to another refactor of the patch to actually make the calls not block the main thread :) The MediaEngine interfaces are synchronous so this is best done in the DOM binding.

Comment on attachment 625328 [details] [diff] [review] WIP - v0.1 (In reply to Fabrice Desré [:fabrice] from comment #2) > ::: content/media/Makefile.in > @@ -46,5 @@ > > LIBXUL_LIBRARY = 1 > > -XPIDL_MODULE = content_media > > - > > -XPIDLSRCS = \ > > - nsIDOMMediaStream.idl \ > > I don't see this file being moved do dom/media/interfaces in this patch I don't know if that's just hg being weird, but here it is: >diff --git a/content/media/nsIDOMMediaStream.idl b/dom/media/interfaces/nsIDOMMediaStream.idl >rename from content/media/nsIDOMMediaStream.idl >rename to dom/media/interfaces/nsIDOMMediaStream.idl >diff --git a/dom/media/interfaces/nsIDOMNavigatorUserMedia.idl b/dom/media/interfaces/nsIDOMNavigatorUserMedia.idl >new file mode 100644 >--- /dev/null >+++ b/dom/media/interfaces/nsIDOMNavigatorUserMedia.idl

First take at the DOM binding: navigator.getUserMedia({video:bool,audio:bool,picture:bool}, onSuccess, onError); In combination with the patches in bug 750943 and bug 752351, this will return grey pixels and silence on all platforms. The "real" backends are being tracked as follows: Desktop: bug 691234 B2G: bug 740997 Android (picture only): bug 738528

Fixes a crash in the previous patch caused by not creating the DOMMediaStream on the main thread.

Comment on attachment 628151 [details] [diff] [review] DOM bindings for getUserMedia - v1 Review of attachment 628151 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/Makefile.in @@ +10,5 @@ > +relativesrcdir = dom/media > + > +include $(DEPTH)/config/autoconf.mk > + > +PARALLEL_DIRS = interfaces src Don't do that, just put everything in dom/media ::: dom/media/interfaces/nsIDOMNavigatorUserMedia.idl @@ +22,5 @@ > + > +[scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)] > +interface nsIMediaStreamOptions : nsISupports > +{ > + readonly attribute boolean audio; Should be a dictionary afaict ::: dom/media/src/Makefile.in @@ +25,5 @@ > + $(NULL) > + > +LOCAL_INCLUDES = \ > + -I$(topsrcdir)/dom/base \ > + $(NULL) Why? ::: dom/media/src/MediaManager.h @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "Navigator.h" > +#include "MediaStreamGraph.h" > +#include "MediaEngineDefault.h" Don't think you need to include all that @@ +9,5 @@ > +namespace mozilla { > +namespace dom { > +namespace media { > + > +class MediaManager : public nsISupports { Not sure why this inherits from nsISupports @@ +16,5 @@ > + ~MediaManager() { delete mBackend; }; > + > + NS_DECL_ISUPPORTS > + > + mozilla::MediaEngine* GetBackend(); No need for the mozilla:: @@ +22,5 @@ > + nsIDOMGetUserMediaSuccessCallback* onSuccess, > + nsIDOMGetUserMediaErrorCallback* onError); > + > +private: > + mozilla::MediaEngine* mBackend; Not here either

Comment on attachment 628434 [details] [diff] [review] DOM bindings for getUserMedia - v1.1 Thanks Ms2ger! There are a few more changes in the pipeline, so clearing flag for now.

Doug did a bunch of work to clean up the DOM bindings to make it compatible with an implementation of {picture:true} for Android (bug 738528). This patch solves several issues that the previous versions had, and now uses the notify callbacks provided by MediaStreams (bug 759908) to operate MediaEngine backends more efficiently.

We're now using an nsIDOMFile instead of a nsILocalFile for the Snapshot method because we can store the former in memory and include type information. Also fixed the path in the layout Makefile.

Comment on attachment 629046 [details] [diff] [review] DOM bindings for getUserMedia - v2 Review of attachment 629046 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +918,5 @@ > + if (!mMediaManager) { > + mMediaManager = new media::MediaManager(); > + } > + > + nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow)); ...win = do_... ::: dom/media/Makefile.in @@ +4,5 @@ > + > +DEPTH = ../.. > +topsrcdir = @top_srcdir@ > +srcdir = @srcdir@ > +VPATH = @srcdir@ (I'm not much of a fan of aligning the =s, but at least you're not using tabs.) @@ +24,5 @@ > + nsIDOMNavigatorUserMedia.idl \ > + $(NULL) > + > +EXPORTS = \ > + MediaManager.h \ This should be exported as mozilla/dom/MediaManager.h, like here: <http://mxr.mozilla.org/mozilla-central/source/content/base/public/Makefile.in#48> ::: dom/media/MediaManager.cpp @@ +15,5 @@ > +namespace mozilla { > +namespace dom { > +namespace media { > + > +class DOMWindowHolder : public nsISupports I don't think this needs to inherit from nsISupports @@ +19,5 @@ > +class DOMWindowHolder : public nsISupports > +{ > +public: > + DOMWindowHolder(nsIDOMWindow* aWindow) > + : mWindow(aWindow) {} I guess you want to assert that you're on the main thread here too, as you're AddRefing @@ +21,5 @@ > +public: > + DOMWindowHolder(nsIDOMWindow* aWindow) > + : mWindow(aWindow) {} > + > + ~DOMWindowHolder() {} And here for the Release @@ +28,5 @@ > + > + nsIDOMWindow* > + GetDOMWindow() > + { > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); MOZ_ASSERT @@ +38,5 @@ > + > +NS_IMPL_THREADSAFE_ISUPPORTS0(DOMWindowHolder) > + > + > +class ErrorCallbackRunnable : public nsRunnable { { on the next line @@ +57,5 @@ > + nsCOMPtr<nsIDOMGetUserMediaErrorCallback> mError; > + const nsString mErrorMsg; > +}; > + > +class SuccessCallbackRunnable : public nsRunnable { id @@ +72,5 @@ > + , mSuccess(aSuccess) > + , mStream(aStream) {} > + > + JSContext* > + contextForWindow() Always start with a capital letter, though this can probably all go away; see my comment below @@ +84,5 @@ > + if (!scriptContext) { > + return nsnull; > + } > + JSContext *cx = scriptContext->GetNativeContext(); > + return cx; return scriptContext->GetNativeContext(); @@ +150,5 @@ > + > +/** > + * This runnable creates a nsDOMMediaStream from a given MediaEngineSource > + * and returns it via a success callback. Both must be done on the main thread. > + */ Yay, a comment! Please add comments for the runnables above :) @@ +151,5 @@ > +/** > + * This runnable creates a nsDOMMediaStream from a given MediaEngineSource > + * and returns it via a success callback. Both must be done on the main thread. > + */ > +class GetUserMediaCallbackRunnable : public nsRunnable { { on a new line @@ +167,5 @@ > + > + /** > + * This inner class is an implementation of MediaStreamListener > + */ > + class GetUserMediaCallbackMediaStreamListener : public MediaStreamListener { id @@ +173,5 @@ > + GetUserMediaCallbackMediaStreamListener(MediaEngineSource *aSource, > + nsDOMMediaStream *aStream, TrackID aListenId) > + : mSource(aSource) > + , mStream(aStream) > + , mId(aListenId) {} MOZ_ASSERT that your arguments aren't null (also elsewhere). @@ +181,5 @@ > + { > + if (aConsuming == CONSUMED) { > + nsDOMMediaStream* stream = mStream.get(); > + nsRefPtr<SourceMediaStream> sStream = stream->GetStream()->AsSourceStream(); > + mSource->Start(sStream.get(), mId); "sStream" sounds like a static variable, and you shouldn't need the .get()s. How about nsRefPtr<SourceMediaStream> stream = mStream->GetStream()->AsSourceStream(); mSource->Start(stream, mId); @@ +207,5 @@ > + NS_IMETHOD > + Run() > + { > + // Normally we would now get the name & UUID for the device and ask the > + // user permission. We will do that when we have some UI. Sounds like you want to list a bug number @@ +208,5 @@ > + Run() > + { > + // Normally we would now get the name & UUID for the device and ask the > + // user permission. We will do that when we have some UI. > + nsCOMPtr<nsDOMMediaStream> mComStream = mSource->Allocate(); Don't start local variables with "m", that's for members. @@ +209,5 @@ > + { > + // Normally we would now get the name & UUID for the device and ask the > + // user permission. We will do that when we have some UI. > + nsCOMPtr<nsDOMMediaStream> mComStream = mSource->Allocate(); > + if (mComStream == nsnull) { !mComStream @@ +218,5 @@ > + } > + > + // Add our listener. We'll call Start() on the source when get a bcallback > + // that the MediaStream has started consuming. > + nsDOMMediaStream* stream = mComStream.get(); This seems rather pointless @@ +239,5 @@ > + nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> mSuccess; > + nsCOMPtr<nsIDOMGetUserMediaErrorCallback> mError; > +}; > + > +class GetUserMediaSnapshotCallbackRunable : public nsRunnable { . @@ +256,5 @@ > + NS_IMETHOD > + Run() > + { > + nsCOMPtr<nsDOMMediaStream> comStream = mSource->Allocate(); > + if (comStream == nsnull) { !comStream @@ +264,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsCOMPtr<nsILocalFile> file; > + mSource->Snapshot(mDuration, getter_AddRefs(file)); Dunno where that's implemented, but can this return the file? Also, you'll need to update to nsIFile once bug 749930 lands. @@ +275,5 @@ > + > +private: > + nsRefPtr<DOMWindowHolder> mWindow; > + nsCOMPtr<MediaEngineSource> mSource; > + PRUint32 mDuration; Doesn't make much of a difference here, but generally sort members from largest to smallest size. @@ +313,5 @@ > + NS_IMETHOD > + Run() > + { > + > + if (mPicture) { Stray newline @@ +337,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + NS_IMETHOD > + SendPicture() You don't want to make those NS_IMETHOD, that gives you a virtual call you don't need. Return nsresult, or preferably void (as we never fail). @@ +340,5 @@ > + NS_IMETHOD > + SendPicture() > + { > + nsTArray<nsRefPtr<MediaEngineVideoSource> > videoSources; > + MediaEngine* mBackend = mManager->GetBackend(); "backend", or drop the local altogether @@ +343,5 @@ > + nsTArray<nsRefPtr<MediaEngineVideoSource> > videoSources; > + MediaEngine* mBackend = mManager->GetBackend(); > + mBackend->EnumerateVideoDevices(&videoSources); > + > + PRInt32 count = videoSources.Length(); PRUint32 @@ +355,5 @@ > + NS_DispatchToMainThread(new GetUserMediaSnapshotCallbackRunable( > + mWindow, videoSource, 0 /* duration */, mSuccess, mError > + )); > + return NS_OK; > + } All the comments apply to SendVideo and SendAudio as well @@ +404,5 @@ > + > +private: > + bool mAudio; > + bool mVideo; > + bool mPicture; Again, at the end @@ +459,5 @@ > + > + return NS_OK; > +} > + > +} } // namespace foo ::: dom/media/MediaManager.h @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "MediaEngine.h" > +#include "nsIDOMWindow.h" > +#include "nsIDOMNavigatorUserMedia.h" Don't think you need all these includes @@ +9,5 @@ > +namespace mozilla { > +namespace dom { > +namespace media { > + > +class MediaManager : public nsISupports { Inherit? @@ +26,5 @@ > +private: > + MediaEngine* mBackend; > +}; > + > +} } // namespace foo ::: dom/media/nsIDOMNavigatorUserMedia.idl @@ +11,5 @@ > + /* > + * value must be a nsIDOMBlob if picture is true and a > + * nsIDOMMediaStream if either audio or video are true. > + */ > + void onSuccess(in jsval value); I think you would make life a lot easier for yourself if you made this take nsISupports and had XPConnect handle the conversion to jsval. @@ +30,5 @@ > + readonly attribute DOMString camera; > +}; > + > +[scriptable, uuid(381e0071-0be5-4f6b-ae21-8e3407a37faa)] > +interface nsIDOMNavigatorUserMedia : nsISupports I'm not sure if you need a separate interface here, if it can't be preffed off.

Comment on attachment 629105 [details] [diff] [review] DOM bindings for getUserMedia - v2.1 This is really based on the previous version, but I think all of this applies to this version as well. I looked mostly at high level stuff here for now, still want to look through in more detail, but these issues need to be dealt with one way or another before this can land... Navigator::MozGetUserMedia(): + if (!win || !win->GetOuterWindow() || !win->GetDocShell()) { + return NS_ERROR_FAILURE; + } Why are the !win->GetOuterWindow() and !win->GetDocShell() checks relevant here? + return mMediaManager->GetUserMedia(win->GetOuterWindow(), aParams, onSuccess, onError); In fact, you really want the inner window here, the outer can navigate and change origins under the covers etc at any time. All that's done here needs to be bound to an inner window so that it remains associated with the site it came from throughout its lifetime. Speaking of lifetime... What kills (or pauses?) the media stream stuff when a page is navigated? Seems we need a hook somewhere here to stop/pause all things media when navigation happens. Currently the Navigator class doesn't know when navigation actually happens, but that could be added if needed. But we'd also need to deal with the case where a page gets shoved into the fastback cache (see nsGlobalWindow::SaveInnerWindowState()). - In dom/base/nsDOMClassInfo.cpp DOM_CLASSINFO_MAP_BEGIN(Navigator, nsIDOMNavigator) DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigator) DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorDeviceStorage) DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorGeolocation) + DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorUserMedia) This needs the #ifdef we discussed, either hard coded for android and/or b2g, or put behind a configure flag. +class DOMWindowHolder : public nsISupports +{ +public: + DOMWindowHolder(nsIDOMWindow* aWindow) + : mWindow(aWindow) {} + + ~DOMWindowHolder() {} + + NS_DECL_ISUPPORTS + + nsIDOMWindow* + GetDOMWindow() + { + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); + return mWindow; + } +private: + nsCOMPtr<nsIDOMWindow> mWindow; +}; + +NS_IMPL_THREADSAFE_ISUPPORTS0(DOMWindowHolder) This class, and its usage, is not enough to guarantee that the DOM window doesn't get released on the a thread other than the main thread (and that would be very bad). You'll need to at the very least make the destructor in this class proxy the release call on mWindow to the main thread (if this class happens to be deleted by a release call on another thread, of which you have no guaranteed control). Or alternatively (and I think I'd prefer this, at least long term) you could make GetUserMediaRunnable etc not hold a strong reference to the window at all (it can still have a raw pointer to it so it can be passed along to other runnables, who also need to not hold a strong reference to the window). And instead of the strong window reference, you'd ensure that the runnables themselves are held alive (by holding references to the runnables and explicitly managing their lifetimes) and then the runnables can also be invalidated if navigation happens while we're waiting for the runnables to run etc - In class SuccessCallbackRunnable: + jsval + nsIFileToJsval(nsILocalFile* aFile) + { + if (aFile == nsnull) { + return JSVAL_NULL; + } This check is redundant, the caller of this function checks this for you. + nsContentUtils::WrapNative(cx, + JS_GetGlobalObject(cx), JS_GetGlobalObject() returns an outer window, you need the inner window here. As a rule you never ever want to wrap something in an outer window's scope, cause again, its origin can change etc (there are a very small number of exceptions to this rule, but this is not one of them). Use nsIScriptGlobalObject::GetGlobalJSObject() here once the window you have here is the inner window and you'll have the right scope. + jsval + streamToJsval(nsIDOMMediaStream* aStream) + { + if (aStream == nsnull) { + return JSVAL_NULL; + } Same as above. + jsval wrapped; + nsContentUtils::WrapNative(cx, + JS_GetGlobalObject(cx), Also same as above. - In class GetUserMediaCallbackRunnable: + NS_IMETHOD + Run() + { + // Normally we would now get the name & UUID for the device and ask the + // user permission. We will do that when we have some UI. When will we have such UI? And why do we not need it right now? r- based on that. Still need to look through more of the runnable logic etc here, but that'll need to wait until I'm more awake than I am right now :)

MediaManager is now a singleton, and we don't keep a reference in Navigator. Doug moved around a few things to make sure the WindowHelper is safe, and proper use of nsCOMPtr.swap() for the window references. The patch works on Desktop, but crashes on Android, Doug is tracking it down. I haven't addressed all of your comments, there is still a bit of work to do (configure flag, style nits, etc.); but in my mind the lifetime issue is the meat of the matter which we should resolve first. I'll work on the other comments now.

This should cover most of the concerns. (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #16) > + if (!win || !win->GetOuterWindow() || !win->GetDocShell()) { > + return NS_ERROR_FAILURE; > + } > > Why are the !win->GetOuterWindow() and !win->GetDocShell() checks relevant > here? We're passing the outer window to the Manager now, and the DocShell check is simply to prevent the API from being available in windows that are not visible, or part of a DOM tree. > Speaking of lifetime... What kills (or pauses?) the media stream stuff when > a page is navigated? Seems we need a hook somewhere here to stop/pause all > things media when navigation happens. Currently the Navigator class doesn't > know when navigation actually happens, but that could be added if needed. > But we'd also need to deal with the case where a page gets shoved into the > fastback cache (see nsGlobalWindow::SaveInnerWindowState()). So we've moved to a singleton model now. The only objects tied to a window are the runnables and the media stream (or blobs). For MediaStream cleanup, we rely on the NotifyConsumptionChanged callback to tell us when there are no more consumers for a particular stream and clean up then. Currently, that callback is not being invoked on page navigation or close. I'm not sure if that's intended behaviour (I've asked in bug 759908). If it is, then we'll have to monitor page navigation ourselves and make sure to clean up properly. > This needs the #ifdef we discussed, either hard coded for android and/or > b2g, or put behind a configure flag. I added a configure flag (--enable-media-navigator). The pref is now media.navigator.enabled. We have both the flag and the pref because the while the flag disables the availability of the API itself, setting the pref to false merely makes the API silently fail. I think both are useful to have (and eventually the flag will go away but we'll want to retain the pref). > + NS_IMETHOD > + Run() > + { > + // Normally we would now get the name & UUID for the device and ask the > + // user permission. We will do that when we have some UI. > > When will we have such UI? And why do we not need it right now? We don't need a UI because the only active backend is the android picture backend. When {picture:true} is set in getUserMedia, a UI is not necessary since the user selecting a picture is implicit permission (like <input>). I made a note of the bug for the UI for desktop camera support. That should be landed when we add the desktop backend for MediaEngine.

Comment on attachment 629451 [details] [diff] [review] DOM bindings for getUserMedia - v4 Couple of minor comments beyond what we've chatted about on irc... - In class SuccessCallbackRunnable: + NS_IMETHOD + Run() + { + jsval result = JSVAL_NULL; This is unused now, so yo ucan remove it. - In class GetUserMediaRunnable: + NS_IMETHOD + Run() + { [...] + return NS_ERROR_FAILURE; The caller of the runnable isn't going to care, so might as well return NS_OK here too. +class MediaManager { +public: + static MediaManager* Get() { + if (!mSingleton) { + mSingleton = new MediaManager(); + } + return mSingleton; + } I don't see anything ever deleting this singleton. You should add an XPCOM shutdown observer that deletes the singleton. +nsresult +MediaManager::GetUserMedia(nsIDOMWindow* aWindow, + nsIMediaStreamOptions *aParams, + nsIDOMGetUserMediaSuccessCallback* onSuccess, + nsIDOMGetUserMediaErrorCallback* onError) Replace the tabs with spaces... +[scriptable, uuid(381e0071-0be5-4f6b-ae21-8e3407a37faa)] +interface nsIDOMNavigatorUserMedia : nsISupports +{ + void mozGetUserMedia(in nsIMediaStreamOptions params, + in nsIDOMGetUserMediaSuccessCallback onsuccess, + in nsIDOMGetUserMediaErrorCallback onerror); Replace the tabs with spaces... With that and the additional tweaks we've been chatting about this is very close to ready to land!

More refinement, but still not perfect. I don't think I'm storing the runnable references into the hash table and array correctly. The problem is that the runnables sometimes get freed on their own (when they complete) and sometimes they don't (when the page reloads/navigates/closes). If I don't store them as nsRefPtrs, the ->Invalidate() call in OnNavigation() causes a segfault in the cases where the runnables get freed. However, when I do store them as RefPtrs, I cannot Clear() the array either. On xpcom-shutdown, the array is cleared since I delete mSingleton, Clear() gets called and causes a crash.

Comment on attachment 629462 [details] [diff] [review] DOM bindings for getUserMedia - v5 Review of attachment 629462 [details] [diff] [review]: ----------------------------------------------------------------- The lifetime management of the runnables is confusing; most other users of runnables include an nsRefPtr to the item to be interacted with. Given we're doing things with them I don't see elsewhere (invalidate, hash, etc). It would help to document in the code when and where the runnables are owned (and released). I'm pretty sure NS_DispatchToMainThread() releases the object passed to it, and assumes (in most cases) that this will destroy the object. Because we stored them in the array/hash, the ownership is different than normal, we need to make sure they all get released at the right time (when they're done running, and that this bubbles up correctly since you have runnables firing other runnables. Also, if a 'master' runnable is invaldiated, make sure any children are (this may already be the case). ::: dom/media/MediaManager.cpp @@ +209,5 @@ > + > + // Add the listener to CallbackRunnables so it can be invalidated. > + mRunnables->AppendElement(listener); > + > + SuccessCallbackRunnable* scr = new SuccessCallbackRunnable( While perhaps not strictly necessary, looking through other users of nsIRunnable subclasses I see that *if* they assign it to a var, it's always an nsCOMPtr<nsIRunnable>, not a raw pointer. Those that don't assign it to a local var though do pass the result of new directly to NS_DispatchToMainThread(). (This applies to the other runnables as well.) @@ +222,5 @@ > + nsCOMPtr<MediaEngineSource> mSource; > + TrackID mId; > + nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> mSuccess; > + nsCOMPtr<nsIDOMGetUserMediaErrorCallback> mError; > + CallbackRunnables* mRunnables; This is mildly confusing, as there's also an mRunnables in the MediaManager class, and this mRunnables points to the master mRunnables. I'd rename all these pointers-to-mRunnables, or rename MediaManager's master mRunnables to mCallbackRunnables or some such. ::: dom/media/MediaManager.h @@ +12,5 @@ > +#include "nsIDOMNavigatorUserMedia.h" > + > +namespace mozilla { > +namespace dom { > +namespace media { Please check the namespace guidelines for new classes (updated by ehsan in the last few months). Nested namespaces are now deprecated (for new code) with only a few exceptions.

Sorry for chiming in late, but why do we want MediaManager to be a singleton? If this is to manage the exclusive access to hardware resources, that's pointless in b2g since with e10s we'll end up with a singleton per process. And we'll manage the hw locking by remoting what we need from the Engine.

I think the way you would need to do the ownership management of the runnables with the array of them as part of this system is to leave the array hold raw pointers (i.e. not nsRefPtr<>'s) and make the runnables themselves remove themselves out of the array whenever they're destroyed. And I also think that the main GetUserMediaRunnable runnable that gets posted to a different thread needs to be tracked here too, so that we can prevent its Run() method from doing anything if we race with it. All that means we'd need to add locking to the hash/arrays etc, which we could do, but after reading through this I think we can do this even simpler. What if we simply maintain a hash of the currently "valid" window IDs in the media manager, and in the Run() methods that happen on the main thread we simply check whether our window ID is still in the hash of "valid" window IDs? IOW, whenever getUserMedia() is called, we make sure the window ID is in the hash (which might already be there if this is not the first call from a given window ID), and then on navigation we make sure we remove the window ID from the hash. I think that would take care of all cases I can think of right now. Oh, except for one case, which is if someone somehow manages to call getUserMedia() on an already navigated navigator object, but we can fix that easily by preventing the call if the navigator objects window is not its outer windows current inner window.

(In reply to Fabrice Desré [:fabrice] from comment #22) > Sorry for chiming in late, but why do we want MediaManager to be a > singleton? If this is to manage the exclusive access to hardware resources, > that's pointless in b2g since with e10s we'll end up with a singleton per > process. And we'll manage the hw locking by remoting what we need from the > Engine. Making it a singleton makes lifecycle management a lot easier on non-e10s platforms and lets us address advanced use-cases like mediating a single stream between two different tabs. Looks like this won't fully work on B2G because we'll end up with a singleton per process, but we can always special case h/w locking as you mention in that case.

We're very close! This all works great... except on xpcom-shutdown, where: delete mSingleton; causes the following crashh: (gdb) bt #0 0xb773c424 in __kernel_vsyscall () #1 0xb74e4c16 in nanosleep () from /lib/i386-linux-gnu/libc.so.6 #2 0xb74e4a0f in sleep () from /lib/i386-linux-gnu/libc.so.6 #3 0xb3964a93 in ah_crap_handler (signum=11) at /home/anant/Code/mozilla-inbound/toolkit/xre/nsSigHandlers.cpp:87 #4 0xb396e61a in nsProfileLock::FatalSignalHandler (signo=11, info=0xbfcaef4c, context=0xbfcaefcc) at /home/anant/Code/mozilla-inbound/objdir-desktop/toolkit/profile/nsProfileLock.cpp:190 #5 <signal handler called> #6 0xb5126612 in ReleaseObjects (aElement=0xab7f2ac0) at /home/anant/Code/mozilla-inbound/objdir-desktop/xpcom/build/nsCOMArray.cpp:134 #7 0xb512c3eb in nsVoidArray::EnumerateForwards (this=0xbfcaf480, aFunc=0xb51265f8 <ReleaseObjects(void*, void*)>, aData=0x0) at /home/anant/Code/mozilla-inbound/objdir-desktop/xpcom/build/nsVoidArray.cpp:690 #8 0xb51266a6 in nsCOMArray_base::Clear (this=0xbfcaf518) at /home/anant/Code/mozilla-inbound/objdir-desktop/xpcom/build/nsCOMArray.cpp:144 #9 0xb51262d1 in nsCOMArray_base::~nsCOMArray_base (this=0xbfcaf518, __in_chrg=<optimized out>) at /home/anant/Code/mozilla-inbound/objdir-desktop/xpcom/build/nsCOMArray.cpp:25 #10 0xb4b36edf in nsCOMArray<nsIObserver>::~nsCOMArray (this=0xbfcaf518, __in_chrg=<optimized out>) at ../../../../dist/include/nsCOMArray.h:139 #11 0xb514dd59 in nsObserverList::NotifyObservers (this=0xa7e8e2a0, aSubject=0xb7253504, aTopic=0xb615cf88 "xpcom-shutdown", someData=0x0) I don't fully understand why as we call Clear() on the array OnNavigation(), and then we Clear() the Hashtable again right before deleting the singleton. The behavior isn't dependent on whether or not we choose to use nsRefPtrs to store the MediaStreamListener objects. Commenting "delete mSingleton;" make the problem go away (but will cause a shutdown leak?). Calling "delete &mActiveWindows" has the same effect: (gdb) bt #0 0xb778e424 in __kernel_vsyscall () #1 0xb7536c16 in nanosleep () from /lib/i386-linux-gnu/libc.so.6 #2 0xb7536a0f in sleep () from /lib/i386-linux-gnu/libc.so.6 #3 0xb39b0a93 in ah_crap_handler (signum=6) at /home/anant/Code/mozilla-inbound/toolkit/xre/nsSigHandlers.cpp:87 #4 0xb39ba61a in nsProfileLock::FatalSignalHandler (signo=6, info=0xbfc9831c, context=0xbfc9839c) at /home/anant/Code/mozilla-inbound/objdir-desktop/toolkit/profile/nsProfileLock.cpp:190 #5 <signal handler called> #6 0xb778e424 in __kernel_vsyscall () #7 0xb74b11ef in raise () from /lib/i386-linux-gnu/libc.so.6 #8 0xb74b4835 in abort () from /lib/i386-linux-gnu/libc.so.6 #9 0x080675c4 in arena_run_reg_dalloc (run=0xa386b000, bin=0xb73801fc, ptr=0xa386bb00, size=48) at /home/anant/Code/mozilla-inbound/memory/jemalloc/jemalloc.c:3288 #10 0x0806be85 in arena_dalloc_small (arena=0xb7380040, chunk=0xa3800000, ptr=0xa386bb00, mapelm=0xa3800514) at /home/anant/Code/mozilla-inbound/memory/jemalloc/jemalloc.c:4486 #11 0x0806c725 in arena_dalloc (ptr=0xa386bb00, offset=441088) at /home/anant/Code/mozilla-inbound/memory/jemalloc/jemalloc.c:4614 #12 0x0806fb8d in free (ptr=0xa386bb00) at /home/anant/Code/mozilla-inbound/memory/jemalloc/jemalloc.c:6541 #13 0xb777dbf8 in moz_free (ptr=0xa386bb00) at /home/anant/Code/mozilla-inbound/memory/mozalloc/mozalloc.cpp:48 #14 0xb44d5dca in operator delete (ptr=0xa386bb00) at ../../dist/include/mozilla/mozalloc.h:224 #15 mozilla::dom::media::MediaManager::Observe (this=0xa386baf0, aSubject=0xb7253504, aTopic=0xb61a8ea8 "xpcom-shutdown", aData=0x0) So the problem is in freeing the nsClassHashtable itself, not the singleton.

$ fortune -s "Never make any mistaeks." (Anonymous, in a mail discussion about to a kernel bug report.) Ekr gratefully pointed out that we were double free'ing the singleton. The observer service frees it for us. No leaks!

Comment on attachment 629522 [details] [diff] [review] DOM bindings for getUserMedia - v7 - In Navigator::MozGetUserMedia(): + nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow); Add this check here: if (!win || !win->GetOuterWindow() || win->GetOuterWindow->GetCurrentInnerWindow() != win) { return NS_ERROR_NOT_AVAILABLE; } + return manager->GetUserMedia(win->WindowID(), aParams, onSuccess, onError); To make sure we don't start media stuff on a window that's already been navigated and/or closed. - In class ErrorCallbackRunnable: + Run() + { + // Only run if the window is still active. + WindowTable* activeWindows = MediaManager::Get()->GetActiveWindows(); + if (!activeWindows->Get(mWindowID)) { + return NS_OK; + } + mError->OnError(mErrorMsg); + return NS_OK; Maybe write that as: if (activeWindows->Get(mWindowID)) { mError->OnError(mErrorMsg) } return NS_OK; to save on an early return in this trivial function? Same thing in SuccessCallbackRunnable, though there's a bit more code there... I can go either way here. - In class GetUserMediaRunnable: + GetUserMediaRunnable(bool aAudio, bool aVideo, bool aPicture, + MediaManager* aManager, Given that the MediaManager is a singleton, do you need to pass it in here, can't this code simply grab the singleton when it needs the media manager? - In MediaManager::GetUserMedia(): + // Store the WindowID in a hash table and mark as active. The entry is removed + // when this window is closed or navigated away from. + StreamListeners* listeners = new StreamListeners; + mActiveWindows.Put(aWindowID, listeners); If I call getUserMedia() twice in the same page we'll leak these listeners :( Maybe we simply need to check whether we already have listeners for the given window and simply use that? But this also means we have race conditions further down the road since we have multiple threads adding/removing listeners at the same time. See below... + nsCOMPtr<nsIRunnable> gUMRunnable = new GetUserMediaRunnable( + audio, video, picture, this, onSuccess, onError, aWindowID, listeners + ); + + nsCOMPtr<nsIThread> gUMThread; + rv = NS_NewThread(getter_AddRefs(gUMThread), gUMRunnable); + NS_ENSURE_SUCCESS(rv, rv); This is a bit worrisome. This creates a whole new thread per getUserMedia() call, and threads are pretty heavy weight (memory wise). Maybe we can simply create one single thread here, and have the singleton MediaManager hold on to that thread and re-use it? And maybe even destroy it whenever there's nothing left to do, and re-create it on next call etc? - In MediaManager::OnNavigation(): + StreamListeners* listeners = mActiveWindows.Get(aWindowID); + if (!listeners) { + return; + } + + PRUint32 length = listeners->Length(); + for (PRUint32 i = 0; i < length; i++) { + nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener = + listeners->SafeElementAt(i); No need for SafeElementAt() here, you already know 'i' is a valid index. + listener->Invalidate(); + } + listeners->Clear(); So this runs on the main thread and it mutates the listeners array, which gets appended to on a different thread in GetUserMediaCallbackRunnable. I don't see how we can make this safe w/o a lock. The lock could be a member of the MediaManager, so a global lock, effectively. But that should be fine here, we're only protecting the array operations here. Use mozilla::MutexAutoLock etc. + mActiveWindows.Remove(aWindowID); You'll need to delete listeners too, the hash does not do that for you here. - In MediaManager::Observe(): + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); + obs->RemoveObserver(this, "xpcom-shutdown"); + + // Close off any remaining active windows. + mActiveWindows.Clear(); + return NS_OK; This also needs to null out the mSingleton static, or you'll leak (if you don't something else is screwy with the reference counting here). +typedef nsTArray<GetUserMediaCallbackMediaStreamListener*> StreamListeners; This really needs to be nsTArray<nsRefPtr<GetUserMediaCallbackMediaStreamListener> >, if not, your listener memory management won't be sound. - In class MediaManager: + static nsRefPtr<MediaManager> mSingleton; Please rename mSingleton to sSingleton to signify in the name the fact that this is a static member. +interface nsIDOMNavigatorUserMedia : nsISupports +{ + void mozGetUserMedia(in nsIMediaStreamOptions params, + in nsIDOMGetUserMediaSuccessCallback onsuccess, + in nsIDOMGetUserMediaErrorCallback onerror); This still uses tabs for indentation. r-, but other than the threading and memory management pieces here, this is looking good!

In this patch we create only one listener array per window. We also reuse a single thread for all incoming getUserMedia calls. At a later point we might want to use a pool to balance between memory usage and speed. Array operations are locked by a mutex appropriately. One thing is left unresolved: deleting the listeners nsTArray. Removing it from the hashtable is unsufficient, but a manual "delete listeners;" in MediaManager::OnNavigation leads to the following stack trace: http://pastebin.mozilla.org/1654505

doublec and ekr were investigating on IRC and it looks like the hashtable owns whatever objects are added to it. Hence, deleting them is not required as the objects will be freed when they are removed from the hashtable.

Comment on attachment 629548 [details] [diff] [review] DOM bindings for getUserMedia - v8 - In class GetUserMediaCallbackRunnable: + NS_IMETHOD + Run() [...] + MutexAutoLock lock(*(MediaManager::Get()->GetLock())); + mListeners->AppendElement(listener); + + // Add the listener to CallbackRunnables so it can be invalidated. + NS_DispatchToMainThread(new SuccessCallbackRunnable( + mSuccess, comStream.get(), mWindowID + )); You should scope that lock even narrower, cause the only thing you're protecting is the append. i.e.: + { + MutexAutoLock lock(*(MediaManager::Get()->GetLock())); + mListeners->AppendElement(listener); + } - In class GetUserMediaRunnable: + SendPicture() + { + nsTArray<nsRefPtr<MediaEngineVideoSource> > videoSources; + mManager->GetBackend()->EnumerateVideoDevices(&videoSources); + + PRUint32 count = videoSources.Length(); + if (!count) { + NS_DispatchToMainThread(new ErrorCallbackRunnable( + mError, NS_LITERAL_STRING("NO_DEVICES_FOUND"), mWindowID + )); + } Add an early return inside the if (!count) block, or you'll access memory before the array buffer below. + SendVideo() Same thing here... + SendAudio() ... and here. r=jst with those few things fixed!

Comment on attachment 629548 [details] [diff] [review] DOM bindings for getUserMedia - v8 Review of attachment 629548 [details] [diff] [review]: ----------------------------------------------------------------- One more nit: ::: dom/media/MediaManager.cpp @@ +15,5 @@ > +#include "nsDOMFile.h" > + > +namespace mozilla { > +namespace dom { > +namespace media { From https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide C++ namespaces Mozilla project C++ declarations should be in the "mozilla" namespace. Modules should avoid adding nested namespaces under "mozilla" unless they are meant to contain names which have a high probability of colliding with other names in the code base (e.g., Point, Path, etc.).

Verified through testing this on desktop in a separate bug. Will file followup bugs per test plan, but this verifies that this has indeed landed.