Steps: 1. http://people.mozilla.org/~ayang/gum/gUM%20Test%20Page.html 2. press FakeVideo 3. press the link "Main webrtc demo page" to open another gUM test page. 4. press video The camera can't be initialize due the the fake video will create the camera manager. And the new WindowID won't be added into the WindowTable in DOMCameraManager::Register.

After investigation, windowID among MediaEngine, CameraManager and MediaManager are not the sync. I print window id at: http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1288 http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#499 http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraManager.cpp#144 Condition 1, in normal video gUM: 1st gUM(video) 2nd gUM(video) 3rd gUM(video) -------------------------------------------------------------------------- MediaManager 6 7 8 MediaEngineWebRTC\ VideoSource 6 6 6 DOMCameraManager 6 6 6 It works because MediaEngine and DOMCameraManager use the same windowID. DOMCameraControl is created by MediaEngine and has the same windowID. There windowId test in nsDOMCameraManager::IsWindowStillActive [1] will be success, although both modules use the wrong windowID actually. Condition 2, in mochitest test: 1st gUM(fake) 2nd gUM(video) 3rd gUM(video) ---------------------------------------------------------------------------------- MediaManager 6 7 8 MediaEngine (MediaEngineDefault) (MediaEngineWeb\ null RTCVideoSource) 7 7 DOMCameraManager 6 6 6 The 2nd/3rd gUM don't work because MediaEngine and DOMCameraManager use different windowID and it will fail at [1]. My patch will disable the DOMCameraManger at 1st gUM which will sync the windowID on MediaEngine and DOMCameraManager at 2nd/3rd, that'd make situation same as condition 1. To fix the problem in condition 1; IMHO, it probbably needs to avoid using static DOMCameraManager and remove mBackend from static MediaManager. [1] http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControl.cpp#524

Hi Randell, need your valuable input here, thanks

