When you attach a webaudio stream to a PeerConnection, createOffer() fails with "Cannot create SDP without any streams" (with signaling:5 logging). This appears to be due to WebAudio not 'hinting' that it will have an audio track later, given that MediaStreamGraph tracks are lazily constructed on the first data push. It's possible there is more required to get it all to work than just adding the hint, but createOffer() must work to start.

Ugly testcase

Randel, can you write a simple mochitest for this, so we are covered in the future? Let me know if I should do it.

Something roughly equivalent to this is a start at verifying we can make an offer. A better test (once it works) would make a call with (say) all audio over 500Hz filtered out, and then use webaudio to verify the fake audio 1KHz doesn't get through, then up it to a 2K filter (or make it a high-pass) and verify it gets through.

The patch doesn't work because pc.addStream is already implemented as follows in PeerConnection.js [1]: addStream: function(stream) { stream.getTracks().forEach(track => this.addTrack(track, stream)); }, Bug 1032835 comment 15 marks a regression of sorts for hints support. The spec is moving toward a track-based API, which seems fundamentally at odds with streams as containers for tracks arriving later. While we can probably put back the magic of addStream, I'd like to understand the long-term DOM model here. Consider this content workaround: function start() { var pc = new mozRTCPeerConnection(); navigator.mozGetUserMedia({ audio: true }, function(stream) { var context = new AudioContext(); var microphone = context.createMediaStreamSource(stream); var filter = context.createBiquadFilter(); filter.frequency.value = 440; filter.type = 0; var peer = context.createMediaStreamDestination(); microphone.connect(filter); filter.connect(peer); step2(peer.stream); }, function() { log("GetUserMedia failed");}); } function step2(newstream) { if (!newstream.getTracks().length) { log("track not arrived yet. Waiting 10 ms"); setTimeout(() => step2(newstream), 10); return; } pc.addStream(newstream); pc.createOffer(function() { ok("It works!!!"); }, function() { failed("It failed..."); }, null ); } When I run this, I see two "Waiting 10 ms" messages, so it takes about 20 ms for the audio track to appear in the mediaStream. This is clearly observable in content. Is this a bug of ours or a feature of the spec? I see no asynchronous calls on any of the AudioContext objects, and pc.addStream is synchronous as well. If this is a feature, then "fixing" addStream would not be a kludge so much as a feature that addStream implies "add all tracks AND all hints of tracks to come (shortly) in the mediaStream", making it fundamentally different from addTrack. Also, something I don't understand with "streams as containers for tracks arriving later": If a slot in the offer is reserved for a track "arriving later", how do we know there aren't two tracks arriving later? What if a (second) audio track takes longer to arrive, is there a race on createOffer whether the offer goes out with one audio-track or two? Does setLocalDescription wait? If someone could explain how this is supposed to hang together that would be great, so that I know what'll be expected of the kludge as far as behavior. [1] http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=c885916823da&mark=841-841#840

*In theory* the WebRTC WG has assumed that tracks are available after getUserMedia() returns. Realize there are no source for MediaStreams currently in the drafts other than getUserMedia() and WebAudio destination nodes, and constructed-from-tracks MediaStreams. Now, roc's design for MediaStream Processing and stuff like mozCaptureStreamUntilEnded() ties track definitions to what are seen in decoded media sources - tracks can appear out of the blue later. (And go away?) So in theory, it would be nice for getUserMedia streams to have their tracks from the start (even if no data has been received yet), ditto WebAudio (and eventually canvas.captureStream()). video.(moz)captureStream is the odd man out. Perhaps what should be done (and part of the proposal for captureStream()) would be to include known tracks, and when tracks get added (or if any other stream somehow gets a track added) to a stream attached to a PeerConnection we'd fire onnegotiationneeded (which is kinda obvious). The biggest issue (other than making getUserMedia and such create their tracks "early") is how to know if a video element will have audio, video or both (and how many) at the start. (Some type of pre-roll-find-tracks?) Also, creating tracks "early" may cause issues for MSG since data for those tracks may not arrive for a while (especially cameras), and we don't want to insert a black frame. For audio, we could supply 0's until the real audio starts, but we'd have to be careful not the lock in 0-10ms of delay. Perhaps we can just let it underflow. roc? Martin - since you volunteered to write the captureStream draft ;-) any comments?

