1142105 - Move SDP helper code from the jsep code where possible

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

There is a bunch of SDP helper code scattered around the jsep code. It would be good to consolidate it, and move it to the sdp directory.

Comment 1

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Bug 1142105 - Part 1: Move SDP helper code functions out of JsepSessionImpl and into a separate class.

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8630111 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Bug 1142105 - Part 1: Move SDP helper code functions out of JsepSessionImpl and into a separate class.

Comment 3

[Byron Campen [:bwc]]

I'd like to review and land piecemeal here, to keep bitrot under control. First patch is just some bulk moves. More complicated refactoring to extract more functionality will come in a later patch.

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8630111 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

https://reviewboard.mozilla.org/r/12679/#review11299

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:638
(Diff revision 1)
> -  rv = SetupTransportAttributes(*sdp, sdp.get());
> +  if (GetAnswer()) {
> +    // We have an old answer from a previous negotiation
> +    rv = SetupTransportParams(*GetAnswer(), *sdp, sdp.get());
> -  NS_ENSURE_SUCCESS(rv,rv);
> +    NS_ENSURE_SUCCESS(rv,rv);
> +  }

This looks like more than a refactor.

Comment 5

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/12679/#review11299

> This looks like more than a refactor.

Yeah, this was a tiny bit more than just moving code. I renamed SetupTransportAttributes to SetupTransportParams (for consistency), moved the GetAnswer() check out of it (since that was all that was necessary to make it movable), and moved the resulting function to SdpHelper. Let me know if that matches what you see.

Comment 6

[Byron Campen [:bwc]]

Check back on try.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8630111 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Bug 1142105 - Part 1: Move SDP helper code functions out of JsepSessionImpl and into a separate class.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8630111 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

https://reviewboard.mozilla.org/r/12679/#review11847

I seem to be victim of reviewboard repeat review request recursion.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25f4b1ce54b5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25f4b1ce54b5

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8630111 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Comment 12

[Byron Campen [:bwc]]

Ok, mozreview did not do what I expected it to do there, and the interdiff is completely bogus.

Comment 13

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp

Comment 14

[Hector Zhao [:hectorz]]

Hi :bwc, if the system wide nspr is installed at a custom path, |#include "nspr/prprf.h"| at line 13 of /media/webrtc/signaling/src/sdp/SdpHelper.cpp will break building with system nspr, switching to |#include "prprf.h"| should fix it. Thanks!

Comment 15

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/14035/#review12615

LGTM.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:58
(Diff revision 1)
> -    const SdpRtpmapAttributeList& rtpmap = attrs.GetRtpmap();
> +    const SdpRtpmapAttributeList::Rtpmap* entry(remoteMsection.FindRtpmap(fmt));

Do we have guidance on
    Foo x(y);
vs
    Foo x = y;
?

Assignment of a pointer doesn't seem to need this treatment.

::: media/webrtc/signaling/src/sdp/SdpMediaSection.h:152
(Diff revision 1)
> +  FindParameters(const std::string& pt) const

FindFmtpParameters or FindFmtp

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Good work here.

Comment 17

[Byron Campen [:bwc]]

(In reply to Hector Zhao [:hectorz] from comment #14)
> Hi :bwc, if the system wide nspr is installed at a custom path, |#include
> "nspr/prprf.h"| at line 13 of /media/webrtc/signaling/src/sdp/SdpHelper.cpp
> will break building with system nspr, switching to |#include "prprf.h"|
> should fix it. Thanks!

I'll put this into part 2, thanks for the heads-up!

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Carry forward r=mt

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Comment 21

[Martin Thomson [:mt:]]

Comment on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

https://reviewboard.mozilla.org/r/14037/#review12945

Ship It!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7d6315fd6b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf7d6315fd6b

Comment 24

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. r=mt

Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8640624 [details]
MozReview Request: Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. r=mt

https://reviewboard.mozilla.org/r/14347/#review13021

Ship It!

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:780
(Diff revision 1)
> -  UniquePtr<SdpExtmapAttributeList> ourExtmap(new SdpExtmapAttributeList);
> +  mSdpHelper.AddCommonExtmaps(remoteMsection, *ourExtensions, msection);

Early return seems less obvious now.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:609
(Diff revision 1)
> +  // Undo track assignments from a previous call to CreateOffer

The change in comment loses the fact that this is only for non-negotiated msections.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:285
(Diff revision 1)
> +  if (!bundledMids.empty()) {

I would remove this outer check, since you are calling bundledMids.count(mid) below.  Even though this might avoid some tiny amount of work, we don't need this to be super-performant.

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 8640624 [details]
MozReview Request: Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. r=mt

Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. r=mt

Comment 27

[Byron Campen [:bwc]]

Check back on try

Comment 28

[Byron Campen [:bwc]]

Was there anything else you'd like to see done as part of this bug?

Comment 29

[Martin Thomson [:mt:]]

Nah, this looks like a good start.  JsepSessionImpl is still too large, but the next cut is a harder one, and one that can use another bug.  That cut might be to remove ICE handling, but that's probably quite a lot more work.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b3c99e1756

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1b3c99e1756