I think the difference may be between InnerWindow IDs and OuterWindow IDs; for example see MediaManager::MediaCaptureWindowStateInternal() which finds all GUM streams for a given OuterWindow. See https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows Note that the MediaManager's list is not all InnerWindows - see MediaManager::RemoveFromWindowList() for an example of how they're handled. MediaManager keeps a hash table keyed by WindowID of active gUM streams, and it uses the windowID it was given by Navigator.mozGetUserMedia() - there's no GetInnerWindow/etc in the code for the MediaManager.cpp spot where you dump the WindowID. (Iframes complicate things a bit, but mostly for the UI, not for MediaManager - see above about MediaCaptureWindowStateInternal() for something that deals with iframes.) You need to be very careful that you compare windows correctly and deal with their tree-like structure - simple ID comparisons are often wrong. I think the problem (per above) comes from looking up a window id in DOMCameraManager that might not be an inner id; DOMCameraManager appears to store inner windows only. Needinfo to roc & smaug to verify my assessment or give suggestions (since we need to get this resolved ASAP). FYI, from IRC with alfredo, I believe the issue is that if we init DOMCameraManager for fake streams, it's not cleaning up correctly (apparently due to the WindowID mismatch). It may be that significant rework of how WindowIDs are handling between MediaEngine and DOMCameraManager is needed, or perhaps just some judicious use of Inner/OuterWindow calls. (Note DOMCameraManager is B2G-only, and I don't know all the details of how it uses WindowIDs - just what I saw from inspection, and WindowIDs can be confusing.) Concerning the patch here: I had told alfredo I didn't want to make "fake" streams avoid normal initialization (as much as possible) because I want us to test as much of the normal flow as possible, and also because adding a new state of "we're running but the camera manager isn't inited" makes things more complex and may hide issues (like this WindowID issue). His message I'm responding to was the result of his diving into why DOMCameraManager wasn't cleaning up for fake streams.

(In reply to Randell Jesup [:jesup] from comment #4) > I think the difference may be between InnerWindow IDs and OuterWindow IDs; > for example see MediaManager::MediaCaptureWindowStateInternal() which finds > all GUM streams for a given OuterWindow. Hmm, sounds odd that it plays with outerwindow IDs. inner window ID is the one which maps better to a web page. outer window is mapped to docshell lifetime.

(In reply to Randell Jesup [:jesup] from comment #4) > Concerning the patch here: I had told alfredo I didn't want to make "fake" > streams avoid normal initialization (as much as possible) because I want us > to test as much of the normal flow as possible, and also because adding a > new state of "we're running but the camera manager isn't inited" makes > things more complex and may hide issues (like this WindowID issue). His > message I'm responding to was the result of his diving into why > DOMCameraManager wasn't cleaning up for fake streams. I think there is another way to make 'fake' with camera initialization working. I will upload another patch later. Could you please feedback it? Thanks.

(In reply to Randell Jesup [:jesup] from comment #4) > I think the difference may be between InnerWindow IDs and OuterWindow IDs; > for example see MediaManager::MediaCaptureWindowStateInternal() which finds > all GUM streams for a given OuterWindow. > > See https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows > > Note that the MediaManager's list is not all InnerWindows - see > MediaManager::RemoveFromWindowList() for an example of how they're handled. > MediaManager keeps a hash table keyed by WindowID of active gUM streams, and > it uses the windowID it was given by Navigator.mozGetUserMedia() - there's > no GetInnerWindow/etc in the code for the MediaManager.cpp spot where you > dump the WindowID. > > (Iframes complicate things a bit, but mostly for the UI, not for > MediaManager - see above about MediaCaptureWindowStateInternal() for > something that deals with iframes.) You need to be very careful that you > compare windows correctly and deal with their tree-like structure - simple > ID comparisons are often wrong. > > I think the problem (per above) comes from looking up a window id in > DOMCameraManager that might not be an inner id; DOMCameraManager appears to > store inner windows only. My explanation here is not right (I checked more of the code): 1) all windows in the MediaManager list are inner windows obtained from Navigator and verified by mWindow->GetOuterWindow()->getCurrentInnerWindow() == mWindow. This is then used to get the WindowId and that's used for the hashtable entry/chain. 2) Note that per the diagram in the mdn page, an inner window can have iframes with their own outer and inner windows. 3) Note that MediaCaptureWindowStateInternal() does in fact find all streams for an outerwindow, but by iterating the window tree and finding all the innerwindows, and then asking the hashtable for any gUM() captures for those innerwindows. The *real* issue here is that MediaEngineWebRTCVideoSource's (which are tied to the DOMCameraControl objects) are used to provide the WindowID, but MediaEngineWebRTCVideoSources are long-lived; they do not die when the page does, so having them hold a WindowID is a bad idea. That's why the WindowIDs are the value of the first time the VideoSource was created and never change. (They don't use WindowID for non-B2G code.) In MediaEngineWebRTC::EnumerateVideoDevices(), in the B2G-specific ifdef, it does "if (mVideoSources.Get(...))" and if the source already exists, appends it tot he list of devices, otherwise it creates it. The VideoSource, when Allocate() is called on it to activate it, dispatches an AllocImpl() to mainthread, and that creates the DOMCameraControl using the saved mWindowId - but note this is the window used for the *first* call to Enumerate the video devices, not the *current* getUserMedia() call! The question is: for DOMCameraControls created from a gUM call, which are tied to an allocated VideoSource: Do the need to know the window at all? Normally it's used for DOMCameraManager::OnNavigation to clear the DOMCameraControls out - but the gUM mediaStreams will get hit with MediaManager::OnNavigation as well, and will Deallocate() any VideoSources, which will release the DOMCameraControls. So, I'd posit the best solution is to let MediaManager handle freeing up the DOMCameraControls associated with it, and not add them to the main DOMCameraWindow table. The only complication is that as DOM objects, DOMCameraControls must be freed on MainThread, but the AllocImpl/DeallocImpl calls already run there, so it's not a problem. The other use of the table is to free all the DOMCameraControls on shutdown. Similarly, all MediaManager captures get shut down, so the DOMCameraControls should get shut down via MediaManager anyways.

Builds clean (and fixes some B2G-specific warnings)

Comment on attachment 8343604 [details] [diff] [review] Don't track windowIDs in MediaEngine for B2G Review of attachment 8343604 [details] [diff] [review]: ----------------------------------------------------------------- Michael - Not sure who's the best to review this (normally I would), but you've touched this code, and schient wrote some of it. I'll take a review from either mwu or schien. schien/alfredo/etc - can you give the a try (and submit a Try?) I'll put one up too, but I'm not sure I have the right set of patches.

Comment on attachment 8343604 [details] [diff] [review] Don't track windowIDs in MediaEngine for B2G Review of attachment 8343604 [details] [diff] [review]: ----------------------------------------------------------------- lgtm for the webrtc part, but I'm not confident to review DOMCameraManager. Add @mikeh for reviewing this part. ::: content/media/webrtc/MediaEngineWebRTC.h @@ +51,5 @@ > #include "GonkCameraControl.h" > #include "ImageContainer.h" > #include "nsGlobalWindow.h" > #include "prprf.h" > +#include "nsProxyRelease.h" nit: Except for the comment in line 402, I didn't see any code that really need this #include.

Probably need to Register() the CameraControl always to avoid problems with IsWindowStillActive() (thanks alfredo)

Comment on attachment 8343470 [details] [diff] [review] gUM_Set_WindowID Review of attachment 8343470 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the DOMCameraManager changes; though bug 909542 (when it lands) will remove the MediaEngine's interactions with DOM layers in the b2g camera code. Instead, everything will be through a narrowly-defined ICameraControl that doesn't involve windows, JSContexts, etc.

https://hg.mozilla.org/integration/b2g-inbound/rev/1d72742584ad r+ from mikeh in IRC for the correct patch

backedout as part of https://tbpl.mozilla.org/?rev=df82be9d89a5 because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209

Backed out from Aurora along with bug 853356.

Hi Randell, Because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209, this patch is also backed out. I wonder if this one will be just re-landed after the other test cases are passed in bug 853356. Thank you!

(In reply to Ivan Tsay (:ITsay) from comment #19) > Hi Randell, > > Because of https://bugzilla.mozilla.org/show_bug.cgi?id=853356#c209, this > patch is also backed out. I wonder if this one will be just re-landed after > the other test cases are passed in bug 853356. Thank you! Yes, Ivan. That is correct. Once the new test cases for bug 853356 have been added (see bug 948826) and are passing, we will re-land the patch on this bug together with the patches on bug 853356.

Randell -- Please unbitrot and reland

Major refactors have made this bug basically moot.