mozGetUserMedia doesn't support getting both audio and video in one stream

VERIFIED FIXED in mozilla19

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia], [blocking-gum+])

Attachments

(2 attachments, 6 obsolete attachments)

mozGetUserMedia({video:true, audio:true},...) doesn't currently work; you have to get separate streams and merge them with new MediaStream(...)
Depends on: 691234
Depends on: 758391
Whiteboard: [getUserMedia], [blocking-gum+]
QA Contact: jsmith
Duplicate of this bug: 799097
Attachment #671189 - Flags: feedback?(anant)
fix for non-merged streams
Attachment #671189 - Attachment is obsolete: true
Attachment #671189 - Flags: feedback?(anant)
Attachment #671199 - Flags: feedback?(anant)
This appears to do something, but media doesn't get through and displayed.  Haven't figured out what is broken yet; perhaps media insertion, perhaps some callback.
Attachment #671199 - Attachment is obsolete: true
Attachment #671199 - Flags: feedback?(anant)
Comment on attachment 671333 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

This version works, and gets audio and video.  Also added NSPR logging for MediaManager/Engine under "MediaManager".
Attachment #671333 - Flags: review?(roc)
FYI, looking at one of the unit tests, I probably should set the CreateInputStream() hints to BLAH_AUDIO|BLAH_VIDEO for the dual-stream case (assuming one is audio and one is video) instead of 0 for the a+v case.  I saw it was an enum and this shied away from ORing the values together.
-> critical because of the pain this inflicts on people trying to use getUserMedia as intended
Severity: normal → critical
Comment on attachment 671333 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

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

I feel like someone else should review this as well. Maybe Anant?

::: dom/media/MediaManager.cpp
@@ +249,2 @@
>      }
> +    stream = nsDOMMediaStream::CreateInputStream(hints);

You might as well pass the correct hints here.

@@ +297,4 @@
>    StreamListeners* mListeners;
>    uint64_t mWindowID;
> +  TrackID mTrackID1;
> +  TrackID mTrackID2;

Maybe here we should have mVideoSource, mAudioSource, mVideoTrackID, mAudioTrackID?

Though, why do we haven have variable track IDs? can't you define a constant track ID for audio and another one for video? Since you choose the track IDs.

@@ +334,5 @@
> +      MOZ_ASSERT(!(mAudio && mVideo));
> +      if (mAudio)
> +        mAudioDevice = aDevice;
> +      else
> +        mVideoDevice = aDevice;

{} around if/else bodies

@@ +451,5 @@
> +    MOZ_ASSERT(!(mAudio && mVideo));
> +    if (mAudio)
> +      mAudioDevice = aDevice;
> +    else
> +      mVideoDevice = aDevice;

{}

::: dom/media/MediaManager.h
@@ +126,5 @@
>    void NotifyQueuedTrackChanges(MediaStreamGraph* aGraph, TrackID aID,
>      TrackRate aTrackRate, TrackTicks aTrackOffset,
> +                                uint32_t aTrackEvents, const MediaSegment& aQueuedMedia) {}
> +  void NotifyHasCurrentData(MediaStreamGraph* aGraph,
> +                            bool aHasCurrentData) {}

Note that we have a default implementation of most of the methods so you don't need to override with an empty body.

@@ +132,4 @@
>  
>  private:
> +  nsRefPtr<MediaEngineSource> mSource1;
> +  nsRefPtr<MediaEngineSource> mSource2;

Maybe just bite the bullet and have an nsTArray<nsRefPtr<MediaEngineSource> > and use loops instead of duplicated code. Ends up being the same amount of code but should look cleaner.

@@ +136,3 @@
>    nsCOMPtr<nsDOMMediaStream> mStream;
> +  TrackID mID1;
> +  TrackID mID2;

nsTArray<TrackID> then
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 671333 [details] [diff] [review]
> Support getting audio and video in the same getUserMedia call
> 
> Review of attachment 671333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like someone else should review this as well. Maybe Anant?

