Closed Bug 1070216 Opened 10 years ago Closed 9 years ago

Add support for MediaStream constructors (from MediaStreamTracks)

Categories

(Core :: Audio/Video, defect, P2)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jesup, Assigned: pehrsons)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(11 files)

40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
padenot
: review+
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
padenot
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
40 bytes, text/x-review-board-request
jib
: review+
Details
+++ This bug was initially created as a clone of Bug #910249 +++

One of the things we've never implemented is the ability to take tracks from two MediaStreams and combine them using a MediaStream constructor to make a third MediaStream.  For example, and video track from a getUserMedia stream, and an audio track from a second (audio processed through WebAudio, for example).

Note: not explicitly Webrtc, so in Video/Audio
Unfeasible if MediaStream.addTrack() is not implemented: #1103188
Depends on: 1103188
This should be simple with a default constructor and addTrack() (blocking this bug).
This will be very useful for bug 1156472, see comment 1. In fact, it's kind of blocking it.
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Comment on attachment 8665902 [details]
MozReview Request: Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc

Paul, if you check the diff of this patch you'll see that all three constructors are doing `MediaStreamGraph::GetInstance(SYSTEM_THREAD_DRIVER, AudioChannel::Normal);`

Is there something to think about here? Could anything go wrong if, let's say, I add a track belonging to another graph than I got from GetInstance()?
Attachment #8665902 - Flags: feedback?(padenot)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #6)
> Comment on attachment 8665902 [details]
> MozReview Request: Bug 1070216 - Implement MediaStream Constructors.
> r?smaug,roc
> 
> Paul, if you check the diff of this patch you'll see that all three
> constructors are doing `MediaStreamGraph::GetInstance(SYSTEM_THREAD_DRIVER,
> AudioChannel::Normal);`
> 
> Is there something to think about here? Could anything go wrong if, let's
> say, I add a track belonging to another graph than I got from GetInstance()?

You should get the graph from the stream, and maybe add a debug assert and early return in release just to make sure.

For now, we should throw if you try to put two streams from different graphs together. In the future, we will need to support connections between graphs, but I don't think it should block this.
Attachment #8665902 - Flags: feedback?(padenot)
Comment on attachment 8665902 [details]
MozReview Request: Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc

Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc
This allows us to initiate a DOMMediaStream into three states:
* One that has input, owned and playback streams, for data producers
  like gUM or RTCPeerConnection.
* One with owned and playback streams, for cloned DOM streams.
  If a cloned DOM stream has an empty input stream connected to its
  owned stream, both are regarded as not having current data.
* One with only a playback stream, for when it has been created with the
  default constructor from JS. Its track set can only be changed by
  addTrack() and removeTrack() which when called only affect mPlaybackStream.
Attachment #8665902 - Attachment description: MozReview Request: Bug 1070216 - Implement MediaStream Constructors. r?smaug,roc → MozReview Request: Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc
Attachment #8665902 - Flags: review?(roc)
Bug 1070216 - constify DOMMediaStream::Get[Audio/Video]Tracks(). r?roc
Attachment #8668793 - Flags: review?(roc)
Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot
Attachment #8668794 - Flags: review?(padenot)
Bug 1070216 - Implement MediaStream constructors. r?smaug,jib
Attachment #8668795 - Flags: review?(jib)
Attachment #8668795 - Flags: review?(bugs)
Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot
Attachment #8668796 - Flags: review?(padenot)
Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot
Attachment #8668797 - Flags: review?(padenot)
Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot
Attachment #8668798 - Flags: review?(padenot)
Bug 1070216 - Test MediaStream Constructors. r?jib
Attachment #8668799 - Flags: review?(jib)
Attachment #8668795 - Flags: review?(jib) → review+
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

https://reviewboard.mozilla.org/r/21037/#review18927

::: dom/media/DOMMediaStream.cpp:469
(Diff revision 1)
> +/* static */ already_AddRefed<DOMMediaStream>
> +DOMMediaStream::Constructor(const dom::GlobalObject& aGlobal,
> +                            const dom::Sequence<OwningNonNull<MediaStreamTrack>>& aTracks,
> +                            ErrorResult& aRv)

The other two constructors can be implemented with this one.

