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

VERIFIED FIXED in mozilla19

Status

()

Core
WebRTC
--
critical
VERIFIED FIXED
5 years ago
5 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)

(Assignee)

Description

5 years ago
mozGetUserMedia({video:true, audio:true},...) doesn't currently work; you have to get separate streams and merge them with new MediaStream(...)
(Assignee)

Updated

5 years ago
Depends on: 691234
Depends on: 758391

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+]

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Duplicate of this bug: 799097
(Assignee)

Comment 2

5 years ago
Created attachment 671189 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call
(Assignee)

Updated

5 years ago
Attachment #671189 - Flags: feedback?(anant)
(Assignee)

Comment 3

5 years ago
Created attachment 671199 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

fix for non-merged streams
(Assignee)

Updated

5 years ago
Attachment #671189 - Attachment is obsolete: true
Attachment #671189 - Flags: feedback?(anant)
(Assignee)

Updated

5 years ago
Attachment #671199 - Flags: feedback?(anant)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 671333 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call
(Assignee)

Updated

5 years ago
Attachment #671199 - Attachment is obsolete: true
Attachment #671199 - Flags: feedback?(anant)
(Assignee)

Comment 6

5 years ago
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)
Blocks: 796890
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
-> 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
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 671394 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call
(Assignee)

Updated

5 years ago
Attachment #671333 - Attachment is obsolete: true
Attachment #671333 - Flags: review?(roc)
(Assignee)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
Created attachment 671434 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call
(Assignee)

Updated

5 years ago
Attachment #671394 - Attachment is obsolete: true
Attachment #671394 - Flags: review?(roc)
Attachment #671394 - Flags: review?(anant)
(Assignee)

Comment 14

5 years ago
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+
(Assignee)

Comment 16

5 years ago
Created attachment 671507 [details]
Updated gUM test with audio/video support
(Assignee)

Comment 17

5 years ago
Created attachment 671508 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call
(Assignee)

Updated

5 years ago
Attachment #671434 - Attachment is obsolete: true
Attachment #671434 - Flags: review?(roc)
(Assignee)

Comment 18

5 years ago
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)
(Assignee)

Comment 19

5 years ago
Created attachment 671532 [details] [diff] [review]
Support getting audio and video in the same getUserMedia call

Fixes the single 'aDevice' specified from content.  Doesn't include any content changes for stuff that uses the MediaOptions 'device' property
(Assignee)

Updated

5 years ago
Attachment #671508 - Attachment is obsolete: true
Attachment #671508 - Flags: review?(roc)
Attachment #671508 - Flags: review?(anant)
(Assignee)

Comment 20

5 years ago
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+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80e81edc9f1
(Assignee)

Updated

5 years ago
Blocks: 801843
https://hg.mozilla.org/mozilla-central/rev/b80e81edc9f1
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
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.