I can have him look as well.

> ::: dom/media/MediaManager.cpp
> @@ +249,2 @@
> >      }
> > +    stream = nsDOMMediaStream::CreateInputStream(hints);
> 
> You might as well pass the correct hints here.

Yes.

> @@ +297,4 @@
> >    StreamListeners* mListeners;
> >    uint64_t mWindowID;
> > +  TrackID mTrackID1;
> > +  TrackID mTrackID2;
> 
> Maybe here we should have mVideoSource, mAudioSource, mVideoTrackID,
> mAudioTrackID?
> 
> Though, why do we haven have variable track IDs? can't you define a constant
> track ID for audio and another one for video? Since you choose the track IDs.

Perhaps overly generic, but I wrote this part of the code to not care what the two streams it was given are.  Basically nothing but the main GetUserMedia Run() that calls ProcessGetUserMedia() knows what stream1 and stream2 are.

> @@ +334,5 @@
> > +      MOZ_ASSERT(!(mAudio && mVideo));
> > +      if (mAudio)
> > +        mAudioDevice = aDevice;
> > +      else
> > +        mVideoDevice = aDevice;
> 
> {} around if/else bodies

Ok.

> @@ +451,5 @@
> > +    MOZ_ASSERT(!(mAudio && mVideo));
> > +    if (mAudio)
> > +      mAudioDevice = aDevice;
> > +    else
> > +      mVideoDevice = aDevice;
> 
> {}

ok

> ::: dom/media/MediaManager.h
> @@ +126,5 @@
> >    void NotifyQueuedTrackChanges(MediaStreamGraph* aGraph, TrackID aID,
> >      TrackRate aTrackRate, TrackTicks aTrackOffset,
> > +                                uint32_t aTrackEvents, const MediaSegment& aQueuedMedia) {}
> > +  void NotifyHasCurrentData(MediaStreamGraph* aGraph,
> > +                            bool aHasCurrentData) {}
> 
> Note that we have a default implementation of most of the methods so you
> don't need to override with an empty body.

Ok.  I added HasCurrentData while debugging what Notify...()s we were getting (and had debugs in the {}'s).

> @@ +132,4 @@
> >  
> >  private:
> > +  nsRefPtr<MediaEngineSource> mSource1;
> > +  nsRefPtr<MediaEngineSource> mSource2;
> 
> Maybe just bite the bullet and have an nsTArray<nsRefPtr<MediaEngineSource>
> > and use loops instead of duplicated code. Ends up being the same amount of
> code but should look cleaner.

Yeah, I was thinking about that.

> @@ +136,3 @@
> >    nsCOMPtr<nsDOMMediaStream> mStream;
> > +  TrackID mID1;
> > +  TrackID mID2;
> 
> nsTArray<TrackID> then

Yup.
Attachment #671333 - Attachment is obsolete: true
Attachment #671333 - Flags: review?(roc)
Comment on attachment 671394 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

Note that I didn't convert to an nsTArray<>.  The W3 WG has explicitly removed support for getting more than two streams at once to simplify constraints.  I added a comment block about this.  I also removed all the empty Notify's that were there.
Attachment #671394 - Flags: review?(roc)
Attachment #671394 - Flags: review?(anant)
Attachment #671394 - Attachment is obsolete: true
Attachment #671394 - Flags: review?(roc)
Attachment #671394 - Flags: review?(anant)
Comment on attachment 671434 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

Add some small cleanup to the MediaEngineWebRTCAudioSource::Process() function
Attachment #671434 - Flags: review?(roc)
Attachment #671434 - Flags: review?(anant)
Comment on attachment 671434 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

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

Looks great! Just a few nits:

::: dom/media/MediaManager.cpp
@@ +240,1 @@
>  

I also think mVideoSource/mAudioSource makes more sense, especially since we're only going to support these two tracks (it's not possible to have two video tracks or two audio tracks in a single stream, for example).