::: dom/media/DOMMediaStream.cpp:716
(Diff revision 1)
> +DOMMediaStream::CreateEmptyStream(nsIDOMWindow* aWindow)
> +{
> +  nsRefPtr<DOMMediaStream> newStream = new DOMMediaStream();
> +  newStream->mWindow = aWindow;
> +  return newStream.forget();
> +}

Then this inlines.
https://reviewboard.mozilla.org/r/21037/#review18927

> The other two constructors can be implemented with this one.

Not quite, because we want the new playback stream to be created in the same MediaStreamGraph as the tracks we're adding.

With the default constructor we don't know which tracks will be added so we guess at the MSG for the normal AudioChannel (there's one MSG per AudioChannel per my discussion on IRC with padenot yesterday). For the other two constructors we can figure out which MSG to use from the stream/tracks passed in. Streams from different MSGs cannot mix.

I should perhaps let padenot review this one as well..
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

Bug 1070216 - Implement MediaStream constructors. r?smaug,jib
Attachment #8668795 - Flags: review?(padenot)
https://reviewboard.mozilla.org/r/21037/#review18927

> Not quite, because we want the new playback stream to be created in the same MediaStreamGraph as the tracks we're adding.
> 
> With the default constructor we don't know which tracks will be added so we guess at the MSG for the normal AudioChannel (there's one MSG per AudioChannel per my discussion on IRC with padenot yesterday). For the other two constructors we can figure out which MSG to use from the stream/tracks passed in. Streams from different MSGs cannot mix.
> 
> I should perhaps let padenot review this one as well..

But you do that in the last constructor as well if the passed-in tracks sequence is empty.
https://reviewboard.mozilla.org/r/21037/#review18927

> But you do that in the last constructor as well if the passed-in tracks sequence is empty.

Yeah, I could optimize that piece (empty tracks sequence) to use the default constructor, but it doesn't get rid of the need for CreateEmptyStream().
https://reviewboard.mozilla.org/r/21037/#review18927

> Yeah, I could optimize that piece (empty tracks sequence) to use the default constructor, but it doesn't get rid of the need for CreateEmptyStream().

No, the other way around. Implement the default constructor with the last one:

    /* static */ already_AddRefed<DOMMediaStream>
    DOMMediaStream::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
    {
      return Constructor(aGlobal, Sequence<OwningNonNull<MediaStreamTrack>>(), aRv);
    }

The second one can be as well. Also, the dom:: prefixes are unneeded.

> Then this inlines.

Once the constructor implementations are merged, this is used from one place, and can be inlined.
Comment on attachment 8668799 [details]
MozReview Request: Bug 1070216 - Test MediaStream Constructors. r?jib

https://reviewboard.mozilla.org/r/21045/#review18929

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamConstructors.html:49
(Diff revision 1)
> +      checkMediaStreamContains(stream, [audioTrack, videoTrack],
> +                               "List constructed audio and video tracks");

You don't have complete code coverage here with three different constructor implementations. E.g. you'd want to repeat this test with an empty array for instance.

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamConstructors.html:52
(Diff revision 1)
> +      var test = createMediaElement('video', 'testCopyConstructor');
> +      var playback = new MediaStreamPlayback(test, stream);
> +      return playback.playMedia(false).then(() => gUMStream.stop());

Boilerplate x 3. Helper?

::: dom/media/tests/mochitest/test_getUserMedia_mediaStreamConstructors.html:73
(Diff revision 1)
> +        return analyser.waitForAnalysisSuccess(array =>
> +                 array[analyser.binIndexForFrequency(1000)] < 50 &&
> +                 array[analyser.binIndexForFrequency(5000)] < 50 &&
> +                 array[analyser.binIndexForFrequency(10000)] < 50)
> +          .then(() => analyser.disableDebugCanvas());

unusual indent
Attachment #8668799 - Flags: review?(jib) → review+
https://reviewboard.mozilla.org/r/21037/#review18927

> No, the other way around. Implement the default constructor with the last one:
> 
>     /* static */ already_AddRefed<DOMMediaStream>
>     DOMMediaStream::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
>     {
>       return Constructor(aGlobal, Sequence<OwningNonNull<MediaStreamTrack>>(), aRv);
>     }
> 
> The second one can be as well. Also, the dom:: prefixes are unneeded.

Oh. Yeah that works :-)
Attachment #8668794 - Flags: review?(padenot) → review+
Comment on attachment 8668794 [details]
MozReview Request: Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot

