Move SDP helper code from the jsep code where possible

RESOLVED FIXED in Firefox 42

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

3 years ago
backlog: --- → tech-debt
Rank: 25
Priority: -- → P2
(Assignee)

Comment 1

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

Updated

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

Comment 2

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

Comment 3

3 years ago
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.
Whiteboard: [keep-open]

Updated

3 years ago
Assignee: nobody → docfaraday

Updated

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

Comment 5

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

Updated

3 years ago
Keywords: leave-open
Whiteboard: [keep-open]
(Assignee)

Comment 6

3 years ago
Check back on try.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 7

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

Updated

3 years ago
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8630111 - Attachment description: MozReview Request: Bug 1142105 - Part 1: Move SDP helper code functions out of JsepSessionImpl and into a separate class. → MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp
Attachment #8630111 - Flags: review+ → review?(martin.thomson)
(Assignee)

Comment 11

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

Comment 12

3 years ago
Ok, mozreview did not do what I expected it to do there, and the interdiff is completely bogus.
(Assignee)

Updated

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

Comment 13

3 years ago
Created 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
Attachment #8638163 - Flags: review?(martin.thomson)
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!
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 on attachment 8638163 [details]
MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt

Good work here.
Attachment #8638163 - Flags: review?(martin.thomson) → review+
(Assignee)

Comment 17

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

Comment 18

3 years ago
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
Attachment #8638163 - Attachment description: MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp → MozReview Request: Bug 1142105 - Part 2: Move some stuff from JsepCodecDescription into /sdp r=mt
Attachment #8638163 - Flags: review+ → review?(martin.thomson)
(Assignee)

Comment 19

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

Carry forward r=mt
Attachment #8638163 - Flags: review?(martin.thomson) → review+
(Assignee)

Updated

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

Comment 20

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

Updated

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

Updated

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

Comment 24

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

Updated

3 years ago
Attachment #8640624 - Attachment description: MozReview Request: Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. → MozReview Request: Bug 1142105 - Part 3: Extract more SDP-related functionality out of JsepSessionImpl, and some readability improvements. r=mt
(Assignee)

Comment 26

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

Comment 27

3 years ago
Check back on try
Flags: needinfo?(docfaraday)
(Assignee)

Comment 28

3 years ago
Was there anything else you'd like to see done as part of this bug?
Flags: needinfo?(docfaraday) → needinfo?(martin.thomson)
Keywords: checkin-needed
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.
Flags: needinfo?(martin.thomson)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b1b3c99e1756
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.