Closed Bug 1170683 Opened 6 years ago Closed 6 years ago

rtcp port numbers are missing and show in renegotiations

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: bwc)

References

Details

(Whiteboard: [sdp])

Attachments

(1 file)

When test case test_peerConnection_answererAddSecondAudioStream.html switches the roles of offer and answerer before re-negotiating two things appear in the re-negotiation offer-answer:

- The re-offer (previously the answerer) only offers rtcp-mux in the new m-line, the is no rtcp port number offered for the new m-line. We probably want to add a new rtcp port just in case.
- Even though the re-offer only offered rtcp-mux for the new m-section, the re-answer replies with rtcp-mux AND a rtcp port (the same port as the initially offered RTCP port in the initial offer) for the new m-section.
Blocks: 1167443
(In reply to Nils Ohlmeier [:drno] from comment #0)
> When test case test_peerConnection_answererAddSecondAudioStream.html
> switches the roles of offer and answerer before re-negotiating two things
> appear in the re-negotiation offer-answer:
> 
> - The re-offer (previously the answerer) only offers rtcp-mux in the new
> m-line, the is no rtcp port number offered for the new m-line. We probably
> want to add a new rtcp port just in case.

   It doesn't make sense to use the previous rtcp attribute if we muxed last time, because that component is gone. I would expect that we would get a new rtcp attribute once gathering finishes (provided that an rtcp-mux answer doesn't arrive before gathering completes; when this happens we suppress rtcp attributes I think).

> - Even though the re-offer only offered rtcp-mux for the new m-section, the
> re-answer replies with rtcp-mux AND a rtcp port (the same port as the
> initially offered RTCP port in the initial offer) for the new m-section.

   Yeah, this is wrong. We should also be pruning out candidates for components that we did not end up using.