https://reviewboard.mozilla.org/r/21035/#review18941

::: dom/media/DOMMediaStream.cpp:484
(Diff revision 1)
> +  if (mPlaybackStream->Graph() !=

Maybe we should notify authors in this case ?
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

https://reviewboard.mozilla.org/r/21037/#review18943
Attachment #8668795 - Flags: review?(padenot)
Attachment #8668796 - Flags: review?(padenot) → review+
Comment on attachment 8668796 [details]
MozReview Request: Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot

https://reviewboard.mozilla.org/r/21039/#review18945
Comment on attachment 8668797 [details]
MozReview Request: Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot

https://reviewboard.mozilla.org/r/21041/#review18947
Attachment #8668797 - Flags: review?(padenot) → review+
Comment on attachment 8668798 [details]
MozReview Request: Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot

https://reviewboard.mozilla.org/r/21043/#review18949
Attachment #8668798 - Flags: review?(padenot) → review+
https://reviewboard.mozilla.org/r/21035/#review18941

> Maybe we should notify authors in this case ?

What do you suggest that makes sense?

"Combining tracks bound to different AudioChannels is not supported" ?
Yeah that'd be enough I guess, otherwise I can imagine a number of developers scratching their head for a long time before finding the reason.
jib, FYI, I'm running into this again (mediamanager:5 logs):

> 821575680[11313ae40]: virtual nsresult mozilla::MediaEngineRemoteVideoSource::Allocate(const dom::MediaTrackConstraints &, const mozilla::MediaEnginePrefs &, const nsString &)
> 821575680[11313ae40]: Video device 4097 allocated shared
> 821575680[11313ae40]: virtual nsresult mozilla::MediaEngineRemoteVideoSource::Stop(mozilla::SourceMediaStream *, mozilla::TrackID)
> 821575680[11313ae40]: Deallocate
> 821575680[11313ae40]: Video device 4097 deallocated
> 821575680[11313ae40]: virtual nsresult mozilla::MediaEngineRemoteVideoSource::Start(mozilla::SourceMediaStream *, TrackID)
> 821575680[11313ae40]: StartCapture failed
> 821575680[11313ae40]: Starting video failed , rv=-2147467259


I can't stop the played stream now since the constructors create DOMMediaStream and not DOMLocalMediaStream instances.

I looked a bit more into the issue - there is actually code to count the number of users of a device, but a device's lifetime is defined by Alloc()/Dealloc() while we do the counting in Start()/Stop() :(

Perhaps it's as simple as moving the counting to Alloc()/Dealloc(). I'll try.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #31)
> Perhaps it's as simple as moving the counting to Alloc()/Dealloc(). I'll try.

I did that, and it works.

However, now I'm running into a deadlock in non-e10s-mode.

* The deadlock consists of main thread (gUM()) waiting for a Mutex in CamerasChild,
vs,
* CamerasChild holding that mutex, waiting for a reply to SendReleaseCaptureDevice - with the receiving CamerasParent having dispatched the runnable responsible for responding to the main thread (see https://hg.mozilla.org/mozilla-central/annotate/2c1fb007137dcb68b1862a79553b53f1a34c99c3/dom/media/systemservices/CamerasParent.cpp#l648).

gcp, jib, how do you suggest solving this?
My two questions right now are,
  Shouldn't IPC stuff run off the main thread?
  Can we fix whatever issue we had on Mac with shutdown to make sure the CaptureDevice is not released on main thread?


In detail:

On main thread we have MediaManager::EnumerateRawDevices() wanting to talk to CamerasParent:
> * thread #1: tid = 0x39d4e7, 0x00007fff8fc8d166 libsystem_kernel.dylib`__psynch_mutexwait + 10, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
>     frame #0: 0x00007fff8fc8d166 libsystem_kernel.dylib`__psynch_mutexwait + 10
>     frame #1: 0x00007fff94719696 libsystem_pthread.dylib`_pthread_mutex_lock + 480
>     frame #2: 0x0000000101331f9e libnss3.dylib`PR_Lock(lock=0x00000001156206c0) + 46 at ptsynch.c:177
>     frame #3: 0x0000000101629197 XUL`mozilla::OffTheBooksMutex::Lock(this=0x00000001159e85b0) + 23 at BlockingResourceBase.cpp:382
>     frame #4: 0x00000001037c0b57 XUL`mozilla::camera::CamerasChild::NumberOfCapabilities(mozilla::camera::CaptureEngine, char const*) [inlined] mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock(aLock=0x00000001159e85b0) + 39 at Mutex.h:164
>     frame #5: 0x00000001037c0b4e XUL`mozilla::camera::CamerasChild::NumberOfCapabilities(mozilla::camera::CaptureEngine, char const*) [inlined] mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock(aLock=0x00000001159e85b0) at Mutex.h:161
>     frame #6: 0x00000001037c0b4e XUL`mozilla::camera::CamerasChild::NumberOfCapabilities(this=0x00000001159e8540, aCapEngine=CameraEngine, deviceUniqueIdUTF8="0x1a11000005ac8510") + 30 at CamerasChild.cpp:245
>     frame #7: 0x0000000103826389 XUL`mozilla::MediaEngineRemoteVideoSource::NumCapabilities(this=0x0000000115458b00) + 41 at MediaEngineRemoteVideoSource.cpp:350
>     frame #8: 0x0000000103821af5 XUL`mozilla::MediaEngineCameraVideoSource::GetBestFitnessDistance(this=0x0000000115458b00, aConstraintSets=0x00007fff5fbfcaa8, aDeviceId=0x00007fff5fbfc9e8) + 37 at MediaEngineCameraVideoSource.cpp:111
>     frame #9: 0x000000010367b707 XUL`mozilla::MediaDevice::GetBestFitnessDistance(this=<unavailable>, aConstraintSets=0x00007fff5fbfcaa8) + 231 at MediaManager.cpp:535
>   * frame #10: 0x00000001036f09f4 XUL`char const* mozilla::MediaConstraintsHelper::SelectSettings<mozilla::VideoDevice>(aConstraints=0x0000000107c9de28, aSources=0x00007fff5fbfcd90) + 276 at MediaTrackConstraints.h:148
>     frame #11: 0x000000010368f78c XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21>(mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21)::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) [inlined] mozilla::SelectSettings(aConstraints=<unavailable>, aSources=<unavailable>) + 5 at MediaManager.cpp:1162
>     frame #12: 0x000000010368f787 XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21>(mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21)::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) + 433 at MediaManager.cpp:2040
>     frame #13: 0x000000010368f5d6 XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::GetUserMedia(this=0x0000000112c6e000, result=<unavailable>)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21>(mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_20, mozilla::MediaManager::GetUserMedia(nsPIDOMWindow*, mozilla::dom::MediaStreamConstraints const&, nsIDOMGetUserMediaSuccessCallback*, nsIDOMGetUserMediaErrorCallback*)::$_21)::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) + 134 at MediaUtils.h:87
>     frame #14: 0x0000000103690574 XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&))::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) [inlined] mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve() + 212 at MediaUtils.h:132
>     frame #15: 0x000000010369056f XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&))::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) [inlined] mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve(nsTArray<nsRefPtr<mozilla::MediaDevice> >* const&) + 40 at MediaUtils.h:111
>     frame #16: 0x0000000103690547 XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&))::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) [inlined] mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)::operator()(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) + 146 at MediaManager.cpp:2245
>     frame #17: 0x00000001036904b5 XUL`void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(this=<unavailable>, result=<unavailable>)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&), void mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Then<mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&)>(mozilla::MediaManager::EnumerateDevicesImpl(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_22::operator()(nsCString const&)::'lambda'(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&))::'lambda'(mozilla::dom::MediaStreamError*&))::Functors::Succeed(nsTArray<nsRefPtr<mozilla::MediaDevice> >*&) + 21 at MediaUtils.h:87
>     frame #18: 0x000000010368f4b8 XUL`mozilla::media::LambdaRunnable<mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_19::operator()()::'lambda'()>::Run() [inlined] mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve() + 136 at MediaUtils.h:132
>     frame #19: 0x000000010368f4b3 XUL`mozilla::media::LambdaRunnable<mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_19::operator()()::'lambda'()>::Run() [inlined] mozilla::media::Pledge<nsTArray<nsRefPtr<mozilla::MediaDevice> >*, mozilla::dom::MediaStreamError*>::Resolve(nsTArray<nsRefPtr<mozilla::MediaDevice> >* const&) + 33 at MediaUtils.h:111
>     frame #20: 0x000000010368f492 XUL`mozilla::media::LambdaRunnable<mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_19::operator()()::'lambda'()>::Run() [inlined] mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_19::operator()()::'lambda'()::operator()() + 82 at MediaManager.cpp:1456
>     frame #21: 0x000000010368f440 XUL`mozilla::media::LambdaRunnable<mozilla::MediaManager::EnumerateRawDevices(unsigned long long, mozilla::dom::MediaSourceEnum, mozilla::dom::MediaSourceEnum, bool, bool)::$_19::operator()()::'lambda'()>::Run(this=<unavailable>) + 16 at MediaUtils.h:194
>     frame #22: 0x00000001015ff839 XUL`nsThread::ProcessNextEvent(this=0x000000010043d900, aMayWait=<unavailable>, aResult=0x00007fff5fbfcf77) + 1481 at nsThread.cpp:960


And on a background thread (MediaManager?) we have the release of a camera waiting for a reply from CamerasParent:
> * thread #79: tid = 0x39d5f0, 0x00007fff8fc8d136 libsystem_kernel.dylib`__psynch_cvwait + 10
>   * frame #0: 0x00007fff8fc8d136 libsystem_kernel.dylib`__psynch_cvwait + 10
>     frame #1: 0x00007fff9471c560 libsystem_pthread.dylib`_pthread_cond_wait + 693
>     frame #2: 0x0000000101332743 libnss3.dylib`PR_WaitCondVar(cvar=0x00000001004deb00, timeout=<unavailable>) + 419 at ptsynch.c:396
>     frame #3: 0x00000001016294c4 XUL`mozilla::CondVar::Wait(this=0x00000001159e85f0, aInterval=<unavailable>) + 68 at BlockingResourceBase.cpp:501
>     frame #4: 0x00000001037c0e81 XUL`mozilla::camera::CamerasChild::DispatchToParent(nsIRunnable*, mozilla::MonitorAutoLock&) [inlined] mozilla::Monitor::Wait(this=<unavailable>, aInterval=4294967295) + 193 at Monitor.h:40
>     frame #5: 0x00000001037c0e73 XUL`mozilla::camera::CamerasChild::DispatchToParent(nsIRunnable*, mozilla::MonitorAutoLock&) [inlined] mozilla::MonitorAutoLock::Wait(this=0x0000000135ba7988, aInterval=4294967295) + 17 at Monitor.h:88
>     frame #6: 0x00000001037c0e62 XUL`mozilla::camera::CamerasChild::DispatchToParent(this=0x00000001159e8540, aRunnable=<unavailable>, aMonitor=0x0000000135ba7988) + 162 at CamerasChild.cpp:232
>     frame #7: 0x00000001037c1a29 XUL`mozilla::camera::CamerasChild::ReleaseCaptureDevice(this=0x00000001159e8540, aCapEngine=<unavailable>, capture_id=<unavailable>) + 201 at CamerasChild.cpp:483
>     frame #8: 0x0000000103825a71 XUL`mozilla::MediaEngineRemoteVideoSource::Deallocate(this=0x0000000115458b00) + 129 at MediaEngineRemoteVideoSource.cpp:146
>     frame #9: 0x00000001036a8e64 XUL`mozilla::MediaOperationTask::Run(this=0x000000012bcbaf80) + 308 at MediaManager.cpp:315
>     frame #10: 0x00000001019a52ac XUL`MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [inlined] MessageLoop::RunTask(this=0x0000000135ba7d00, task=0x000000012bcbaf80) + 69 at message_loop.cc:364
>     frame #11: 0x00000001019a5267 XUL`MessageLoop::DeferOrRunPendingTask(this=0x0000000135ba7d00, pending_task=<unavailable>) + 119 at message_loop.cc:372
>     frame #12: 0x00000001019a55eb XUL`MessageLoop::DoWork(this=0x0000000135ba7d00) + 235 at message_loop.cc:459
>     frame #13: 0x00000001019df3a5 XUL`mozilla::ipc::DoWorkRunnable::Run(this=<unavailable>) + 53 at MessagePump.cpp:220
>     frame #14: 0x00000001015ff839 XUL`nsThread::ProcessNextEvent(this=0x0000000112f27200, aMayWait=<unavailable>, aResult=0x0000000135ba7bf7) + 1481 at nsThread.cpp:960
Flags: needinfo?(jib)
Flags: needinfo?(gpascutto)
Comment on attachment 8668794 [details]
MozReview Request: Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot

Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot
Attachment #8668795 - Attachment description: MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib → MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot
Attachment #8668795 - Flags: review?(padenot)
Comment on attachment 8668796 [details]
MozReview Request: Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot

Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot
Comment on attachment 8668797 [details]
MozReview Request: Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot

Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot
Comment on attachment 8668798 [details]
MozReview Request: Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot

Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot
Comment on attachment 8668799 [details]
MozReview Request: Bug 1070216 - Test MediaStream Constructors. r?jib

Bug 1070216 - Test MediaStream Constructors. r?jib
Bug 1070216 - Guard against a null MediaInputPort in DOMMediaStream::FindPlaybackDOMTrack(). r?roc
Attachment #8668968 - Flags: review?(roc)
Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
We currently avoid Deallocating a CaptureDevice used for multiple gUMStreams
when one of them calls Deallocate() by keeping track of how many called
Start().

The normal lifetime sequence however, is:
Allocate()
Start()
Stop()
Deallocate()

This patches fixes the lifetime management by keeping track of how many
users of the CaptureDevice called Allocate().
Attachment #8668969 - Flags: review?(jib)
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

r+ for the webidl
Attachment #8668795 - Flags: review?(bugs) → review+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #32)

> gcp, jib, how do you suggest solving this?
> My two questions right now are,
>   Shouldn't IPC stuff run off the main thread?

IPC *is* running off of the main thread here, that's not the issue. What you're seeing is that the WebRTC capture API is synchronous so CamerasChild has to block hard on responses.

>   Can we fix whatever issue we had on Mac with shutdown to make sure the
> CaptureDevice is not released on main thread?

If I knew of a fix I wouldn't have put that workaround.

We might be able to make that workaround a SyncRunnable so the reply gets dispatched before anyone else has a chance to try initiating gUM, but I'm not 100% sure this works.

Anyway, why are we doing gUM on the main thread, let alone Capability querying? You can cause whole minutes of yank.
Flags: needinfo?(gpascutto)
Depends on: 1210852
Comment on attachment 8665902 [details]
MozReview Request: Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc

https://reviewboard.mozilla.org/r/20389/#review19087
Attachment #8665902 - Flags: review?(roc) → review+
Comment on attachment 8668793 [details]
MozReview Request: Bug 1070216 - constify DOMMediaStream::Get[Audio/Video]Tracks(). r?roc

https://reviewboard.mozilla.org/r/21033/#review19089
Attachment #8668793 - Flags: review?(roc) → review+
Comment on attachment 8668968 [details]
MozReview Request: Bug 1070216 - Guard against a null MediaInputPort in DOMMediaStream::FindPlaybackDOMTrack(). r?roc

https://reviewboard.mozilla.org/r/21071/#review19093
Attachment #8668968 - Flags: review?(roc) → review+
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

https://reviewboard.mozilla.org/r/21037/#review19133
Attachment #8668795 - Flags: review?(padenot) → review+
Blocks: 910249
No longer depends on: 910249
Comment on attachment 8668969 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib

https://reviewboard.mozilla.org/r/21073/#review19313

lgtm.
Attachment #8668969 - Flags: review?(jib) → review+
Comment on attachment 8668969 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib

https://reviewboard.mozilla.org/r/21073/#review19315

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:140
(Diff revision 1)
> -    MonitorAutoLock lock(mMonitor);
> -    empty = mSources.IsEmpty();
> -  }
> +  MOZ_ASSERT(mNrAllocations >= 0, "Double-deallocations are prohibited");
> +
> +  if (mNrAllocations == 0) {

Hmm, one sec, the old code used a lock here, the new decrement code doesn't. Is Deallocate() called from other threads?
Attachment #8668969 - Flags: review+
If all accesses of mNrAllocations are on the same thread then it would be nice if the code asserted that.
Flags: needinfo?(jib)
Target Milestone: --- → mozilla44
Comment on attachment 8665902 [details]
MozReview Request: Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc

Bug 1070216 - Split DOMMediaStream::InitStreamCommon into three. r?roc
This allows us to initiate a DOMMediaStream into three states:
* One that has input, owned and playback streams, for data producers
  like gUM or RTCPeerConnection.
* One with owned and playback streams, for cloned DOM streams.
  If a cloned DOM stream has an empty input stream connected to its
  owned stream, both are regarded as not having current data.
* One with only a playback stream, for when it has been created with the
  default constructor from JS. Its track set can only be changed by
  addTrack() and removeTrack() which when called only affect mPlaybackStream.
Comment on attachment 8668793 [details]
MozReview Request: Bug 1070216 - constify DOMMediaStream::Get[Audio/Video]Tracks(). r?roc

Bug 1070216 - constify DOMMediaStream::Get[Audio/Video]Tracks(). r?roc
Comment on attachment 8668794 [details]
MozReview Request: Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot

Bug 1070216 - Guard against adding a track owned by one MSG to a stream owned by another. r?padenot
Comment on attachment 8668795 [details]
MozReview Request: Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot

Bug 1070216 - Implement MediaStream constructors. r?smaug,jib,padenot
Attachment #8668795 - Flags: review+
Comment on attachment 8668796 [details]
MozReview Request: Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot

Bug 1070216 - Make it possible to disable AudioStreamAnalyser's debug canvas. r?padenot
Comment on attachment 8668797 [details]
MozReview Request: Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot

Bug 1070216 - Let AudioStreamAnalyser accept streams with no tracks. r?padenot
Comment on attachment 8668798 [details]
MozReview Request: Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot

Bug 1070216 - Break out createOscillatorStream from test_gUM_addTrackRemoveTrack.html to test framework. r?padenot
Comment on attachment 8668799 [details]
MozReview Request: Bug 1070216 - Test MediaStream Constructors. r?jib

Bug 1070216 - Test MediaStream Constructors. r?jib
Comment on attachment 8668968 [details]
MozReview Request: Bug 1070216 - Guard against a null MediaInputPort in DOMMediaStream::FindPlaybackDOMTrack(). r?roc

Bug 1070216 - Guard against a null MediaInputPort in DOMMediaStream::FindPlaybackDOMTrack(). r?roc
Comment on attachment 8668969 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib

Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
We currently avoid Deallocating a CaptureDevice used for multiple gUMStreams
when one of them calls Deallocate() by keeping track of how many called
Start().

The normal lifetime sequence however, is:
Allocate()
Start()
Stop()
Deallocate()

This patches fixes the lifetime management by keeping track of how many
users of the CaptureDevice called Allocate().
Attachment #8668969 - Flags: review?(jib)
Bug 1070216 - Assert main MediaEngine APIs are called on the owning thread. r?jib
Attachment #8673449 - Flags: review?(jib)
Comment on attachment 8673449 [details]
MozReview Request: Bug 1070216 - Assert main MediaEngine APIs are called on the owning thread. r?jib

https://reviewboard.mozilla.org/r/21925/#review19707

::: dom/media/webrtc/MediaEngine.h:13
(Diff revision 1)
> +#include "nsPrintfCString.h"

litter?
Attachment #8673449 - Flags: review?(jib) → review+
Comment on attachment 8668969 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib

https://reviewboard.mozilla.org/r/21073/#review19711

No change (bug 1169360)
Attachment #8668969 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #64)
> Comment on attachment 8668969 [details]
> MozReview Request: Bug 1070216 - Properly manage lifetime of allocated
> CaptureDevices. r?jib
> 
> https://reviewboard.mozilla.org/r/21073/#review19711
> 
> No change (bug 1169360)

Well, you unshipped it before :-)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #63)
> Comment on attachment 8673449 [details]
> MozReview Request: Bug 1070216 - Assert main MediaEngine APIs are called on
> the owning thread. r?jib
> 
> https://reviewboard.mozilla.org/r/21925/#review19707
> 
> ::: dom/media/webrtc/MediaEngine.h:13
> (Diff revision 1)
> > +#include "nsPrintfCString.h"
> 
> litter?

Seems so indeed, I think I misinterpreted a compilation failure and added it then.
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #65)
> Well, you unshipped it before :-)

Whoops, you're right. :*) Looks like commenting in mozReview automatically does that unless I remember to uncheck "open an issue".
No longer depends on: 1223696
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: