Closed Bug 1016476 Opened 6 years ago Closed 5 years ago

SDP handling should support Bundle - configure media conduits

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: shell, Assigned: bwc)

References

Details

(Whiteboard: p=3)

Attachments

(3 files, 20 obsolete files)

100.03 KB, patch
bwc
: review+
Details | Diff | Splinter Review
77.22 KB, patch
drno
: review+
Details | Diff | Splinter Review
3.11 KB, patch
drno
: review+
Details | Diff | Splinter Review
This is part of the break-up of work to support SDP bundling
Depends on: sdparta
Attached patch (WIP) Bundle support. (obsolete) — Splinter Review
Work in progress on bundle support. Passes signaling_unittests except for one that tests unmuxed rtcp (which is not going to happen with bundle)
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attached patch (WIP) Bundle support. (obsolete) — Splinter Review
Attach to the correct TransportFlow.
Attachment #8527094 - Attachment is obsolete: true
Attached patch (WIP) Bundle support (obsolete) — Splinter Review
Fix build on OS X. talky.io works with bundle, but mochitests are sad for some reason.
Attachment #8527115 - Attachment is obsolete: true
Attached patch (WIP) Bundle support (obsolete) — Splinter Review
Attach datachannel to the right transportflow when bundled. Mochitests are green now.
Attachment #8527133 - Attachment is obsolete: true
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
Implement unique payload type correlation.
Attachment #8527353 - Attachment is obsolete: true
Attached patch Part 2: Test work (obsolete) — Splinter Review
Run tests both with bundle and without, and write some tests that target the RTP correlation logic.
Comment on attachment 8528719 [details] [diff] [review]
Part 1: (WIP) Bundle support

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

