Initial cut.

May not apply cleanly without the patches from bug 1016476, haven't checked.

Don't assume the appdata field is present when parsing source-level msid. Also, fixup ws a little.

Unrot

Move a change from the multistream patch.

Allow the source-level msid finding logic to run when no media-level msid attrs are present.

Move a hunk from the bundle work.

Actually, this hunk can stay in the bundle patch

Fix a few things, fold in test patch.

Comment on attachment 8537956 [details] [diff] [review] Part 1: msid support Review of attachment 8537956 [details] [diff] [review]: ----------------------------------------------------------------- Whoever can get to it.

Unrot

Comment on attachment 8538821 [details] [diff] [review] Part 1: msid support Review of attachment 8538821 [details] [diff] [review]: ----------------------------------------------------------------- So far so good, but I'm out of time today. ::: dom/media/tests/mochitest/identity/test_peerConnection_peerIdentity.html @@ -35,5 @@ > }); > test.setMediaConstraints([{ > audio: true, > - peerIdentity: id2 > - }, { You folded these into the one stream, why was that necessary? ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp @@ +318,5 @@ > + for (auto i = allMsids.begin(); i != allMsids.end(); ++i) { > + if (allMsidsAreWebrtc || webrtcMsids.count(i->identifier)) { > + // For now, we assume that there is exactly one streamId/trackId pair > + // per m-section. Later on, we'll add handling for multiple remote tracks > + // per m-section. Do we have a bug number for this? I assume that this all depends on being able to compose streams of arbitrary sets of tracks. The logic I was expecting here was that you would produce a list of streams, though you would enforce the fact that the track ID was constant over the msection (within those msids that had the WMS semantic, that is). @@ +337,5 @@ > + if (msection.GetAttributeList().HasAttribute(SdpAttribute::kMsidAttribute)) { > + *msids = msection.GetAttributeList().GetMsid().mMsids; > + } > + > + // Can we find some additional msids in ssrc attributes? It sucks that we have to do this. I've asked whether the a=ssrc version of msid is actually something we need to support, with no answer. I'd like to kill this piece of code, but realize that Chrome compatibility needs it.

