Closed Bug 1192390 Opened 4 years ago Closed 4 years ago

Simulcast negotiation support in JsepSessionImpl

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox42 --- affected
Blocking Flags:

People

(Reporter: bwc, Assigned: bwc)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Includes the ability to negotiate imageattr.
Rank: 10
Priority: -- → P1
backlog: --- → webRTC+
Assignee: nobody → docfaraday
Blocks: 1197884
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390: (WIP) Simulcast negotiation.
Attachment #8652545 - Flags: review?(martin.thomson)
There's a lot going on here, since I did some opportunistic refactoring. Pretty much all code for negotiating tracks has been moved from JsepSessionImpl to JsepTrack. Also, sending and receiving tracks each have their own copies of JsepCodecDescription, so we don't have a single set that's trying to do double-duty anymore.
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

https://reviewboard.mozilla.org/r/17175/#review16233

This is too much for me.  I'm pretty happy with the refactoring, though that makes the rest of the changes much harder to see.  What I'm not sure about is what the actual plan is for simulcast.  Looking at the tests, I'm seeing stuff that makes me a little nervous.

I was also surprised to see pure imageattr tests in here, can we separate those?

Can you outline exactly what you intend to do here for me?

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:22
(Diff revision 8)
>  struct JsepCodecDescription {

It might pay to make this a class now that it does so much.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:34
(Diff revision 8)
>          mEnabled(enabled),
>          mStronglyPreferred(false),
> -        mNegotiated(false)
> +        mNegotiated(false),
> +        mSending(false)

These need some documentation.  It is no longer obvious what is going on here.  enabled/sending/negotiated all sound almost the same.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:76
(Diff revision 8)
> -  Negotiate(const SdpMediaSection& remoteMsection)
> +  Negotiate(const std::string& pt, const SdpMediaSection& remoteMsection)

defaultPt

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:339
(Diff revision 8)
> +    // note: If the other end does something wonky like leaving out a recv
> +    // imageattr section for an m-section that they are receiving on, we'll
> +    // interpret that as being willing to accept anything, instead of willing to
> +    // accept nothing. If this is really something that is problematic, we can
> +    // check for this earlier and reject the SDP as malformed.

This shouldn't be a problem.  That is what leaving out a=imageattr means (as well as indicating that you don't support it).

::: media/webrtc/signaling/src/jsep/JsepTrack.h:30
(Diff revision 8)
> +void FreeCodecsBySimulcastVersion(CodecsBySimulcastVersion& codecs);