This look about right? (Note that I have ripped out the old double-transport stuff in MediaPipeline, since it is a bad architecture for handling bundle renegotiation. I have some ideas about how to do it better, which we can try out when it comes time for renegotiation, since it won't matter until then)
Attachment #8528719 - Flags: feedback?(adam)
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
Use a=ssrc:cname since it is required by RFC 5576.
Attachment #8529336 - Flags: feedback?(adam)
Attachment #8528719 - Attachment is obsolete: true
Attachment #8528719 - Flags: feedback?(adam)
Attached patch Part 1: (WIP) Bundle support. (obsolete) — Splinter Review
Unrot
Attachment #8529336 - Attachment is obsolete: true
Attachment #8529336 - Flags: feedback?(adam)
Attachment #8534673 - Flags: feedback?(adam)
Comment on attachment 8528720 [details] [diff] [review]
Part 2: Test work

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

Sorry this has a problem which needs to be fixed before landing.

::: dom/media/tests/mochitest/templates.js
@@ +179,5 @@
>        });
>      }
>    ],
>    [
> +    'STEEPLECHASE_SIGNAL_OFFER',

Sorry but this does not work for Steeplechase. Steeplechase filters this sequence of steps into two parts. And the only filter criteria right now is the name of each of the steps. So each step has to start with "PC_LOCAL...|PC_REMOTE...".
Attachment #8528720 - Flags: review-
Attached patch add_no_bundle_test.patch (obsolete) — Splinter Review
Just one more test for no bundle.
Comment on attachment 8528720 [details] [diff] [review]
Part 2: Test work

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

::: dom/media/tests/mochitest/test_dataChannel_basicAudioVideoNoBundle.html
@@ +16,1 @@
>      title: "Basic data channel audio/video connection"

Bug numbers and titles should be updated to this bug.
(In reply to Nils Ohlmeier [:drno] from comment #11)
> Comment on attachment 8528720 [details] [diff] [review]
> Part 2: Test work
> 
> Review of attachment 8528720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this has a problem which needs to be fixed before landing.
> 
> ::: dom/media/tests/mochitest/templates.js
> @@ +179,5 @@
> >        });
> >      }
> >    ],
> >    [
> > +    'STEEPLECHASE_SIGNAL_OFFER',
> 
> Sorry but this does not work for Steeplechase. Steeplechase filters this
> sequence of steps into two parts. And the only filter criteria right now is
> the name of each of the steps. So each step has to start with
> "PC_LOCAL...|PC_REMOTE...".

Oh bummer. I can fix though.
(In reply to Byron Campen [:bwc] from comment #14)
> Oh bummer. I can fix though.

What is the intention behind moving this out into a separate step?
(In reply to Nils Ohlmeier [:drno] from comment #15)
> (In reply to Byron Campen [:bwc] from comment #14)
> > Oh bummer. I can fix though.
> 
> What is the intention behind moving this out into a separate step?

I can swap that back in once I fix; I had a good reason.
Attached patch Part 2: Test work. (obsolete) — Splinter Review
Incorporate feedback from drno. The reason I broke the steeplechase signaling into a separate step was to make it possible to tweak the offer before it was sent to the other end (in this case, to prevent bundle from being negotiated, see test_dataChannel_basicAudioVideoNoBundle.html)
Attachment #8528720 - Attachment is obsolete: true
Attached patch Part 2: Test work. (obsolete) — Splinter Review
Update mochitest info
Attachment #8537351 - Attachment is obsolete: true
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
Remove a dependency on the msid work.
Attachment #8534673 - Attachment is obsolete: true
Attachment #8534673 - Flags: feedback?(adam)
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
This hunk did not actually depend on the msid work, so putting it back.
Attachment #8537517 - Attachment is obsolete: true
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
Remove an unused variable
Attachment #8537537 - Attachment is obsolete: true
Comment on attachment 8537540 [details] [diff] [review]
Part 1: (WIP) Bundle support

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

Whoever can get to it.

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=67de08ef9bda

try (non-unified):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09e8a3a5b97d
Attachment #8537540 - Flags: review?(martin.thomson)
Attachment #8537540 - Flags: review?(adam)
Attachment #8537355 - Flags: review?(drno)
Comment on attachment 8536737 [details] [diff] [review]
add_no_bundle_test.patch

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

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoNoBundle.html
@@ +12,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "796892",

Put bug number here?

@@ +22,5 @@
> +    test = new PeerConnectionTest(options);
> +    test.chain.insertAfter('PC_LOCAL_CREATE_OFFER',
> +    [['PC_LOCAL_REMOVE_BUNDLE_FROM_OFFER',
> +      function (test) {
> +        test.originalOffer.sdp = test.originalOffer.sdp.replace("a=group:BUNDLE 0 1\r\n","");

I ended up replacing "a=group:BUNDLE" with "a=foo:" so it would just show up as an unknown attribute, since I wasn't sure whether we'd change the way we would choose mids in the future.
Attachment #8536737 - Flags: feedback+
Comment on attachment 8537540 [details] [diff] [review]
Part 1: (WIP) Bundle support

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

LGTM r=mt with comments addressed.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ +309,5 @@
> +      mids.push_back(attrs.GetMid());
> +    }
> +  }
> +
> +  SetupBundle(mids, sdp.get());

Maybe move the above loop into SetupBundle.  And can't you pass the SDP pointer by reference?

@@ +379,5 @@
> +    // When using ssrc attributes, we are required to at least have a cname.
> +    // (See https://tools.ietf.org/html/rfc5576#section-6.1)
> +    // Just use the track id until we have some need for something special
> +    std::string cnameAttr("cname:");
> +    cnameAttr += track.GetTrackId();

This isn't right.  The CNAME should be the same for all tracks (unless the track has a different clock source, that is, and I don't think we're there yet).  The only trick there is that CNAME is supposed to be random: https://tools.ietf.org/html/rfc7022

My suggestion is to generate a fixed value for now and open a bug to get this right later.  I've noted the other place to mention the bug number.

@@ +610,5 @@
>    AddExtmap(msection);
>  
> +  std::ostringstream osMid;
> +  osMid << msection->GetLevel();
> +  AddMid(osMid.str(), msection);

I was going to say "I approve", but in renegotiation, if the peer was the original offerer, and we are offering to add m= sections, and they choose to number similarly, but use GetLevel() + 1 instead, this is going to fail every time.  I don't know if we should care about fixing that, but it's probably worth noting as a potential failure mode.

You could choose to use the mid selection method that Adam picked for the partial offer/answer stuff, but that increases the size of the RTP header extension as a result...

@@ +704,5 @@
> +    localAttrs.SetAttribute(new SdpStringAttribute(SdpAttribute::kMidAttribute,
> +                                                   remoteAttrs.GetMid()));
> +  }
> +  // Is there any reason to assign our own if the offerer did not pick one?
> +  // If so, we need to make sure that what we pick is unique.

https://tools.ietf.org/html/rfc5888#section-9.4.1

@@ +970,5 @@
> +
> +  std::set<std::string> bundleMids;
> +  const SdpMediaSection* bundleMsection = nullptr;
> +
> +  auto* group = FindBundleGroup(answer);

I assume that we're only going to support a single bundle group.  I guess that we won't offer them, and won't accept them if offered?

@@ +979,5 @@
> +      JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> +          " does not exist in the SDP. (mid="
> +          << group->tags[0] << ")");
> +      return NS_ERROR_INVALID_ARG;
> +    }

It might pay to check that the m= section at the start of the group doesn't contain a port of zero  too.

@@ +1004,3 @@
>      // TODO(bug 1017888): Need to handle renegotiation somehow.
> +    if (answerMsection.GetPort() == 0) {
> +      // Transports start out in closed, so we don't need to do anything here.

This comment doesn't make a lot of sense.  If the line is negotiated off, then we are fine.  The only trick here is to disable an already active transport during renegotiation.  That is, if it isn't bundled and still in use by another.

@@ +1203,5 @@
>  
>    *transport = new JsepTransport("transport-id", components);
> +  // Start transports out as closed, negotiation will mark them active if
> +  // needed.
> +  (*transport)->mState = JsepTransport::kJsepTransportClosed;

Any reason not to move this to the JsepTransport constructor?

@@ +1835,5 @@
> +// given RTP packet belongs to by looking at the payload type field. This only
> +// works, however, if that payload type appeared in only one m-section.
> +// We figure that out here.
> +nsresult
> +JsepSessionImpl::SetUniquePayloadTypes()

I hope that this is well-tested.  The cyclomatic complexity is higher than my poor brain can tolerate.

@@ +1844,5 @@
> +
> +  for (size_t i = 0; i < mRemoteTracks.size(); ++i) {
> +    auto track = mRemoteTracks[i].mTrack;
> +
> +    if (track->GetMediaType() == SdpMediaSection::kApplication) {

Maybe != kAudio and != kVideo instead to be completely rigorous about it.

@@ +1850,5 @@
> +    }
> +
> +    // Kinda ugly, but oh well
> +    auto* details = static_cast<JsepTrackNegotiatedDetailsImpl*>(
> +        track->GetNegotiatedDetails());

You should take this as a sign.  I think that the reason you are doing this is because the logic that you have here isn't properly encapsulated.  There are a few phases to this logic: determine what the negotiated codecs are for all the tracks and reduce that to a set.  Then, look at all the payload types.  If the payload type only appears once, then it is unique (and I guess it can be used as a selector).

Maybe exposing a new method on JsepTrackNegotiatedDetails (not the Impl):
 - void setUniquePayloadTypes(const set<uint8_t>&)

That means probably a third pass, but I think that it makes this code a little clearer.

::: media/webrtc/signaling/src/jsep/JsepTrackImpl.h
@@ +63,5 @@
>      return nullptr;
>    }
>  
> +  virtual std::vector<uint8_t>
> +  GetUniquePayloadTypes() const {

Maybe move this up.

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
@@ +516,5 @@
> +      }
> +    }
> +
> +    // For now, just use track id as cname
> +    conduit->SetLocalCNAME(aTrack.GetTrackId().c_str());

OK, this is where the CNAME is set.  See earlier comment.  Just raise a bug and comment here.

I think that what you want to do is generate the CNAME when building SDP, move the CNAME onto the JsepTrack from the SDP when creating the track, and then finally use the value here.

@@ +641,5 @@
> +      }
> +    }
> +
> +    // For now, just use track id as cname
> +    conduit->SetLocalCNAME(aTrack.GetTrackId().c_str());

As above.

::: media/webrtc/signaling/src/sdp/SdpAttribute.h
@@ +435,5 @@
> +          j = i->tags.erase(j);
> +        } else {
> +          ++j;
> +        }
> +      }

