Closed
Bug 773649
Opened 12 years ago
Closed 12 years ago
mozGetUserMedia doesn't support getting both audio and video in one stream
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla19
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Whiteboard: [getUserMedia], [blocking-gum+])
Attachments
(2 files, 6 obsolete files)
mozGetUserMedia({video:true, audio:true},...) doesn't currently work; you have to get separate streams and merge them with new MediaStream(...)
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+]
Updated•12 years ago
|
QA Contact: jsmith
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #671189 -
Flags: feedback?(anant)
Assignee | ||
Comment 3•12 years ago
|
||
fix for non-merged streams
Assignee | ||
Updated•12 years ago
|
Attachment #671189 -
Attachment is obsolete: true
Attachment #671189 -
Flags: feedback?(anant)
Assignee | ||
Updated•12 years ago
|
Attachment #671199 -
Flags: feedback?(anant)
Assignee | ||
Comment 4•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #671199 -
Attachment is obsolete: true
Attachment #671199 -
Flags: feedback?(anant)
Assignee | ||
Comment 6•12 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)
Assignee | ||
Comment 7•12 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•12 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•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #671333 -
Attachment is obsolete: true
Attachment #671333 -
Flags: review?(roc)
Assignee | ||
Comment 12•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #671394 -
Attachment is obsolete: true
Attachment #671394 -
Flags: review?(roc)
Attachment #671394 -
Flags: review?(anant)
Assignee | ||
Comment 14•12 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 15•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #671434 -
Attachment is obsolete: true
Attachment #671434 -
Flags: review?(roc)
Assignee | ||
Comment 18•12 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•12 years ago
|
||
Fixes the single 'aDevice' specified from content. Doesn't include any content changes for stuff that uses the MediaOptions 'device' property
Assignee | ||
Updated•12 years ago
|
Attachment #671508 -
Attachment is obsolete: true
Attachment #671508 -
Flags: review?(roc)
Attachment #671508 -
Flags: review?(anant)
Assignee | ||
Comment 20•12 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+
Updated•12 years ago
|
Attachment #671532 -
Flags: review?(roc)
Attachment #671532 -
Flags: review?(anant)
Attachment #671532 -
Flags: review+
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80e81edc9f1
Comment 23•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b80e81edc9f1
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 24•12 years ago
|
||
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
Comment 25•12 years ago
|
||
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.
Description
•