(In reply to Martin Thomson [:mt] from comment #13) > Comment on attachment 8538821 [details] [diff] [review] > Part 1: msid support > > Review of attachment 8538821 [details] [diff] [review]: > ----------------------------------------------------------------- > > So far so good, but I'm out of time today. > > ::: dom/media/tests/mochitest/identity/test_peerConnection_peerIdentity.html > @@ -35,5 @@ > > }); > > test.setMediaConstraints([{ > > audio: true, > > - peerIdentity: id2 > > - }, { > > You folded these into the one stream, why was that necessary? > Because the other side was expecting them to be presented as one stream all along, which prior to msid was what happened. > ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp > @@ +318,5 @@ > > + for (auto i = allMsids.begin(); i != allMsids.end(); ++i) { > > + if (allMsidsAreWebrtc || webrtcMsids.count(i->identifier)) { > > + // For now, we assume that there is exactly one streamId/trackId pair > > + // per m-section. Later on, we'll add handling for multiple remote tracks > > + // per m-section. > > Do we have a bug number for this? I assume that this all depends on being > able to compose streams of arbitrary sets of tracks. > > The logic I was expecting here was that you would produce a list of streams, > though you would enforce the fact that the track ID was constant over the > msection (within those msids that had the WMS semantic, that is). Well, unified plan makes the assumption that there is exactly one track per m-line, although maybe simulcast will shake that up. I think this is at the wait-and-see stage. > @@ +337,5 @@ > > + if (msection.GetAttributeList().HasAttribute(SdpAttribute::kMsidAttribute)) { > > + *msids = msection.GetAttributeList().GetMsid().mMsids; > > + } > > + > > + // Can we find some additional msids in ssrc attributes? > > It sucks that we have to do this. I've asked whether the a=ssrc version of > msid is actually something we need to support, with no answer. > > I'd like to kill this piece of code, but realize that Chrome compatibility > needs it. Not just Chrome compat, but everyone who has imitated Chrome.

(In reply to Byron Campen [:bwc] from comment #14) > > The logic I was expecting here was that you would produce a list of streams, > > though you would enforce the fact that the track ID was constant over the > > msection (within those msids that had the WMS semantic, that is). > > Well, unified plan makes the assumption that there is exactly one track > per m-line, although maybe simulcast will shake that up. I think this is at > the wait-and-see stage. I don't think that we should be allowing stuff to go through without understanding it properly. That means that we should be looking for multiple track identifiers and failing if they are different. Until we support simulcast or whatever ends up changing that (I suspect that simulcast won't change anything in this regard; if anything, it will use a=ssrc to signal different encoding parameters, but the track will remain 1-1 with the media section).

Incorporate feedback.

Comment on attachment 8540874 [details] [diff] [review] Part 1: msid support Carry forward r=mt

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=11745ffa09c1 non-unified try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5b083c0eea0

/r/2059 - Bug 1095218 - Part 1: msid support. r=mt /r/2061 - Bug 784517: Multistream support. Pull down these commits: hg pull review -r 1c5c1b754297e108ced87aa5ac420c41a3a46458

https://reviewboard.mozilla.org/r/2061/#review1311 I think that I'm going to have another look at this later. Not that I can see any major problems, but I'm not confident about this review. If you can get a second review, then that works too. ::: dom/media/tests/mochitest/pc.js (Diff revision 1) > + ok(desc.sdp.contains(track.id), I think that we can do better than this. Try: '\na=msid:' + stream.id + ' ' + track.id + '\r' Though for now a RegExp() with stream.id replaced with '[^ ]+' would be fine. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h (Diff revision 1) > - rtcp_transport, filter), > + rtp_transport, rtcp_transport, filter), Do we have a problem if our peer provides duplicate track ID values? It doesn't look like the MediaPipeline needs to have any routing information available to it, is that only on the listener? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp (Diff revision 1) > - //track_id_ = track_id; not threadsafe to change this; and we don't need to > + track_id_ = track_id; Just to confirm, `track_id_` is only updated on the main thread. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp (Diff revision 1) > (conduit_->type() == MediaSessionConduit::AUDIO ?"audio":"video")); Might pay to update the debug statement here to include `track_id_`. Or just use `description_`. ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp (Diff revision 1) > + aTrackPair.mLevel, `MediaPipelineReceive*` use `aTrackPair.mLevel + 1` ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp (Diff revision 1) > - stream->StorePipeline(aTrackPair.mLevel, SdpMediaSection::kVideo, pipeline); > + rv = stream->StorePipeline(aTrack.GetTrackId(), Here is a point where you might need to concern yourself with duplicate track IDs. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (Diff revision 1) > for (uint32_t i = 0; i < tracks.Length(); i++) { Nit: s/uint32_t/size_t/ ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (Diff revision 1) > + AssignNextIdToTrack(tracks[i]); So this looks a little fragile to me. You get an unlabelled track from the media components that you need to apply a label to. Why not use the `TrackID` (assuming that it is set to `mLevel + 1`) as a correlator? That might mean that you need to modularize the code in MediaPipelineFactory so that you can rely on that mapping being reliable. Then you could have a simple mapping of TrackID to MSID-based ID and avoid having multiple lists. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h (Diff revision 1) > + mozilla::RefPtr<mozilla::MediaPipeline> aPipeline); RefPtr<T>& ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (Diff revision 1) > + // bwc: What? How can a track be replaced more than once? Let's not argue in the source code. The problem is that A can be replaced with B, then B replaced with C. I think that your code is the right check. Let's clear this with :jib and get the right answer. (Honestly, I don't understand why we need to pass a stream, or why we might need to ensure that both tracks belong to the same stream. As long as they are basically compatible, this should be OK.) ::: media/webrtc/signaling/test/FakeMediaStreams.h (Diff revision 1) > + nsCOMPtr<nsIUUIDGenerator> uuidgen = Why not just have a static counter rather than create a dependency on the uuid generator? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h (Diff revision 1) > + virtual bool IsVideo() const MOZ_OVERRIDE { return false; } You might want to use an enumeration here rather than a boolean. kAudio/kVideo would be more readable, I think. ::: dom/media/tests/mochitest/mochitest.ini (Diff revision 1) > -# [test_peerConnection_twoVideoStreams.html] > +# Renegotiation is not yet supported Bug number for renegotiation please.

https://reviewboard.mozilla.org/r/2061/#review1315 > I think that we can do better than this. Try: > > '\na=msid:' + stream.id + ' ' + track.id + '\r' > > Though for now a RegExp() with stream.id replaced with '[^ ]+' would be fine. |stream| has no |id| field, so we cannot do this yet. I can do the RegExp thing, though. > Do we have a problem if our peer provides duplicate track ID values? It doesn't look like the MediaPipeline needs to have any routing information available to it, is that only on the listener? I'm not sure what will happen if we get a duplicate track id. I suppose it could not hurt to write a test-case. > Just to confirm, `track_id_` is only updated on the main thread. Yes, and only used from main as well. > So this looks a little fragile to me. You get an unlabelled track from the media components that you need to apply a label to. Why not use the `TrackID` (assuming that it is set to `mLevel + 1`) as a correlator? That might mean that you need to modularize the code in MediaPipelineFactory so that you can rely on that mapping being reliable. > > Then you could have a simple mapping of TrackID to MSID-based ID and avoid having multiple lists. TrackID is not going to be level + 1 for long; we're going to need something else for renegotiation. (track-ids moving from one level to another, old track ids going away and being replaced by new ones on the old level, etc) When a track id moves from level to level, we should do absolutely nothing with the DOMMediaStream/tracks, since that is not the place to have transport-related side-effects. Ideally, we would only update the transport parameters on the MediaPipeline, and leave everything else alone. > Why not just have a static counter rather than create a dependency on the uuid generator? I suppose we could do this. > `MediaPipelineReceive*` use `aTrackPair.mLevel + 1` This c'tor does not take a TrackID, which is what we were passing the level+1 to MediaPipelineReceive for. > You might want to use an enumeration here rather than a boolean. kAudio/kVideo would be more readable, I think. I find |IsVideo| to be pretty readable, personally. Also, keep in mind that this is a pre-existing function; the default impl inherited by MediaPipelineReceiveVideo returned false!

/r/2059 - Bug 1095218 - Part 1: msid support. r=mt /r/2061 - Bug 784517: Multistream support. Pull down these commits: hg pull review -r 797ee503ac2d7a70f63bf70ceba2d028dba828fe

https://reviewboard.mozilla.org/r/2061/#review1475 Almost there. ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp (Diff revision 2) > + std::pair<std::string, std::string> msid; > + nsresult rv = GetIdsFromMsid(*parsed, > + parsed->GetMediaSection(i), > + &msid.first, > + &msid.second); > + > + if (NS_SUCCEEDED(rv)) { > + if (msids.count(msid)) { > + JSEP_SET_ERROR("msid:" << msid.first << " " << msid.second > + << " appears in more than one m-section at level " << i); I think that this isn't quite enough. The tests we need are: - each media section needs to have exactly one track ID for all the a=msid attributes (a media section can't be multiple different tracks at once) - each track ID needs to be unique across all the media sections in the SDP (i.e., if your single track ID is used elsewhere, barf) The follow-on from that is that you can't have a track+stream pair duplicated across media sections. But that is easily addressed by the second check.

/r/2059 - Bug 1095218 - Part 1: msid support. r=mt /r/2061 - Bug 784517: Multistream support. Pull down these commits: hg pull review -r 2b69cce66610dd5de788127dff03aa3bc07f10d4

https://reviewboard.mozilla.org/r/2061/#review1503 Ship It!

https://reviewboard.mozilla.org/r/2057/#review1505 Ship It!

https://reviewboard.mozilla.org/r/2059/#review1507 Ship It!

Check back on try. try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7479c23d4ee non-unified try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a89081c8e202

Actually, scratch that. Need to pick up the patch from bug 1115212 to prevent an intermittent orange. Will need to rebase once that lands, since there's some rot there. try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0482ef778edd non-unified try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f0afd8ade11d

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f513dc1a248 https://hg.mozilla.org/integration/mozilla-inbound/rev/f666641df41e https://hg.mozilla.org/integration/mozilla-inbound/rev/d93f0d813704

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/eecfe4499f38 - turns out you want to run more than m1-4, since on b2g your tests are in m6 on opt, and m12 on debug, like https://treeherder.mozilla.org/logviewer.html#?job_id=5528013&repo=mozilla-inbound or sometimes https://treeherder.mozilla.org/logviewer.html#?job_id=5525891&repo=mozilla-inbound

Or more intermittently, https://treeherder.mozilla.org/logviewer.html#?job_id=5528186&repo=mozilla-inbound on mochitest-e10s.

(In reply to Phil Ringnalda (:philor) from comment #32) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/eecfe4499f38 - turns > out you want to run more than m1-4, since on b2g your tests are in m6 on > opt, and m12 on debug, like > https://treeherder.mozilla.org/logviewer.html#?job_id=5528013&repo=mozilla- > inbound or sometimes > https://treeherder.mozilla.org/logviewer.html#?job_id=5525891&repo=mozilla- > inbound We don't run webrtc tests on b2g anymore, because the emulator is not capable of meeting our timing requirements. I really doubt this backout will help you there.

Looks like we forgot to disable a new test on b2g, actually. Will need to do that before relanding. I'll try to figure out the cause of the intermittent, though.

/r/2059 - Bug 1095218 - Part 1: msid support. r=mt /r/2061 - Bug 1095218 - Part 2: Multistream support. Pull down these commits: hg pull review -r 318d388b055e407475daff003448feae2b020bba

https://reviewboard.mozilla.org/r/2057/#review1757 Only looking at the changes, I see that you are disabling on gonk. Do the changes work on b2g desktop? That's mochitest-1, I think. Here's what I've used recently to get comprehensive WebRTC mochitest coverage: try: -b o -p linux64,linux64-mulet,macosx64,win32,android-api-9,android-api-11,emulator,linux32_gecko,linux64_gecko -u mochitest-1[b2g],mochitest-3[x64,Mulet Linux,10.6,Windows XP,Windows 8],mochitest-5[Android],mochitest-6[b2g] -t none

None of the webrtc tests work reliably on b2g emulator, but I don't know about b2g desktop. Is there a way to disable on b2g emulator only? I would be surprised if there is.

Skipping emulator should be "skip-if = (buildapp == 'b2g' && toolkit == 'gonk')" (based on what most people find they need, skipping b2g desktop but not emulator, with skip-if = (buildapp == 'b2g' && toolkit != 'gonk'))

try run, including patches to intermittents that this patchset trips over: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfca214e0135

Ok, lots of crashes on linux x86 debug e10s M3. After adding some logging, it appears that we're running out of fds and crashing when trying to send a video frame to the parent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c2fa69ddcb nical: This test case has double the normal number of video streams (also double the number of audio streams), could this be intermittently exhausting the pool of available fds when running a debug build on a wimpy AWS instance? I see that each frame gets its own fd, but I'm having some difficulty determining how long that fd is kept open. Do I need to file a bug?

FWIW, running dom/media/tests on my relatively beefy linux box and camping /proc sees the number of fds in use topping out at 153. I'm also seeing some signs of fd leakage. Need to investigate further.

We sometimes run into the FD limit and video indeed helps a lot since each frame has its own fd. Tiling also contributes a lot since each tile has its own shmem. At some point we'll probably have to have some sort of shmem-backed allocator and group several video frames or tiles in the same shmems. (In reply to Byron Campen [:bwc] from comment #42) > I'm also seeing some signs of fd leakage. Need to investigate further. This is pretty bad!

I chatted with Nical; we decided we should re-Try with a patch to not actually render and see if that fixes the issue on Try. If so, we can look at optimizations to have the compositor skip stuff if it falls too far behind. Longer-term, roc's video-timestamp-based compositing may also make this better. Let's do the Try; things may fail but likely elsewhere if we're right. I'd be ok with landing with the test turned off (or somehow turned down in load) for Linux-debug-e10s. (lower framerate somehow, lower resolution perhaps, don't make the video elements visible, etc) If I can figure out the right patchset for Try I'll push one and note here

(In reply to Nicolas Silva [:nical] from comment #43) > (In reply to Byron Campen [:bwc] from comment #42) > > I'm also seeing some signs of fd leakage. Need to investigate further. > > This is pretty bad! After some more investigation, it doesn't seem like a leak, just the chromium fds are sometimes staying open for several seconds. Might be some other part of the system, or maybe an artifact of the OS.

(In reply to Randell Jesup [:jesup] from comment #45) > I chatted with Nical; we decided we should re-Try with a patch to not > actually render and see if that fixes the issue on Try. If so, we can look > at optimizations to have the compositor skip stuff if it falls too far > behind. Longer-term, roc's video-timestamp-based compositing may also make > this better. > > Let's do the Try; things may fail but likely elsewhere if we're right. I'd > be ok with landing with the test turned off (or somehow turned down in load) > for Linux-debug-e10s. (lower framerate somehow, lower resolution perhaps, > don't make the video elements visible, etc) > > If I can figure out the right patchset for Try I'll push one and note here This test does not show the video elements...

I see the following logging a lot in these failing tests, and less-so in other tests: 11:11:54 INFO - [Child 2494] WARNING: DataCallback buffer filled entirely from scratch buffer, skipping iteration.: file /builds/slave/try-lx-d-000000000000000000000/build/src/dom/media/GraphDriver.cpp, line 920 This seems to imply some sort of cycle starvation, or falling behind, but I haven't had time to figure out where.

Paul - I'm not sure those messages imply cycle starvation, from when I remember of that code - though starvation may be occurring

So, I talked to jlund over IRC, and he said the per-process fd limit was 4K, but after adding some logging it looks like the child process is only getting 1K. Not sure what's up with that. I did a more extreme test (64 streams instead of 4) on my linux box (8 hyperthreads, probably higher clock speed too), and was able to get the fd count to grow beyond 1K every time with this test. Once I get into the office, I will perform a large tab-to-tab-to-tab-... test with e10s and the pre-multistream code, and if that runs over the fd limit, I am going to open some bugs and disable the test on linux-debug e10s.

Irritatingly, opentokrtc.com no longer works. talky.io doesn't quite work either (call is established, but no video is rendered). But, setting up 14 simultaneous talky.io calls (28 video streams, compared to the 4 that is set up by the mochitest) causes the 1K fd barrier to be broken on hardware more than 8 times as powerful. This is just a pre-existing problem that we had not hit yet because we weren't pushing the system hard enough.

Ok, have disabled the multistream video tests on e10s debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88eefa79cb8f Looks good so far, but will retrigger our mochitests and crashtests just to be sure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c70466ac38a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/99fc16e53192

https://hg.mozilla.org/mozilla-central/rev/c70466ac38a9 https://hg.mozilla.org/mozilla-central/rev/99fc16e53192

(In reply to Randell Jesup [:jesup] from comment #50) > Paul - I'm not sure those messages imply cycle starvation, from when I > remember of that code - though starvation may be occurring Yes, this has the symptoms of a system API trying to recover from hitting the real-time CPU limit on an audio callback thread. Is this specific to a platform ?

Added to release notes for 38 Aurora/DevEd as: WebRTC now has multistream and renegotiation support

(clearing needinfo)