Not std::find ?

::: media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c
@@ +5349,5 @@
> +
> +
> +sdp_result_e sdp_build_attr_ssrc(sdp_t *sdp_p,
> +                                 sdp_attr_t *attr_p,
> +                                 flex_string *fs)

I'm wondering if we can't find a way to drop all of the build_attr functions.  We don't use them.  Later, perhaps.
Attachment #8537540 - Flags: review?(martin.thomson) → review+
Blocks: 1112692
> This isn't right.  The CNAME should be the same for all tracks (unless the track has a different clock 
> source, that is, and I don't think we're there yet).  The only trick there is that CNAME is supposed 
> to be random: https://tools.ietf.org/html/rfc7022

When we support multiple audio streams or multiple video streams, they should be assumed to have different clocks (unless they are cloned from the same source like simulcast).

> My suggestion is to generate a fixed value for now and open a bug to get this right later.  I've noted 
> the other place to mention the bug number.

Seems reasonable.
(In reply to Randell Jesup [:jesup] from comment #25)
> When we support multiple audio streams or multiple video streams, they
> should be assumed to have different clocks (unless they are cloned from the
> same source like simulcast).

I'd rather make the opposite assumption.  Most of our tracks use the same (i.e., system) clock source.  It's only in the case where RTP is generated by a hardware device that we need to worry about having different clocks.  If we use different sources, then we will be causing streams to be unsynchronizable at the receiver end.
Attached patch add_no_bundle_test.patch (obsolete) — Splinter Review
Added requestFlakyTimeout() call and updated bug number.
Attachment #8536737 - Attachment is obsolete: true
Attachment #8538003 - Flags: review?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #23)
> @@ +22,5 @@
> > +    test = new PeerConnectionTest(options);
> > +    test.chain.insertAfter('PC_LOCAL_CREATE_OFFER',
> > +    [['PC_LOCAL_REMOVE_BUNDLE_FROM_OFFER',
> > +      function (test) {
> > +        test.originalOffer.sdp = test.originalOffer.sdp.replace("a=group:BUNDLE 0 1\r\n","");
> 
> I ended up replacing "a=group:BUNDLE" with "a=foo:" so it would just show up
> as an unknown attribute, since I wasn't sure whether we'd change the way we
> would choose mids in the future.

Do you think removing it could cause problems? If not, I would suggest to leave the two tests with different approaches. That gives us better coverage of different scenarios :-)
(In reply to Martin Thomson [:mt] from comment #26)
> (In reply to Randell Jesup [:jesup] from comment #25)
> > When we support multiple audio streams or multiple video streams, they
> > should be assumed to have different clocks (unless they are cloned from the
> > same source like simulcast).
> 
> I'd rather make the opposite assumption.  Most of our tracks use the same
> (i.e., system) clock source.  It's only in the case where RTP is generated
> by a hardware device that we need to worry about having different clocks. 
> If we use different sources, then we will be causing streams to be
> unsynchronizable at the receiver end.

All audio devices by default use the sampling device's timebase, not system timebase.  At most system timebase would be used for streams that looped through webaudio.  Video devices typically would use system time, though I'm not sure that's well-defined.
(In reply to Martin Thomson [:mt] from comment #24)
> Comment on attachment 8537540 [details] [diff] [review]
> Part 1: (WIP) Bundle support
> 
> Review of attachment 8537540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM r=mt with comments addressed.
> 
> ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
> @@ +309,5 @@
> > +      mids.push_back(attrs.GetMid());
> > +    }
> > +  }
> > +
> > +  SetupBundle(mids, sdp.get());
> 
> Maybe move the above loop into SetupBundle.  And can't you pass the SDP
> pointer by reference?
> 

   Convention is for outparams to be pointers.

> @@ +379,5 @@
> > +    // When using ssrc attributes, we are required to at least have a cname.
> > +    // (See https://tools.ietf.org/html/rfc5576#section-6.1)
> > +    // Just use the track id until we have some need for something special
> > +    std::string cnameAttr("cname:");
> > +    cnameAttr += track.GetTrackId();
> 
> This isn't right.  The CNAME should be the same for all tracks (unless the
> track has a different clock source, that is, and I don't think we're there
> yet).  The only trick there is that CNAME is supposed to be random:
> https://tools.ietf.org/html/rfc7022
> 
> My suggestion is to generate a fixed value for now and open a bug to get
> this right later.  I've noted the other place to mention the bug number.
>

   Ok.

> @@ +610,5 @@
> >    AddExtmap(msection);
> >  
> > +  std::ostringstream osMid;
> > +  osMid << msection->GetLevel();
> > +  AddMid(osMid.str(), msection);
> 
> I was going to say "I approve", but in renegotiation, if the peer was the
> original offerer, and we are offering to add m= sections, and they choose to
> number similarly, but use GetLevel() + 1 instead, this is going to fail
> every time.  I don't know if we should care about fixing that, but it's
> probably worth noting as a potential failure mode.
> 
> You could choose to use the mid selection method that Adam picked for the
> partial offer/answer stuff, but that increases the size of the RTP header
> extension as a result...
>

   Seems like "sdparta_<level>" could avoid this.

> @@ +704,5 @@
> > +    localAttrs.SetAttribute(new SdpStringAttribute(SdpAttribute::kMidAttribute,
> > +                                                   remoteAttrs.GetMid()));
> > +  }
> > +  // Is there any reason to assign our own if the offerer did not pick one?
> > +  // If so, we need to make sure that what we pick is unique.
> 
> https://tools.ietf.org/html/rfc5888#section-9.4.1
> 

   Ok, removing this comment.

> @@ +970,5 @@
> > +
> > +  std::set<std::string> bundleMids;
> > +  const SdpMediaSection* bundleMsection = nullptr;
> > +
> > +  auto* group = FindBundleGroup(answer);
> 
> I assume that we're only going to support a single bundle group.  I guess
> that we won't offer them, and won't accept them if offered?
> 

   We will use the first bundle group, and ignore the rest. I can file a bug for supporting multiple BUNDLE groups and add a comment.

> @@ +979,5 @@
> > +      JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> > +          " does not exist in the SDP. (mid="
> > +          << group->tags[0] << ")");
> > +      return NS_ERROR_INVALID_ARG;
> > +    }
> 
> It might pay to check that the m= section at the start of the group doesn't
> contain a port of zero  too.
> 

   Sure.

> @@ +1004,3 @@
> >      // TODO(bug 1017888): Need to handle renegotiation somehow.
> > +    if (answerMsection.GetPort() == 0) {
> > +      // Transports start out in closed, so we don't need to do anything here.
> 
> This comment doesn't make a lot of sense.  If the line is negotiated off,
> then we are fine.  The only trick here is to disable an already active
> transport during renegotiation.  That is, if it isn't bundled and still in
> use by another.
> 

   Yes, we will need to handle this for renegotiation. This code changes shape significantly with the renegotiation work, so I'll handle it here.

> @@ +1203,5 @@
> >  
> >    *transport = new JsepTransport("transport-id", components);
> > +  // Start transports out as closed, negotiation will mark them active if
> > +  // needed.
> > +  (*transport)->mState = JsepTransport::kJsepTransportClosed;
> 
> Any reason not to move this to the JsepTransport constructor?

   Actually, I think I will have them start out in offered whenever we set the local offer, and transition offered->closed when negotiation is done. That way, if for some reason something inspects the transport state pre-answer (nothing does that now), it will get a better result.

> 
> @@ +1835,5 @@
> > +// given RTP packet belongs to by looking at the payload type field. This only
> > +// works, however, if that payload type appeared in only one m-section.
> > +// We figure that out here.
> > +nsresult
> > +JsepSessionImpl::SetUniquePayloadTypes()
> 
> I hope that this is well-tested.  The cyclomatic complexity is higher than
> my poor brain can tolerate.
> 

   There are some tests in signaling_unittests in Part 2, but they're not super thorough. I can check if it can be simplified further, but I am doubtful.

> @@ +1844,5 @@
> > +
> > +  for (size_t i = 0; i < mRemoteTracks.size(); ++i) {
> > +    auto track = mRemoteTracks[i].mTrack;
> > +
> > +    if (track->GetMediaType() == SdpMediaSection::kApplication) {
> 
> Maybe != kAudio and != kVideo instead to be completely rigorous about it.
> 

   I would expect the negotiated details to be null in this case, which will cause this to be skipped.
   
> @@ +1850,5 @@
> > +    }
> > +
> > +    // Kinda ugly, but oh well
> > +    auto* details = static_cast<JsepTrackNegotiatedDetailsImpl*>(
> > +        track->GetNegotiatedDetails());
> 
> You should take this as a sign.  I think that the reason you are doing this
> is because the logic that you have here isn't properly encapsulated.  There
> are a few phases to this logic: determine what the negotiated codecs are for
> all the tracks and reduce that to a set.  Then, look at all the payload
> types.  If the payload type only appears once, then it is unique (and I
> guess it can be used as a selector).
> 
> Maybe exposing a new method on JsepTrackNegotiatedDetails (not the Impl):
>  - void setUniquePayloadTypes(const set<uint8_t>&)
> 
> That means probably a third pass, but I think that it makes this code a
> little clearer.
> 

   That would be easy enough.

> ::: media/webrtc/signaling/src/jsep/JsepTrackImpl.h
> @@ +63,5 @@
> >      return nullptr;
> >    }
> >  
> > +  virtual std::vector<uint8_t>
> > +  GetUniquePayloadTypes() const {
> 
> Maybe move this up.
> 

   Sure.

> ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp
> @@ +516,5 @@
> > +      }
> > +    }
> > +
> > +    // For now, just use track id as cname
> > +    conduit->SetLocalCNAME(aTrack.GetTrackId().c_str());
> 
> OK, this is where the CNAME is set.  See earlier comment.  Just raise a bug
> and comment here.
> 
> I think that what you want to do is generate the CNAME when building SDP,
> move the CNAME onto the JsepTrack from the SDP when creating the track, and
> then finally use the value here.
> 
> @@ +641,5 @@
> > +      }
> > +    }
> > +
> > +    // For now, just use track id as cname
> > +    conduit->SetLocalCNAME(aTrack.GetTrackId().c_str());
> 
> As above.
> 
> ::: media/webrtc/signaling/src/sdp/SdpAttribute.h
> @@ +435,5 @@
> > +          j = i->tags.erase(j);
> > +        } else {
> > +          ++j;
> > +        }
> > +      }
> 
> Not std::find ?
> 

   That would be cleaner. I'm not sure why I thought that std::find required the data to be sorted...