Unused forward decl

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:94
(Diff revision 8)
> +      // |codec| cannot use its current payload type. Try to find another.
> +      for (uint16_t freePt = 0; freePt <= 128; ++freePt) {

I know that this isn't new, but should we block out a chunk here for RTCP?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:246
(Diff revision 8)
> +    // This assumes that we never answer with asymmetric payload types (ie; the
> +    // remote SDP always has the authoritative rtpmap)

Comments aren't as effective as asserts.  Is there anyway to guarantee that we don't add payload types in an answer?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:306
(Diff revision 8)
> +    for (auto i = extmap.begin(); i != extmap.end(); ++i) {

for (x : y) ?

::: media/webrtc/signaling/src/sdp/SdpMediaSection.cpp:18
(Diff revision 8)
> +    for (auto i = fmtps.mFmtps.begin(); i != fmtps.mFmtps.end(); ++i) {

:

::: media/webrtc/signaling/src/sdp/SdpMediaSection.cpp:96
(Diff revision 8)
> +      if (rtcpfb.pt == "*" || rtcpfb.pt == pt) {

So for imageattr you use Maybe<>, here you have "*".  Any desire to be consistent between the two?

::: media/webrtc/signaling/src/sdp/SdpMediaSection.cpp:150
(Diff revision 8)
> +SdpImageattrAttributeList::Imageattr
> +SdpMediaSection::GetImageattr(uint16_t pt) const

Return by value is unusual for this code, I assume that this is because you don't like the invasive modification that we started out with.

::: media/webrtc/signaling/src/sdp/SdpMediaSection.cpp:154
(Diff revision 8)
> +    SdpImageattrAttributeList imageattrs(GetAttributeList().GetImageattr());
> +    for (const auto& imageattr : imageattrs.mImageattrs) {
> +      if (!imageattr.pt.isSome() || *imageattr.pt == pt) {

Any risk of there being '*' and a pt?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:905
(Diff revision 8)
> +  // TODO: Pass std::vector<std::vector<SdpSimulcastAttributeList::Set>> instead
> +  // of counts and check imageattr too.

Bug number?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:924
(Diff revision 8)
> +      ASSERT_EQ(4U, simulcast.sendVersions[v].choices.size());

What does 4 mean here?  (Hint: we need to use a constant here.)

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:1057
(Diff revision 8)
> +  CheckSimulcast(const JsepSession& offerer,
> +                 const JsepSession& answerer,
> +                 size_t offSendSimulcastCount,
> +                 size_t ansRecvSimulcastCount,
> +                 size_t ansSendSimulcastCount,
> +                 size_t offRecvSimulcastCount)

Are there any cases where we will accept simulcast tracks?  Shouldn't we fix the *recv values to 1?
Attachment #8652545 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/17175/#review16233

For simulcast, the intent is the following:

1) Implement just the SDP negotiation for simulcast and imageattr working, and just enough API surface on JsepSessionImpl to allow simulcast negotiation to be unit-tested (that's this bug). This includes checking the the SDP contains what is expected, and also that the negotiated codecs are what we expect.

2) Plumb the simulcast codecs down to the media conduit code (this requires interfacing with webrtc.org, and openH264; there may be a period where we only support simulcast for webrtc.org).

3) Expose just enough API surface to JS (probably behind a pref, or maybe even avoid the JS API entirely in favor of a pref) so interested parties can try using simulcast.

4) Roll out "proper" JS API surface for simulcast, once it looks like the impl is solid.

I could separate the pure imageattr tests, I suppose, but the imageattr negotiation (what little we have) might be difficult to factor out.

> It might pay to make this a class now that it does so much.

I could see that. I was going to move most of the code to a cpp file in either a subsequent patch, or another bug, since I did not want to both move and change at the same time. Changing it to a class now is easy and at least sends the right signals, even if they're technically the same thing.

> These need some documentation.  It is no longer obvious what is going on here.  enabled/sending/negotiated all sound almost the same.

I was thinking about making mSending be mDirection already, seems like I should go ahead and do that.

> defaultPt

That seems misleading; |pt| is what we use to look up which parameters from |remoteMsection| we are going to use. It so happens that we remember the pt as our new default, but nothing in the spec requires this; this was merely something that helps with interop.

> This shouldn't be a problem.  That is what leaving out a=imageattr means (as well as indicating that you don't support it).

I'm not so much talking about leaving out imageattr entirely, I was talking about something like "a=imageattr:120 send *". Does this really mean "a=imageattr:120 send * recv *", or does it mean that recv is not supported for pt 120? (The spec hints that it is the latter)

> I know that this isn't new, but should we block out a chunk here for RTCP?

I'm not sure I understand. RTCP payload types are all > 128, right?

> Comments aren't as effective as asserts.  Is there anyway to guarantee that we don't add payload types in an answer?

In order to assert this, we'd need to pass the answer too, which means we could just use the pts from there. The code ends up being a little harder to understand if we do this, but I could go either way.

> So for imageattr you use Maybe<>, here you have "*".  Any desire to be consistent between the two?

I can revisit this. I might be able to avoid some of the conversion code if I just store it as a string for imageattr.

> Return by value is unusual for this code, I assume that this is because you don't like the invasive modification that we started out with.

I did this to have a dirt-simple accessor that would make it more terse to modify an existing (or add a new) attribute. The whole if (exists) { copy existing } else { make new } modify new; set new; boilerplate was much harder to read.

> Any risk of there being '*' and a pt?

The spec doesn't say anything about what it means to have both.

> Bug number?

I'll probably end up doing this here.

> What does 4 mean here?  (Hint: we need to use a constant here.)

Good point.

> Are there any cases where we will accept simulcast tracks?  Shouldn't we fix the *recv values to 1?

We have test cases where we negotiate recv simulcast so we can test the send negotiation support on the other side. There is no plan to allow recv simulcast to actually happen, at least not in the near term. This is strictly there to ease testing.
Alias: simulcast
Alias: simulcast
Depends on: 1212908
Depends on: 1212907
Attachment #8652545 - Attachment description: MozReview Request: Bug 1192390: (WIP) Simulcast negotiation. → MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Attachment #8652545 - Flags: review?(martin.thomson)
Comment on attachment 8652545 [details]
MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.

Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Attachment #8652545 - Attachment is obsolete: true
Attachment #8652545 - Flags: review?(martin.thomson)
Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r?mt r?jesup
Attachment #8677132 - Attachment description: MozReview Request: Bug 1192390 - Part 1: (WIP) Lay architectural groundwork for simulcast negotiation. → MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r?mt r?jesup
Attachment #8677132 - Flags: review?(rjesup)
Attachment #8677132 - Flags: review?(martin.thomson)
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

https://reviewboard.mozilla.org/r/22891/#review20937

This is almost too good.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:217
(Diff revision 5)
>  JsepTrack::Negotiate(const SdpMediaSection& answer,
>                       const SdpMediaSection& remote)

I think that you need to block a=simulcast on audio sections.  Where is that?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:245
(Diff revision 5)
> +  std::vector<SdpRidAttributeList::Rid> answerRids;

Factor out please.
Attachment #8677132 - Flags: review?(martin.thomson) → review+
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r?jesup
Attachment #8677132 - Attachment description: MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r?mt r?jesup → MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r?jesup
https://reviewboard.mozilla.org/r/22891/#review20937

> I think that you need to block a=simulcast on audio sections.  Where is that?

I haven't gotten to the actual simulcast negotiation stuff yet, but yes we will not be negotiating it for audio.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r?jesup
Bug 1192390 - Part 2: (WIP) Simulcast and RID negotiation.
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Bug 1192390 - Part 2: (WIP) Simulcast and RID negotiation.
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Bug 1192390 - Part 2: (WIP) Simulcast and RID negotiation.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/7-8/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/3-4/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/4-5/
Attachment #8677132 - Flags: review?(rjesup)
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

https://reviewboard.mozilla.org/r/22891/#review21025

Basically r+ except for the aspect ratio issue.  I'll note that by the end MEGO; it's a long patch.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2068
(Diff revision 5)
> -  vp8->mMaxFs = 12288;
> +  vp8->mConstraints.maxFs = 12288;

I realize it's pre-existing, but can we add a comment about where 12288 comes from here?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1015
(Diff revision 5)
> +      width = mCurSendCodecConfig->mEncodingConstraints.maxWidth;

Both this and height have a problem; if you constrain against maxWidth/Height, you must adjust width and height to maintain aspect ratio

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1020
(Diff revision 5)
> +      width = mCurSendCodecConfig->mEncodingConstraints.maxHeight;

height, not width

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1057
(Diff revision 5)
> -    mb_max = (unsigned) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize);
> +      mb_max = (unsigned) sqrt(8 * (double) max_fs);

I want to go over this whole block of code; I just checked and it's from 2 years ago, by swu and r=derf.  For now it's ok.
Attachment #8677132 - Flags: review?(rjesup)
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/8-9/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/5-6/
https://reviewboard.mozilla.org/r/22891/#review21025

> I realize it's pre-existing, but can we add a comment about where 12288 comes from here?

That just came from the code in sipcc, and I don't think there were any comments there either. I don't know why this number is what it is.

> Both this and height have a problem; if you constrain against maxWidth/Height, you must adjust width and height to maintain aspect ratio

Ok, I took a crack at fixing this, by breaking out the aspect-ratio-preserving code from later on in the function. Let me know if it looks sane to you.
Attachment #8677132 - Flags: review?(rjesup) → review+
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

https://reviewboard.mozilla.org/r/22891/#review21927

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1014
(Diff revisions 8 - 9)
> +    (*height) = max_width * (*height) / (*width) + 1;

We don't need the even-pixel-thing anymore

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1027
(Diff revisions 8 - 9)
> +  (*height) = std::max((*height) & ~1, 2);

we don't need the even-pixel thing anymore
Attachment #8677132 - Attachment description: MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r?jesup → MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/9-10/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/6-7/
Check back on try so part 1 can be landed.
Flags: needinfo?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #32)
> https://reviewboard.mozilla.org/r/22891/#review21025
> 
> > I realize it's pre-existing, but can we add a comment about where 12288 comes from here?
> 
> That just came from the code in sipcc, and I don't think there were any
> comments there either. I don't know why this number is what it is.

I found it (after looking at factors: 128x96 (times 16x16) = 2048x1536).  This is where it came from (not sipcc:)

pref("media.navigator.video.max_fs", 12288); // Enough for 2048x1536

Note this limits us to less than 4K video...  this may actually matter in some odd (very odd) cases for screenshares.  We do want enough bits for a full-screen bit-exact share on desktops, though, so we may want to bump it now.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/10-11/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/7-8/
Attachment #8681984 - Attachment description: MozReview Request: Bug 1192390 - Part 2: (WIP) Simulcast and RID negotiation. → MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r?mt
Attachment #8681984 - Flags: review?(martin.thomson)
Attachment #8681984 - Flags: review?(martin.thomson) → review+
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

https://reviewboard.mozilla.org/r/23945/#review22221

I think that this could use a little more commentary in a few places and I've noted that we don't need pt-based simulcast, but I'll let you decide how to manage that.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:179
(Diff revision 8)
> +    std::string id;

rid?

::: media/webrtc/signaling/src/jsep/JsepTrack.h:226
(Diff revision 8)
> +      const std::string& id,

rid?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:157
(Diff revision 8)
>  void
> +JsepTrack::NegotiateRids(const std::vector<SdpRidAttributeList::Rid>& rids,
> +                         std::vector<JsConstraints>* constraintsList) const

This needs some commentary.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:208
(Diff revision 8)
> -  // TODO(bug 1192390): Get list of rids from |answer|; first rid from each
> -  // simulcast version.
> +  if (!msection.GetAttributeList().HasAttribute(
> +        SdpAttribute::kSimulcastAttribute)) {
> +    return;
> -}
> +  }