Assignee: nobody → docfaraday
(In reply to Byron Campen [:bwc] from comment #1)
> (In reply to Nils Ohlmeier [:drno] from comment #0)
> > - The re-offer (previously the answerer) only offers rtcp-mux in the new
> > m-line, the is no rtcp port number offered for the new m-line. We probably
> > want to add a new rtcp port just in case.
> 
>    It doesn't make sense to use the previous rtcp attribute if we muxed last
> time, because that component is gone. I would expect that we would get a new
> rtcp attribute once gathering finishes (provided that an rtcp-mux answer
> doesn't arrive before gathering completes; when this happens we suppress
> rtcp attributes I think).

I'm not quite sure I understand this correctly. But I think I was not clear in my original message: I'm only talking about SDP's captured after EOC, so when gathering should be over.

So with being said: I agree that if we have a agreed on rtcp-mux on the first m-section, the re-offer should just leave it at that.
What I think is wrong is: the new m-section only has 'a=rtcp-mux' in it and it missing 'a=rtcp:NUMBER' as if we assume the rtcp-mux from the first section has to apply to new second m-section as well.
(In reply to Nils Ohlmeier [:drno] from comment #2)
> (In reply to Byron Campen [:bwc] from comment #1)
> > (In reply to Nils Ohlmeier [:drno] from comment #0)
> > > - The re-offer (previously the answerer) only offers rtcp-mux in the new
> > > m-line, the is no rtcp port number offered for the new m-line. We probably
> > > want to add a new rtcp port just in case.
> > 
> >    It doesn't make sense to use the previous rtcp attribute if we muxed last
> > time, because that component is gone. I would expect that we would get a new
> > rtcp attribute once gathering finishes (provided that an rtcp-mux answer
> > doesn't arrive before gathering completes; when this happens we suppress
> > rtcp attributes I think).
> 
> I'm not quite sure I understand this correctly. But I think I was not clear
> in my original message: I'm only talking about SDP's captured after EOC, so
> when gathering should be over.
> 
> So with being said: I agree that if we have a agreed on rtcp-mux on the
> first m-section, the re-offer should just leave it at that.
> What I think is wrong is: the new m-section only has 'a=rtcp-mux' in it and
> it missing 'a=rtcp:NUMBER' as if we assume the rtcp-mux from the first
> section has to apply to new second m-section as well.

   If we are the offerer, and an rtcp-mux answer comes in before we finish gathering, we won't add an rtcp attribute (because we'll suppress it since the answerer already told us we did not need it). Otherwise, we should be.
Bug 1170683: (WIP) Do a better job in copying previous transport parameters into new offers/answers.
some external folks depend on rtcp to be right
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
Whiteboard: [sdp]
See Also: → 1170677
Comment on attachment 8614431 [details]
MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.

Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.
Attachment #8614431 - Attachment description: MozReview Request: Bug 1170683: (WIP) Do a better job in copying previous transport parameters into new offers/answers. → MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.
Comment on attachment 8614431 [details]
MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.

Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.
Attachment #8614431 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/9955/#review9359

The code itself looks fine.  I'm going to need some more time for the tests.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:338
(Diff revision 3)
> +  bool mWasOfferer;

What does this mean?  Does this mean that this peer was an offerer at some point in the past, or that it was an offerer at the last round?

Comment please.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1421
(Diff revision 3)
> +  mWasOfferer = mIsOfferer;

Would this be better if it were moved to SetState, conditional on the new state being kJsepStateStable?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:543
(Diff revision 3)
> -      if (!MsectionIsDisabled(offer->GetMediaSection(i)) &&
> -          !MsectionIsDisabled(oldAnswer->GetMediaSection(i)) &&
> +      if (!MsectionIsDisabled(local->GetMediaSection(i)) &&
> +          AreOldTransportParamsValid(*oldAnswer, newOffer, i)) {

You could remove the MsectionIsDisabled().

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1945
(Diff revision 3)
> +      nsresult rv = GetComponent(candidate, &component);

Bug number please.  We need to move that candidate parsing some place sensible.  Here ain't sensible.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:3114
(Diff revision 3)
> -  MOZ_ASSERT(mState == kJsepStateStable);
> +  return mWasOfferer ? mCurrentRemoteDescription.get()
> -
> -  return mIsOfferer ? mCurrentRemoteDescription.get()
> -                    : mCurrentLocalDescription.get();
> +                     : mCurrentLocalDescription.get();

I'm just confirming, but won't this make PRANCER harder?
Comment on attachment 8614431 [details]
MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.

https://reviewboard.mozilla.org/r/9957/#review9403

I actually minded this less than I expected, based on the horrific <><><<<>>><<<>>> templates in use.  Moving to typedefs and enums will help readability, but this is otherwise nicely factored and basically readable.

I'm not sure about the copying you are doing; I'd have thought that taking references (i.e., `auto&`) would have looked basically the same.  Maybe I just don't understand the stl stuff well enough (operator[] returns a reference, right?)

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:664
(Diff revision 3)
> +          ASSERT_LE(3U, expectedCandidates[transportLevel][RTP].size());

Can you make the 3 a constant?  It seems like that might save on a little code (though it would mean looping in a few places).

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:686
(Diff revision 3)
> +          // Copy so we can be terse and use []
> +          auto expectedCandidates = mCandidates;

`auto expectedCandidates = mCandidates[transportLevel][RTCP]` ?

Or is that too much?

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:726
(Diff revision 3)
> -    std::cerr << "remote SDP after candidates: "
> +      void CheckDefaultRtcpCandidate(bool expectDefault,

As above: CheckRtcpLine, or just include the assert.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:782
(Diff revision 3)
> -        local ? session.GetLocalDescription() : session.GetRemoteDescription();
> -    SipccSdpParser parser;
> -    UniquePtr<Sdp> parsed = parser.Parse(sdp);
> -    ASSERT_TRUE(!!parsed) << "Parse failed on " << std::endl << sdp << std::endl
> -                          << "Errors were: " << GetParseErrors(parser);
> -    ASSERT_LT(0U, parsed->GetMediaSectionCount());
> -
> -    auto& msection_0 = parsed->GetMediaSection(0);
> +    if (expectEoc) {
> +      ASSERT_TRUE(msection.GetAttributeList().HasAttribute(
> +            SdpAttribute::kEndOfCandidatesAttribute))
> +        << context << " (level " << msection.GetLevel() << ")";
> +    } else {
> +      ASSERT_FALSE(msection.GetAttributeList().HasAttribute(
> +            SdpAttribute::kEndOfCandidatesAttribute))
> +        << context << " (level " << msection.GetLevel() << ")";

ASSERT_EQ()  ...though I will note that EXPECT_EQ() was recommended to me by a Googler, for everything that didn't mean that the test had to stop immediately.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:867
(Diff revision 3)
> +    UniquePtr<Sdp> parsed = parser.Parse(sdp);
> +    EXPECT_TRUE(parsed.get()) << "Should have valid SDP" << std::endl
> +                              << "Errors were: " << GetParseErrors(parser);
> +    return parsed;

I expected this to need a Move() call, but yay for C++.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:704
(Diff revision 3)
> +      void CheckDefaultRtpCandidate(bool expectDefault,

This mode switching seems like it would be better as separate functions:
CheckDefaultCLine() and CheckUpdatedCLine() perhaps.

Hmm, I see how this is being used now; and I guess this is OK.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:2580
(Diff revision 3)
> -  SipccSdpParser parser;
> +  auto outputSdp = Parse(offer);

Did you want to be consistent with how these are declared and initialized?  I see `auto x = Parse()`, and `UniquePtr<Sdp> y(Parse())` both.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:2581
(Diff revision 3)
> -  auto outputSdp = parser.Parse(offer);
> +  ASSERT_TRUE(!!outputSdp);

Parse can assert (rather than expect) and then you don't have to duplicate the check.
Attachment #8614431 - Flags: review?(martin.thomson) → review+
https://reviewboard.mozilla.org/r/9957/#review9413

operator[] isn't const, and I would like the functions to be const

> `auto expectedCandidates = mCandidates[transportLevel][RTCP]` ?
> 
> Or is that too much?

Yeah, I can't use operator[] at all in here.

> Did you want to be consistent with how these are declared and initialized?  I see `auto x = Parse()`, and `UniquePtr<Sdp> y(Parse())` both.

Yeah, I've been cleaning those up as I see them.

> Parse can assert (rather than expect) and then you don't have to duplicate the check.

ASSERT_X doesn't end the test. It returns from the function (which must return void). ASSERT_DEATH can be used to crash the process, but I would prefer to not use that.
https://reviewboard.mozilla.org/r/9955/#review9443

> Would this be better if it were moved to SetState, conditional on the new state being kJsepStateStable?

That'll do the wrong thing with rollback, I think.

> I'm just confirming, but won't this make PRANCER harder?

I don't think so; provisional answers will go in mPending...Description, and if we need to be able to get a provisional answer, we'll have some other accessor.

> What does this mean?  Does this mean that this peer was an offerer at some point in the past, or that it was an offerer at the last round?
> 
> Comment please.

It means last round. I can rename.
Comment on attachment 8614431 [details]
MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.

Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.
Attachment #8614431 - Flags: review+
Comment on attachment 8614431 [details]
MozReview Request: Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.

Bug 1170683: Do a better job in copying previous transport parameters into new offers/answers.
https://hg.mozilla.org/mozilla-central/rev/42bf8560b395
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.