> ::: media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c
> @@ +5349,5 @@
> > +
> > +
> > +sdp_result_e sdp_build_attr_ssrc(sdp_t *sdp_p,
> > +                                 sdp_attr_t *attr_p,
> > +                                 flex_string *fs)
> 
> I'm wondering if we can't find a way to drop all of the build_attr
> functions.  We don't use them.  Later, perhaps.

   Me too. File a bug! Give me an excuse to delete more sipcc code.
(In reply to Byron Campen [:bwc] from comment #30)
>    Convention is for outparams to be pointers.

Maybe you understand the term `outparam` differently to me.  I don't regard this as an outparam in the "return value we couldn't return properly because that was already in use".  This is merely something that SetupBundle is allowed to play with.  Hence reference.

> > You could choose to use the mid selection method that Adam picked for the
> > partial offer/answer stuff, but that increases the size of the RTP header
> > extension as a result...
> >
> 
>    Seems like "sdparta_<level>" could avoid this.

At a cost in bytes.  The prefix is probably a good idea though.

> > @@ +1844,5 @@
> > > +
> > > +  for (size_t i = 0; i < mRemoteTracks.size(); ++i) {
> > > +    auto track = mRemoteTracks[i].mTrack;
> > > +
> > > +    if (track->GetMediaType() == SdpMediaSection::kApplication) {
> > 
> > Maybe != kAudio and != kVideo instead to be completely rigorous about it.
> > 
> 
>    I would expect the negotiated details to be null in this case, which will
> cause this to be skipped.

Is that an invariant?

> > I'm wondering if we can't find a way to drop all of the build_attr
> > functions.  We don't use them.  Later, perhaps.
> 
>    Me too. File a bug! Give me an excuse to delete more sipcc code.

Bug 1112737
(In reply to Martin Thomson [:mt] from comment #31)
> (In reply to Byron Campen [:bwc] from comment #30)
> >    Convention is for outparams to be pointers.
> 
> Maybe you understand the term `outparam` differently to me.  I don't regard
> this as an outparam in the "return value we couldn't return properly because
> that was already in use".  This is merely something that SetupBundle is
> allowed to play with.  Hence reference.
>

   Fair enough.
 
> > > @@ +1844,5 @@
> > > > +
> > > > +  for (size_t i = 0; i < mRemoteTracks.size(); ++i) {
> > > > +    auto track = mRemoteTracks[i].mTrack;
> > > > +
> > > > +    if (track->GetMediaType() == SdpMediaSection::kApplication) {
> > > 
> > > Maybe != kAudio and != kVideo instead to be completely rigorous about it.
> > > 
> > 
> >    I would expect the negotiated details to be null in this case, which will
> > cause this to be skipped.
> 
> Is that an invariant?
>

   We won't negotiate anything other than audio, video, or application here. If/when we support something like text, we could update this, but lots of other places will need updates too.
(In reply to Randell Jesup [:jesup] from comment #29)
> (In reply to Martin Thomson [:mt] from comment #26)
> > (In reply to Randell Jesup [:jesup] from comment #25)
> > > When we support multiple audio streams or multiple video streams, they
> > > should be assumed to have different clocks (unless they are cloned from the
> > > same source like simulcast).
> > 
> > I'd rather make the opposite assumption.  Most of our tracks use the same
> > (i.e., system) clock source.  It's only in the case where RTP is generated
> > by a hardware device that we need to worry about having different clocks. 
> > If we use different sources, then we will be causing streams to be
> > unsynchronizable at the receiver end.
> 
> All audio devices by default use the sampling device's timebase, not system
> timebase.  At most system timebase would be used for streams that looped
> through webaudio.  Video devices typically would use system time, though I'm
> not sure that's well-defined.

   So, given what I'm reading here, there is no reason to expect that any given pair of tracks will share a clock source without some explicit indication? If this is the case, it would seem that track id is a sensible default, but we should allow other things to be indicated when JsepTrack is created? Also, I should point out that our present behavior is to not use CNAME at all, which I'm guessing is more or less equivalent to using CNAME with a different value for every track from the other endpoint's perspective?
(In reply to Byron Campen [:bwc] from comment #33)
> > All audio devices by default use the sampling device's timebase, not system
> > timebase.  At most system timebase would be used for streams that looped
> > through webaudio.  Video devices typically would use system time, though I'm
> > not sure that's well-defined.
> 
>    So, given what I'm reading here, there is no reason to expect that any
> given pair of tracks will share a clock source without some explicit
> indication? If this is the case, it would seem that track id is a sensible
> default, but we should allow other things to be indicated when JsepTrack is
> created? Also, I should point out that our present behavior is to not use
> CNAME at all, which I'm guessing is more or less equivalent to using CNAME
> with a different value for every track from the other endpoint's perspective?

Yes, well, I'm concerned that we're not producing RTP that can be synchronized, period.  Let's not solve that right here, right now.  The UUID is sufficient (are they version 4 by default?) to meet the RFC 7022 requirements, so the only concern is that we can't synchronize.
(In reply to Martin Thomson [:mt] from comment #34)
> (In reply to Byron Campen [:bwc] from comment #33)
> > > All audio devices by default use the sampling device's timebase, not system
> > > timebase.  At most system timebase would be used for streams that looped
> > > through webaudio.  Video devices typically would use system time, though I'm
> > > not sure that's well-defined.
> > 
> >    So, given what I'm reading here, there is no reason to expect that any
> > given pair of tracks will share a clock source without some explicit
> > indication? If this is the case, it would seem that track id is a sensible
> > default, but we should allow other things to be indicated when JsepTrack is
> > created? Also, I should point out that our present behavior is to not use
> > CNAME at all, which I'm guessing is more or less equivalent to using CNAME
> > with a different value for every track from the other endpoint's perspective?
> 
> Yes, well, I'm concerned that we're not producing RTP that can be
> synchronized, period.  Let's not solve that right here, right now.  The UUID
> is sufficient (are they version 4 by default?) to meet the RFC 7022
> requirements, so the only concern is that we can't synchronize.

I guess there's a third possibility here; that the webrtc.org code is somehow choosing an appropriate CNAME under the hood. However, given that we use a separate video/voice engine for every m-line, presumably it would not be choosing CNAME any better than we do in this patch? jesup?
Flags: needinfo?(rjesup)
(In reply to Martin Thomson [:mt] from comment #31)
> (In reply to Byron Campen [:bwc] from comment #30)
> >    Convention is for outparams to be pointers.
> 
> Maybe you understand the term `outparam` differently to me.  I don't regard
> this as an outparam in the "return value we couldn't return properly because
> that was already in use".  This is merely something that SetupBundle is
> allowed to play with.  Hence reference.

In this code, refs must always be passed as const. If it's something you
frob it's a ptr.


> > > You could choose to use the mid selection method that Adam picked for the
> > > partial offer/answer stuff, but that increases the size of the RTP header
> > > extension as a result...
> > >
> > 
> >    Seems like "sdparta_<level>" could avoid this.
> 
> At a cost in bytes.  The prefix is probably a good idea though.
> 
> > > @@ +1844,5 @@
> > > > +
> > > > +  for (size_t i = 0; i < mRemoteTracks.size(); ++i) {
> > > > +    auto track = mRemoteTracks[i].mTrack;
> > > > +
> > > > +    if (track->GetMediaType() == SdpMediaSection::kApplication) {
> > > 
> > > Maybe != kAudio and != kVideo instead to be completely rigorous about it.
> > > 
> > 
> >    I would expect the negotiated details to be null in this case, which will
> > cause this to be skipped.
> 
> Is that an invariant?
> 
> > > I'm wondering if we can't find a way to drop all of the build_attr
> > > functions.  We don't use them.  Later, perhaps.
> > 
> >    Me too. File a bug! Give me an excuse to delete more sipcc code.
> 
> Bug 1112737
Comment on attachment 8538003 [details] [diff] [review]
add_no_bundle_test.patch

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

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoNoBundle.html
@@ +24,5 @@
> +    test = new PeerConnectionTest(options);
> +    test.chain.insertAfter('PC_LOCAL_CREATE_OFFER',
> +    [['PC_LOCAL_REMOVE_BUNDLE_FROM_OFFER',
> +      function (test) {
> +        test.originalOffer.sdp = test.originalOffer.sdp.replace("a=group:BUNDLE 0 1\r\n","");

So, I'm actually about to break this; mids are going to have a prefix of "sdparta_" on them as a cheap way of avoiding duplicates with some other implementation. This was exactly the kind of thing that concerned me here.
Wouldn't it be better to have them be pseudorandom? I mean, someone else might decide to use "sdparta".
(In reply to Martin Thomson [:mt] from comment #34)
> (In reply to Byron Campen [:bwc] from comment #33)
> > > All audio devices by default use the sampling device's timebase, not system
> > > timebase.  At most system timebase would be used for streams that looped
> > > through webaudio.  Video devices typically would use system time, though I'm
> > > not sure that's well-defined.
> > 
> >    So, given what I'm reading here, there is no reason to expect that any
> > given pair of tracks will share a clock source without some explicit
> > indication? If this is the case, it would seem that track id is a sensible
> > default, but we should allow other things to be indicated when JsepTrack is
> > created? Also, I should point out that our present behavior is to not use
> > CNAME at all, which I'm guessing is more or less equivalent to using CNAME
> > with a different value for every track from the other endpoint's perspective?
> 
> Yes, well, I'm concerned that we're not producing RTP that can be
> synchronized, period.  Let's not solve that right here, right now.  The UUID
> is sufficient (are they version 4 by default?) to meet the RFC 7022
> requirements, so the only concern is that we can't synchronize.

So: all RTP data can be synchronized.  That's what the RTCP NTP data is for.  It's assumed that all (or almost all) sources will have (or could have) different timebases.  RTCP with the NTP<->timestamp correlations allows you to correct for clock drifts relative to each other, and to synchronize streams with different clocks.  CNAME is about what *should* be synchronized to each other, not what has the same clock.
Flags: needinfo?(rjesup)
Ok, this spec says that the same CNAME MUST be used everywhere in the same PeerConnection, and gives some rationale in section 11. I'm just going to do this for now, and if the spec changes, I can change the code.
(In reply to Eric Rescorla (:ekr) from comment #38)
> Wouldn't it be better to have them be pseudorandom? I mean, someone else
> might decide to use "sdparta".

The only reason I have not done that is it might be useful to be able to glance at an mid in some logging (or a group:BUNDLE line) and know what it corresponds to.
Well, you could do "sdparta_<random>"
Attached patch Part 1: (WIP) Bundle support (obsolete) — Splinter Review
Incorporate feedback.
Attached patch Part 1: Bundle support (obsolete) — Splinter Review
Update commit log.
Attachment #8538253 - Attachment is obsolete: true
Attachment #8537540 - Attachment is obsolete: true
Attachment #8537540 - Flags: review?(adam)
Comment on attachment 8538254 [details] [diff] [review]
Part 1: Bundle support

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

This interdiff look alright?

https://bugzilla.mozilla.org/attachment.cgi?oldid=8537540&action=interdiff&newid=8538254&headers=1
Attachment #8538254 - Flags: review?(martin.thomson)
(In reply to Eric Rescorla (:ekr) from comment #43)
> Well, you could do "sdparta_<random>"

My wording was imprecise; I liked the ability to map mid to level just by glancing at it.
Comment on attachment 8537355 [details] [diff] [review]
Part 2: Test work.

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

LGTM.

Would be great if we could make the two new tests even more strict, but only if reasonably possible.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +4777,5 @@
> +  WaitForCompleted();
> +
> +  // Wait for some data to get written
> +  ASSERT_TRUE_WAIT(a1_->GetPacketsSent(0) >= 10 &&
> +                   a2_->GetPacketsReceived(0) >= 10, kDefaultTimeout * 2);

Can we do more verifications of the result here (and in the test above)?

To me it looks like you ensuring here that we create an are able to create an Answer and that we don't reject the create Answer. But if we could it would be great if could do some verification on the RTP layer that codecs got selected properly.
Attachment #8537355 - Flags: review?(drno) → review+
(In reply to Nils Ohlmeier [:drno] from comment #48)
> Comment on attachment 8537355 [details] [diff] [review]
> Part 2: Test work.
> 
> Review of attachment 8537355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> Would be great if we could make the two new tests even more strict, but only
> if reasonably possible.
> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +4777,5 @@
> > +  WaitForCompleted();
> > +
> > +  // Wait for some data to get written
> > +  ASSERT_TRUE_WAIT(a1_->GetPacketsSent(0) >= 10 &&
> > +                   a2_->GetPacketsReceived(0) >= 10, kDefaultTimeout * 2);
> 
> Can we do more verifications of the result here (and in the test above)?
> 
> To me it looks like you ensuring here that we create an are able to create
> an Answer and that we don't reject the create Answer. But if we could it
> would be great if could do some verification on the RTP layer that codecs
> got selected properly.

   Well, we also verify that data gets through, and I've verified that this fails if the unique payload-types matching is not used, although that requires commenting stuff out. We could verify the unique payload types logic more thoroughly, and since you're the second one to ask for it, I'll do so in jsep_session_unittest.
(In reply to Martin Thomson [:mt] from comment #40)
> https://tools.ietf.org/html/draft-ietf-rtcweb-rtp-usage-21#section-4.9
> suggests otherwise.

Aha.  By "clock" there they don't mean timestamp clock; they mean NTP clock.  I.e. RTCP will cause synchronization of all media with the same CNAME, correcting for any clock drifts between the timestamp clocks used in the different streams.  The timestamp clocks are still independent, per my comments above.  And yes, we should use a single CNAME per section 11.
Comment on attachment 8538003 [details] [diff] [review]
add_no_bundle_test.patch

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

r+ provided the BUNDLE replacement is updated.
Attachment #8538003 - Flags: review?(docfaraday) → review+
Attached patch Part 1: Bundle support (obsolete) — Splinter Review
Add missing include.
Attachment #8538254 - Attachment is obsolete: true
Attachment #8538254 - Flags: review?(martin.thomson)
Comment on attachment 8538565 [details] [diff] [review]
Part 1: Bundle support

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

Updated interdiff:

https://bugzilla.mozilla.org/attachment.cgi?oldid=8537540&action=interdiff&newid=8538565&headers=1
Attachment #8538565 - Flags: review?(martin.thomson)
Comment on attachment 8538565 [details] [diff] [review]
Part 1: Bundle support

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

Interdiff was helpful, thanks.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ +1149,5 @@
> +
> +  // Mark transports that weren't accepted as closed
> +  for (auto i = mTransports.begin(); i != mTransports.end(); ++i) {
> +    if ((*i)->mState != JsepTransport::kJsepTransportAccepted) {
> +      (*i)->mState = JsepTransport::kJsepTransportClosed;

I think that this is good.  Question that this raises: do we have a test for un-bungling media sections?  In particular, if we offer audio and video bundled (but not bundle-only), and the offer accepts without bundle, are we cool?

@@ +1961,5 @@
> +  if (msection.GetAttributeList().HasAttribute(SdpAttribute::kSsrcAttribute)) {
> +    auto& ssrcs = msection.GetAttributeList().GetSsrc().mSsrcs;
> +    for (auto i = ssrcs.begin(); i != ssrcs.end(); ++i) {
> +      if (i->attribute.find("cname:") == 0) {
> +        return i->attribute.substr(6);

I'm a little uncomfortable with the idea that you have a base 6 here.
Attachment #8538565 - Flags: review?(martin.thomson) → review+
(In reply to Martin Thomson [:mt] from comment #54)
> Comment on attachment 8538565 [details] [diff] [review]
> Part 1: Bundle support
> 
> Review of attachment 8538565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Interdiff was helpful, thanks.
> 
> ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
> @@ +1149,5 @@
> > +
> > +  // Mark transports that weren't accepted as closed
> > +  for (auto i = mTransports.begin(); i != mTransports.end(); ++i) {
> > +    if ((*i)->mState != JsepTransport::kJsepTransportAccepted) {
> > +      (*i)->mState = JsepTransport::kJsepTransportClosed;
> 
> I think that this is good.  Question that this raises: do we have a test for
> un-bungling media sections?  In particular, if we offer audio and video
> bundled (but not bundle-only), and the offer accepts without bundle, are we
> cool?
> 

   Yeah, Part 2 modifies signaling_unittests to run every test with bundle, with bundle offered but not accepted, and with no bundle offered at all (with a few exceptions, since a very small number of tests expect rtcp-mux to be disabled, which makes no sense with bundle).
Attached patch Part 2: Test work (ignore this) (obsolete) — Splinter Review
Add a unique payload-types test to jsep_session_unittest.
Attachment #8537355 - Attachment is obsolete: true
Attachment #8538753 - Attachment description: Part 2: Test work → Part 2: Test work (ignore this)
Attachment #8538753 - Attachment is obsolete: true
Attachment #8537355 - Attachment is obsolete: false
Unrot
Attachment #8538565 - Attachment is obsolete: true
Comment on attachment 8538762 [details] [diff] [review]
Part 1: Bundle support

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

Carry forward r=mt
Attachment #8538762 - Flags: review+
Attached patch Part 2: Test work (obsolete) — Splinter Review
Unrot, and add a test for unique payload types.
Attachment #8537355 - Attachment is obsolete: true
Re-add the mochitest that was lost during the unrot.
Attachment #8538820 - Attachment is obsolete: true
Updated to remove any bundle tags.

Carrying forward r+=bwc
Attachment #8538003 - Attachment is obsolete: true
Attachment #8538869 - Flags: review+
Comment on attachment 8538857 [details] [diff] [review]
Part 2: Test work

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

The new test in jsep_session_unittest look good to you?
Attachment #8538857 - Flags: review?(drno)
Comment on attachment 8538857 [details] [diff] [review]
Part 2: Test work

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

LGTM
Attachment #8538857 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/85f2520e212d
https://hg.mozilla.org/mozilla-central/rev/bb0ca3ea654f
https://hg.mozilla.org/mozilla-central/rev/f021629b5356
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1115823
You need to log in before you can comment on or make changes to this bug.