For the captureStream stuff, it seems like we need to consider having empty streams (i.e., remove the assertion). This hints stuff is going to have to be ripped anyway.

Here's what I think we should do: 1) Continue to have MSG be the source of truth about tracks that have data. 2) Allow a MediaStreamTrack to exist in a state where it doesn't yet have data. Let's call it a placeholder track. 3) When a DOMMediaStream is created, you can add placeholder MediaStreamTracks to it. They will have an ID assigned. 4) When MSG notifies the DOMMediaStream that a track has been created, we bind the track to the placeholder MediaStreamTrack with the same ID if one exists. 5) Rip out the hints code. 6) For captureStream, return an empty stream or a stream with placeholder tracks based on the current state of the video element. Need to take care to remove the placeholder track if the main thread is notified that the track has been removed.

This seems fine.

Overall, this sounds good to me. Thanks (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #10) > Here's what I think we should do: > 1) Continue to have MSG be the source of truth about tracks that have data. > 2) Allow a MediaStreamTrack to exist in a state where it doesn't yet have > data. Let's call it a placeholder track. > 3) When a DOMMediaStream is created, you can add placeholder > MediaStreamTracks to it. They will have an ID assigned. This will be complicated by the fact that GetUserMedia streams are SourceMediaStream + TrackUnion stream (solely to avoid blocking); we'll have to teach TrackUnion about placeholder tracks as well (or have it treat the placeholders as real tracks, but with care not to ever block on a placeholder(? or maybe we do, if the TrackUnion is set to block on missing inputs??)). Oh, to not need TrackUnions there..... ;-) > 4) When MSG notifies the DOMMediaStream that a track has been created, we > bind the track to the placeholder MediaStreamTrack with the same ID if one > exists. > 5) Rip out the hints code. > 6) For captureStream, return an empty stream or a stream with placeholder > tracks based on the current state of the video element. Need to take care to > remove the placeholder track if the main thread is notified that the track > has been removed. At what point will a <video> with a .src = URL know what the "initial" tracks are? Is there anyway we can reliably "preroll" and trigger the captureStream when we know it's prerolled? (I use that term instead of preload, which doesn't imply to me decoding enough to know about what tracks are available).

I like this. (In reply to Randell Jesup [:jesup] from comment #12) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep > > 3) When a DOMMediaStream is created, you can add placeholder > > MediaStreamTracks to it. They will have an ID assigned. > > This will be complicated by the fact that GetUserMedia streams are > SourceMediaStream + TrackUnion stream (solely to avoid blocking); we'll have > to teach TrackUnion about placeholder tracks as well (or have it treat the > placeholders as real tracks, but with care not to ever block on a > placeholder(? or maybe we do, if the TrackUnion is set to block on missing > inputs??)). When is this a problem? So far I've not seen tracks be absent in getUserMedia (or at least not by the time pc.addTrack is called). Do we have a race?

(In reply to Randell Jesup [:jesup] from comment #12) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep > 27 to Oct 12) from comment #10) > > Here's what I think we should do: > > 1) Continue to have MSG be the source of truth about tracks that have data. > > 2) Allow a MediaStreamTrack to exist in a state where it doesn't yet have > > data. Let's call it a placeholder track. > > 3) When a DOMMediaStream is created, you can add placeholder > > MediaStreamTracks to it. They will have an ID assigned. > > This will be complicated by the fact that GetUserMedia streams are > SourceMediaStream + TrackUnion stream (solely to avoid blocking); we'll have > to teach TrackUnion about placeholder tracks as well (or have it treat the > placeholders as real tracks, but with care not to ever block on a > placeholder(? or maybe we do, if the TrackUnion is set to block on missing > inputs??)). I don't see the problem. The DOMMediaStream returned by gUM is associated with the TrackUnionStream. TrackUnionStream will acquire tracks with the right IDs, which will bind to the placeholder tracks present in the DOMMediaStream. This binding will be done by DOMMediaStream; MSG will not be aware of placeholder tracks, which are a DOM-only fiction. > > 4) When MSG notifies the DOMMediaStream that a track has been created, we > > bind the track to the placeholder MediaStreamTrack with the same ID if one > > exists. > > 5) Rip out the hints code. > > 6) For captureStream, return an empty stream or a stream with placeholder > > tracks based on the current state of the video element. Need to take care to > > remove the placeholder track if the main thread is notified that the track > > has been removed. > > At what point will a <video> with a .src = URL know what the "initial" > tracks are? onloadedmetadata (or rather, the Gecko code that fires that event). > Is there anyway we can reliably "preroll" and trigger the > captureStream when we know it's prerolled? (I use that term instead of > preload, which doesn't imply to me decoding enough to know about what tracks > are available). Calling captureStream in an onloadedmetadata handler should work.

This gets us past createOffer to a successful connection, but I'm still not hearing anything on the remote end (the local end sounds fine). Unsurprising perhaps since I didn't get remote sound with the comment 7 workaround either. More to do.

Rebased. I've verified that this patch works with a test that sends a mozCaptureStream video-track over peerConnection, so I might as well put it up for review. I've filed Bug 1081409 on a video problem in peerConnection that this uncovered. See that bug for test-case. Audio doesn't work, but this appears to be a bug in TrackUnionStream in that it fails to call NotifyQueuedTrackChanges on listeners when data comes in from being the mOutputStream of an AudioNodeStream rather than through the regular CopyTrackData calls. From the existence of FilterAudioNodeStreamTrack I take it at least two ways of propagating data are in play here). I tried hooking something together but no success yet (looking at empty segments so I must be notifying from the wrong place). MediaPipeLineTransmit listens to TrackUnionStream for data.

This is an optional patch I used to debug TrackUnionStream from lldb (gdb is gone on OSX Mavericks). Turns out lldb can't crack the TrackUnionStream code being in .h file: (lldb) p this error: incomplete type 'mozilla::TrackUnionStream' named in nested name specifier note: forward declaration of 'mozilla::TrackUnionStream' error: 1 errors parsing expression Refactoring it into a traditional cpp file makes inspection in the debugger work. This pretty much erases hg blame for anything but the declarations, so I don't know if we want to land this. If we think fixing this is desirable, but not like this, I can look at making a smaller less blame-disrupting patch. Let me know.

Comment on attachment 8503510 [details] [diff] [review] add placeholder tracks at DOMMediaStream creation (2) Review of attachment 8503510 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/DOMMediaStream.cpp @@ +340,5 @@ > +{ > + MediaStreamTrack* track = nullptr; > + switch (aType) { > + case MediaSegment::AUDIO: { > + for (uint32_t i = 0; i < mTracks.Length(); ++i) { Length() isn't a uint32_t. Canonically it's likely nsTArray<MediaStreamTrack>::size_type (and maps to size_t, which would be ok and is used all over). @@ +355,5 @@ > + MOZ_ASSERT(mTrackTypesAvailable & HINT_CONTENTS_AUDIO); > + break; > + } > + case MediaSegment::VIDEO: { > + for (uint32_t i = 0; i < mTracks.Length(); ++i) { ditto

Comment on attachment 8503512 [details] [diff] [review] refactor TrackUnionStream into cpp to unconfuse lldb debugger (OPTIONAL) Review of attachment 8503512 [details] [diff] [review]: ----------------------------------------------------------------- Use "hg cp" to copy TrackUnionStream.h to TrackUnionStream.cpp and the diff/history will be much nicer.

applies on top of copy patch

Comment on attachment 8503592 [details] [diff] [review] Copy TrackUnionStream.h to Copy TrackUnionStream.cpp These two patches can be merged.

Mixed nits. Carrying forward r=jesup.

Comment on attachment 8503618 [details] [diff] [review] add placeholder tracks at DOMMediaStream creation (3) r=jesup Try - https://tbpl.mozilla.org/?tree=Try&rev=d5b0b9b0d87e

https://hg.mozilla.org/integration/mozilla-inbound/rev/400c4131858e https://hg.mozilla.org/integration/mozilla-inbound/rev/6442a3773f2a

You're perma-failing. Please fix or backout ASAP. https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2936509&repo=mozilla-inbound

Yes, we'll back it out now.

Backed out for M1 failures (the tests may be wrong now...) https://hg.mozilla.org/integration/mozilla-inbound/rev/03de764d461a https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6442a3773f2a shows the the errors

> /tests/content/media/test/test_mediatrack_consuming_mediastream.html | The length of audioTracks > should be 1. - got 2, expected 1 Sounds like the new track arrives and is being added alongside the placeholder track instead of replacing it?

The HTMLVideoElement tracks are being added twice. Once here: > #0 0x0000000104e38ab8 in mozilla::dom::MediaTrackList::AddTrack(mozilla::dom::MediaTrack*) at /Users/Jan/moz/mozilla-central/content/media/MediaTrackList.cpp:94 > #1 0x0000000104e080fd in mozilla::DOMMediaStream::ConstructMediaTracks(mozilla::dom::AudioTrackList*, mozilla::dom::VideoTrackList*) at /Users/Jan/moz/mozilla-central/content/media/DOMMediaStream.cpp:488 > #2 0x0000000104d2b597 in mozilla::dom::HTMLMediaElement::SetupSrcMediaStreamPlayback(mozilla::DOMMediaStream*) at /Users/Jan/moz/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:2841 > #3 0x0000000104d2acbd in mozilla::dom::HTMLMediaElement::SelectResource() at /Users/Jan/moz/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:837 > #4 0x0000000104d2a919 in mozilla::dom::HTMLMediaElement::SelectResourceWrapper() at /Users/Jan/moz/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:804 > #5 0x0000000104d4673a in nsRunnableMethodImpl<void (mozilla::dom::HTMLMediaElement::*)(), void, true>::Run() at /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/content/html/content/src/../../../../dist/include/nsThreadUtils.h:388 and again here: > #0 0x0000000104e5f533 in mozilla::dom::MediaTrackListListener::NotifyMediaTrackCreated(mozilla::dom::MediaTrack*) at /Users/Jan/moz/mozilla-central/content/media/MediaTrackList.cpp:29 > #1 0x0000000104e084aa in mozilla::DOMMediaStream::NotifyMediaStreamTrackCreated(mozilla::dom::MediaStreamTrack*) at /Users/Jan/moz/mozilla-central/content/media/DOMMediaStream.cpp:529 > #2 0x0000000104e2aae2 in mozilla::DOMMediaStream::StreamListener::TrackChange::Run() at /Users/Jan/moz/mozilla-central/content/media/DOMMediaStream.cpp:77 > #3 0x0000000101ee0f18 in nsThread::ProcessNextEvent(bool, bool*) at /Users/Jan/moz/mozilla-central/xpcom/threads/nsThread.cpp:830 Not sure which one I should remove actually. Maybe the mochitests will tell me.

Removing the latter clears M1 and M3 locally and still makes Bug 1081409 work so I'm submitting it. Try - https://tbpl.mozilla.org/?tree=Try&rev=3bce8636b0cf

merged in copy patch. Carrying forward r=roc.

Comment on attachment 8503846 [details] [diff] [review] add placeholder tracks at DOMMediaStream creation (4) Review of attachment 8503846 [details] [diff] [review]: ----------------------------------------------------------------- This is OK for now but the comment below must be fixed soon! ::: content/media/DOMMediaStream.cpp @@ +313,5 @@ > + if (aHintContents & HINT_CONTENTS_AUDIO) { > + CreateDOMTrack(0, MediaSegment::AUDIO); > + } > + if (aHintContents & HINT_CONTENTS_VIDEO) { > + CreateDOMTrack(0, MediaSegment::VIDEO); Actually this is bad :-(. Instead of just an track type hint, we actually need to pass in the IDs of the tracks that are going to be created, and BindDOMTrack should bind to the track with the right ID. What you have here can't work when there are multiple tracks of a given type.

Yes this definitely needs to be revisited when we support more than one track per type.

Comment on attachment 8503846 [details] [diff] [review] add placeholder tracks at DOMMediaStream creation (4) Review of attachment 8503846 [details] [diff] [review]: ----------------------------------------------------------------- Please file a followup bug and link it to whichever bug is tracking more-than-one-stream-of-a-type (check with byron as to which bug it is if you can't find it)

Seeing M1 intermittents on try: imageCapture crashing? Also, got this odd M3 locally. Not sure what to think of it: 2568 INFO TEST-OK | /tests/dom/media/tests/mochitest/test_getUserMedia_constraints.html | took 3932ms 2569 INFO Error: Unable to restore focus, expect failures and timeouts. [94513] ###!!! ASSERTION: track ended but not found: 'Error', file /Users/Jan/moz/mozilla-central/content/media/DOMMediaStream.cpp, line 90 JavaScript error: file:///Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dist/NightlyDebug.app/Contents/Resources/components/nsHandlerService.js, line 891: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] #01: mozilla::DOMMediaStream::StreamListener::TrackChange::Run() (DOMMediaStream.cpp:92, in XUL) #02: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:831, in XUL) #03: NS_ProcessPendingEvents(nsIThread*, unsigned int) (nsThreadUtils.cpp:207, in XUL) #04: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:99, in XUL) #05: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:362, in XUL) #06: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 17 #07: __CFRunLoopDoSources0 (in CoreFoundation) + 242 #08: __CFRunLoopRun (in CoreFoundation) + 831 #09: CFRunLoopRunSpecific (in CoreFoundation) + 309 #10: RunCurrentEventLoopInMode (in HIToolbox) + 226 #11: ReceiveNextEventCommon (in HIToolbox) + 479 #12: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 65 #13: _DPSNextEvent (in AppKit) + 1434 #14: -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (in AppKit) + 122 #15: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (nsAppShell.mm:118, in XUL) #16: -[NSApplication run] (in AppKit) + 553 #17: nsAppShell::Run() (nsAppShell.mm:635, in XUL) #18: nsAppStartup::Run() (nsAppStartup.cpp:280, in XUL) #19: XREMain::XRE_mainRun() (nsAppRunner.cpp:4165, in XUL) #20: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:4238, in XUL) #21: XRE_main (nsAppRunner.cpp:4452, in XUL) #22: do_main(int, char**, nsIFile*) (nsBrowserApp.cpp:287, in firefox) #23: main (nsBrowserApp.cpp:652, in firefox)

Hopefully this will assuage imageCapture.TakePhoto() Try - https://tbpl.mozilla.org/?tree=Try&rev=d447c9d43540

Comment on attachment 8503877 [details] [diff] [review] add placeholder tracks at DOMMediaStream creation (5) Review of attachment 8503877 [details] [diff] [review]: ----------------------------------------------------------------- Green on Try

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9a6298a202 https://hg.mozilla.org/integration/mozilla-inbound/rev/8870dc5bfa71

Filed Bug 1081819 for the audio part.

https://hg.mozilla.org/mozilla-central/rev/0e9a6298a202 https://hg.mozilla.org/mozilla-central/rev/8870dc5bfa71