msid and multistream support

RESOLVED FIXED in mozilla38

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 38+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 11 obsolete attachments)

53.33 KB, patch
bwc
: review+
Details | Diff | Splinter Review
1.17 KB, patch
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
Details | Review
39 bytes, text/x-review-board-request
Details | Review
Comment hidden (empty)
(Assignee)

Updated

4 years ago
Depends on: 1091242
(Assignee)

Comment 1

3 years ago
Created attachment 8529486 [details] [diff] [review]
Part 1: msid support

Initial cut.
(Assignee)

Updated

3 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 784517
(Assignee)

Comment 2

3 years ago
May not apply cleanly without the patches from bug 1016476, haven't checked.
(Assignee)

Comment 3

3 years ago
Created attachment 8531087 [details] [diff] [review]
Part 1: msid support

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

Updated

3 years ago
Attachment #8529486 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8534676 [details] [diff] [review]
Part 1: msid support

Unrot
(Assignee)

Updated

3 years ago
Attachment #8531087 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8535245 [details] [diff] [review]
Part 1: msid support

Move a change from the multistream patch.
(Assignee)

Updated

3 years ago
Attachment #8534676 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8535253 [details] [diff] [review]
Part 1: msid support

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

Updated

3 years ago
Attachment #8535245 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8537518 [details] [diff] [review]
Part 1: msid support

Move a hunk from the bundle work.
(Assignee)

Updated

3 years ago
Attachment #8535253 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8537536 [details] [diff] [review]
Part 1: msid support

Actually, this hunk can stay in the bundle patch
(Assignee)

Updated

3 years ago
Attachment #8537518 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8537564 [details] [diff] [review]
Part 2: Testing work for msid
(Assignee)

Comment 10

3 years ago
Created attachment 8537956 [details] [diff] [review]
Part 1: msid support

Fix a few things, fold in test patch.
(Assignee)

Updated

3 years ago
Attachment #8537564 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8537536 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Comment on attachment 8537956 [details] [diff] [review]
Part 1: msid support

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

Whoever can get to it.
Attachment #8537956 - Flags: review?(martin.thomson)
Attachment #8537956 - Flags: review?(adam)
(Assignee)

Comment 12

3 years ago
Created attachment 8538821 [details] [diff] [review]
Part 1: msid support

Unrot
(Assignee)

Updated

3 years ago
Attachment #8537956 - Attachment is obsolete: true
Attachment #8537956 - Flags: review?(martin.thomson)
Attachment #8537956 - Flags: review?(adam)
(Assignee)

Updated

3 years ago
Attachment #8538821 - Flags: review?(martin.thomson)
Attachment #8538821 - Flags: review?(adam)
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.
(Assignee)

Comment 14

3 years ago
(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.

Updated

3 years ago
Attachment #8538821 - Flags: review?(martin.thomson) → review+
(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).
(Assignee)

Comment 16

3 years ago
Created attachment 8540874 [details] [diff] [review]
Part 1: msid support

Incorporate feedback.
(Assignee)

Comment 17

3 years ago
Comment on attachment 8540874 [details] [diff] [review]
Part 1: msid support

Carry forward r=mt
Attachment #8540874 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8538821 - Attachment is obsolete: true
Attachment #8538821 - Flags: review?(adam)
(Assignee)

Updated

3 years ago
Depends on: 1115212

Updated

3 years ago
Blocks: 1115823
(Assignee)

Comment 19

3 years ago
Created attachment 8544720 [details]
MozReview Request: bz://1095218/bwc
Attachment #8544720 - Flags: review?(martin.thomson)
Attachment #8544720 - Flags: review?(adam)
(Assignee)

Comment 20

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

Updated

3 years ago
Summary: msid support → msid and multistream support
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.
(Assignee)

Comment 22

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

Comment 23

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

Comment 25

3 years ago
/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

Updated

3 years ago
Attachment #8544720 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 29

3 years ago
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
Flags: needinfo?(docfaraday)
(Assignee)

Comment 30

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

Updated

3 years ago
Blocks: 1121756

Updated

3 years ago
Attachment #8544720 - Flags: review?(adam)
(Assignee)

Comment 34

3 years ago
(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.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 35

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

Updated

3 years ago
Depends on: 1122387
(Assignee)

Updated

3 years ago
Attachment #8544720 - Flags: review?(martin.thomson)
Attachment #8544720 - Flags: review?(adam)
Attachment #8544720 - Flags: review+
(Assignee)

Comment 36

3 years ago
/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

Updated

3 years ago
Attachment #8544720 - Flags: review?(martin.thomson) → review+
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
(Assignee)

Comment 38

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

Updated

3 years ago
Attachment #8544720 - Flags: review?(adam)
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'))
(Assignee)

Comment 40

3 years ago
try run, including patches to intermittents that this patchset trips over:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfca214e0135
(Assignee)

Comment 41

3 years ago
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?
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 42

3 years ago
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!
Flags: needinfo?(nical.bugzilla)
Created attachment 8553769 [details] [diff] [review]
hack to see if the compositor being slow is the cause of fd exhaustion
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
(Assignee)

Comment 47

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

Comment 48

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

Comment 49

3 years ago
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.
Flags: needinfo?(rjesup)
Paul - I'm not sure those messages imply cycle starvation, from when I remember of that code - though starvation may be occurring
Flags: needinfo?(rjesup) → needinfo?(padenot)
(Assignee)

Comment 51

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

Comment 52

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

Updated

3 years ago
See Also: → bug 1126078
(Assignee)

Comment 53

3 years ago
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/mozilla-central/rev/c70466ac38a9
https://hg.mozilla.org/mozilla-central/rev/99fc16e53192
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(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 ?
Flags: needinfo?(rjesup)

Updated

3 years ago
Flags: needinfo?(padenot)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 784517
(Assignee)

Updated

3 years ago
Duplicate of this bug: 907339
Added to release notes for 38 Aurora/DevEd as:

WebRTC now has multistream and renegotiation support
relnote-firefox: --- → 38+
(clearing needinfo)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 854557
(Assignee)

Comment 62

3 years ago
Comment on attachment 8544720 [details]
MozReview Request: bz://1095218/bwc
Attachment #8544720 - Attachment is obsolete: true
Attachment #8618566 - Flags: review+
Attachment #8618567 - Flags: review+
(Assignee)

Comment 63

3 years ago
Created attachment 8618566 [details]
MozReview Request: Bug 1095218 - Part 1: msid support. r=mt
(Assignee)

Comment 64

3 years ago
Created attachment 8618567 [details]
MozReview Request: Bug 1095218 - Part 2: Multistream support.
You need to log in before you can comment on or make changes to this bug.