Do you need to clear `*rids` here?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:230
(Diff revision 8)
> +  if (versions->type != SdpSimulcastAttribute::Versions::kRid) {
> +    // No support for PT-based simulcast, yet.
> +    return;
> +  }

We decided at the meeting to not have support for PT-based simulcast, so you could probably delete a whole chunk of code.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:237
(Diff revision 8)
> +      const SdpRidAttributeList::Rid* rid =
> +        msection.FindRid(version.choices[0]);
> +      if (rid && (rid->direction == direction)) {

Can we instead validate the rid and simulcast attributes and assume that these checks are OK to skip?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:265
(Diff revision 8)
> -  GetRids(answer, &answerRids);
> +  GetRids(remote, sdp::kRecv, &rids);

Comment: // These are the rids that we will send.

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:264
(Diff revision 8)
> +        ASSERT_EQ(a.GetSsrcs()[i], b.GetSsrcs()[i]);

n.b., we should probably stop signaling of ssrc at some point.
https://reviewboard.mozilla.org/r/23945/#review22221

> rid?

I was waiting to see what terminology W3C wanted to use here.

> We decided at the meeting to not have support for PT-based simulcast, so you could probably delete a whole chunk of code.

I'm still a little gun shy about this. I'll think about it. In any case, that'll happen in another ticket.

> Can we instead validate the rid and simulcast attributes and assume that these checks are OK to skip?

I would not want to do validation that causes SetLocal/SetRemote to fail, since we are supposed to silently ignore the stuff we don't support. Maybe I could write code that silently drops the offending stuff, but that could (further) aggravate the people who think GetLocal/GetRemote ought to return stuff that is bitwise identical, as opposed to SDP that is semantically identical as far as Firefox is concerned.

> n.b., we should probably stop signaling of ssrc at some point.

Perhaps once we can assume universal usage of MID, but that's a long while out.
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/11-12/
Attachment #8681984 - Attachment description: MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r?mt → MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r=mt
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/8-9/
(In reply to Byron Campen [:bwc] from comment #41)
> > rid?
> 
> I was waiting to see what terminology W3C wanted to use here.

It's rid.
 
> > We decided at the meeting to not have support for PT-based simulcast, so you could probably delete a whole chunk of code.
> 
> I'm still a little gun shy about this. I'll think about it. In any case,
> that'll happen in another ticket.

At the moment, you are only carrying a little extra code, that's fine with me.  It's not like it's obviously buggy.  I usually don't like to land stuff that I know we won't need, but ultimately decide on what will easier in the long term for you.
 
> > Can we instead validate the rid and simulcast attributes and assume that these checks are OK to skip?
> 
> I would not want to do validation that causes SetLocal/SetRemote to fail,
> since we are supposed to silently ignore the stuff we don't support. Maybe I
> could write code that silently drops the offending stuff, but that could
> (further) aggravate the people who think GetLocal/GetRemote ought to return
> stuff that is bitwise identical, as opposed to SDP that is semantically
> identical as far as Firefox is concerned.

I know that creates uncertainty, but if we are expected to act on something, then insisting that it be well-formed is perfectly reasonable.  That means that the rids in an a=simulcast line should correspond to an a=rid line.  More specifically, I think that we are obligated to reject bad values when we understand the semantics.  It's different for extensions that we don't understand (see discussion on max-goats).
(In reply to Martin Thomson [:mt:] from comment #44)
> (In reply to Byron Campen [:bwc] from comment #41)
> > > rid?
> > 
> > I was waiting to see what terminology W3C wanted to use here.
> 
> It's rid.

   Ok, will change.

> > > Can we instead validate the rid and simulcast attributes and assume that these checks are OK to skip?
> > 
> > I would not want to do validation that causes SetLocal/SetRemote to fail,
> > since we are supposed to silently ignore the stuff we don't support. Maybe I
> > could write code that silently drops the offending stuff, but that could
> > (further) aggravate the people who think GetLocal/GetRemote ought to return
> > stuff that is bitwise identical, as opposed to SDP that is semantically
> > identical as far as Firefox is concerned.
> 
> I know that creates uncertainty, but if we are expected to act on something,
> then insisting that it be well-formed is perfectly reasonable.  That means
> that the rids in an a=simulcast line should correspond to an a=rid line. 
> More specifically, I think that we are obligated to reject bad values when
> we understand the semantics.  It's different for extensions that we don't
> understand (see discussion on max-goats).

   On a second look, I think this is something it makes sense to validate; I saw the stuff about direction and thought this was just checking that the rid was something we'd be interested in, but this is actually about consistency between simulcast and rid.
Attachment #8681984 - Attachment description: MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r=mt → MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r?mt
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/9-10/
Bug 1192390 - Part 3: (WIP) Plumb negotiated simulcast down to webrtc.org, without RID.
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

I've added the simulcast/rid validation code, but it is enough that I'd like you to look at it.
Attachment #8681984 - Flags: review+ → review?(martin.thomson)
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

https://reviewboard.mozilla.org/r/23945/#review22495

Yeah, that looks right.

::: media/webrtc/signaling/src/sdp/SipccSdpMediaSection.cpp:277
(Diff revision 10)
> +      } else if (versions.type == SdpSimulcastAttribute::Versions::kPt) {

You don't need to put a whole lot of extra effort into the pt part.

::: media/webrtc/signaling/test/sdp_unittests.cpp:2622
(Diff revision 10)
> +           "a=simulcast: send rid=9" CRLF,

n.b., I am reminded that we are likely to change the syntax such that the space after the colon isn't needed.  I'd rather not land with the space if at all possible.
Attachment #8681984 - Flags: review?(martin.thomson) → review+
https://reviewboard.mozilla.org/r/24957/#review22499

I know that this is WIP, but I noticed a few things.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1573
(Diff revision 1)
> +      stream.numberOfTemporalLayers = 1; // TODO: Ask jesup about this

This is right.  When we get to temporal scalability, this will be signaled in some other fashion.  No one really knows how to signal scalable video codecs in SDP when it gets down to it.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1586
(Diff revision 1)
> +        stream.maxBitrate = encoding.constraints.maxBr;

Don't we separately pay attention to the b= line?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1587
(Diff revision 1)
> +        stream.minBitrate = std::min(stream.minBitrate, stream.maxBitrate);

Where does the minimum bitrate come from?  I don't remember seeing any minimums in the spec.

In this case, I think that you have the right logic, but it would be better not to send the stream than to violate constraints.
(In reply to Martin Thomson [:mt:] from comment #50)
> https://reviewboard.mozilla.org/r/24957/#review22499
> 
> I know that this is WIP, but I noticed a few things.
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1586
> (Diff revision 1)
> > +        stream.maxBitrate = encoding.constraints.maxBr;
> 
> Don't we separately pay attention to the b= line?
> 

   We have no support for b=.

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1587
> (Diff revision 1)
> > +        stream.minBitrate = std::min(stream.minBitrate, stream.maxBitrate);
> 
> Where does the minimum bitrate come from?  I don't remember seeing any
> minimums in the spec.

   I do not know why this is in the webrtc.org code, but I'm just making sure that if it is set for some reason, it doesn't end up being higher than the max. I really don't think it will ever be exposed in the W3C API.
https://reviewboard.mozilla.org/r/23945/#review22495

> n.b., I am reminded that we are likely to change the syntax such that the space after the colon isn't needed.  I'd rather not land with the space if at all possible.

This is a separate issue, I think. I've opened bug 1225877 for this.
Let's check in parts 1 and 2. I'll be continuing work on part 3 in another bug, since there are some dependencies that need to be resolved.
Flags: needinfo?(docfaraday)
Comment on attachment 8677132 [details]
MozReview Request: Bug 1192390 - Part 1: Lay architectural groundwork for simulcast negotiation. r=mt r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22891/diff/12-13/
Comment on attachment 8681984 [details]
MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23945/diff/10-11/
Attachment #8681984 - Attachment description: MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r?mt → MozReview Request: Bug 1192390 - Part 2: Simulcast and RID negotiation. r+mt
Comment on attachment 8686211 [details]
MozReview Request: Bug 1230197: (WIP) Plumb negotiated simulcast down to webrtc.org, without RID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24957/diff/1-2/
Attachment #8686211 - Attachment description: MozReview Request: Bug 1192390 - Part 3: (WIP) Plumb negotiated simulcast down to webrtc.org, without RID. → MozReview Request: Bug 1230197: (WIP) Plumb negotiated simulcast down to webrtc.org, without RID.
Check back on try for landing parts 1,2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acce17c8291&selectedJob=14400421
Flags: needinfo?(docfaraday)
Just check in parts 1 and 2; 3 will be moving to bug 1230197.
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Comment on attachment 8686211 [details]
MozReview Request: Bug 1230197: (WIP) Plumb negotiated simulcast down to webrtc.org, without RID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24957/diff/2-3/
(In reply to Byron Campen [:bwc] from comment #58)
> Just check in parts 1 and 2; 3 will be moving to bug 1230197.

adding leave-open
Keywords: leave-open
Byron -- I'm doing bug clean up.  Can we close this one?
Flags: needinfo?(docfaraday)
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(docfaraday)
Resolution: --- → FIXED
Duplicate of this bug: 1173602
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.