That means some of the code that assumes mSource1 is always set will have to be changed, but it makes the code a bit cleaner IMHO.

@@ +252,5 @@
> +                      nsDOMMediaStream::HINT_CONTENTS_AUDIO);
> +    if (mSource2) {
> +      hints |= (mTrackID2 == kVideoTrack ?
> +                nsDOMMediaStream::HINT_CONTENTS_VIDEO :
> +                nsDOMMediaStream::HINT_CONTENTS_AUDIO);

We can simplify all this code by simply making nsDOMMediaStream::HINT_* be the track ID, they're constants anyway (and it would eliminate two arguments to the already long constructor).

@@ +264,5 @@
> +    if (!stream) {
> +      if (activeWindows->Get(mWindowID)) {
> +        nsCOMPtr<nsIDOMGetUserMediaErrorCallback> error(mError);
> +        LOG(("Returning error for getUserMedia() - no stream"));
> +        error->OnError(NS_LITERAL_STRING("NO STREAM"));

We use "_" for all the other error strings, so maybe "NO_STREAM"?

@@ +344,5 @@
> +        mAudioDevice = aDevice;
> +      } else {
> +        mVideoDevice = aDevice;
> +      }
> +    }

Using mAudioDevice/mVideoDevice as member variables is one more reason to use mAudioSource/mVideoSource instead of mSource1/mSource2 :)

@@ +528,5 @@
> +        aSource1->Deallocate();
> +        NS_DispatchToMainThread(new ErrorCallbackRunnable(
> +          mSuccess, mError, NS_LITERAL_STRING("HARDWARE_UNAVAILABLE"), mWindowID
> +                                                          ));
> +        return;

Indentation.

::: dom/media/MediaManager.h
@@ +105,5 @@
>      if (aConsuming == CONSUMED) {
>        SourceMediaStream* stream = mStream->GetStream()->AsSourceStream();
> +      mSource1->Start(stream, mID1);
> +      if (mSource2)
> +        mSource2->Start(stream, mID2);

Braces around if.
Attachment #671434 - Flags: review?(anant) → review+
Attachment #671434 - Attachment is obsolete: true
Attachment #671434 - Flags: review?(roc)
Comment on attachment 671508 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

Separate Audio and Video sources now; got rid of passing the trackIds around.

Also stopped asking for Pictures on Desktop from causing an assertion due to GetUserMediaRunnable::Run() getting called on Mainthread.  It doesn't work yet, but it shouldn't cause an assertion.
Attachment #671508 - Flags: review?(roc)
Attachment #671508 - Flags: review?(anant)
Fixes the single 'aDevice' specified from content.  Doesn't include any content changes for stuff that uses the MediaOptions 'device' property
Attachment #671508 - Attachment is obsolete: true
Attachment #671508 - Flags: review?(roc)
Attachment #671508 - Flags: review?(anant)
Comment on attachment 671532 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

I promise to stop changing it. :-)
Attachment #671532 - Flags: review?(roc)
Attachment #671532 - Flags: review?(anant)
Comment on attachment 671532 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

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

this looks good to me

::: dom/media/nsIDOMNavigatorUserMedia.idl
@@ +43,5 @@
>    readonly attribute boolean video;
>    readonly attribute boolean picture;
>    readonly attribute DOMString camera;
> +  readonly attribute nsIMediaDevice audiodevice;
> +  readonly attribute nsIMediaDevice videodevice;

audioDevice, videoDevice
Attachment #671532 - Flags: review+
Attachment #671532 - Flags: review?(roc)
Attachment #671532 - Flags: review?(anant)
Attachment #671532 - Flags: review+
Blocks: 801843
https://hg.mozilla.org/mozilla-central/rev/b80e81edc9f1
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
Just finished off a test run on this feature, so this has been fully tested with an initial test pass. Follow up bugs are already on file.
Status: RESOLVED → VERIFIED
Keywords: verifyme
A Mochitest for this feature will be part of my work on bug 796890.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.