Closed Bug 1203246 Opened 4 years ago Closed 4 years ago

Track negotiation logic should be factored out of JsepSessionImpl

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(1 file)

No description provided.
Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.
Comment on attachment 8659582 [details]
MozReview Request: Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.

Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.
Attachment #8659582 - Flags: review?(martin.thomson)
Comment on attachment 8659582 [details]
MozReview Request: Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.

Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.
Comment on attachment 8659582 [details]
MozReview Request: Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.

https://reviewboard.mozilla.org/r/18973/#review16923

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:109
(Diff revision 2)
> -  bool mNegotiated;
> +  // true for send codec, false for recv codec
> +  bool mSending;

I'm starting to think that bool is a poor choice for this sort of thing.  Would a new enum be especially problematic here?

enum class CodecDirection { kSend, kRecv };

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:221
(Diff revision 2)
> +    for (auto& rtcpfb : rtcpfbs.mFeedbacks) {

const auto&

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1606
(Diff revision 2)
> +      for (const std::string& fmt : msection.GetFormats()) {
> +        uint16_t payloadType;
> +        if (!SdpHelper::GetPtAsInt(fmt, &payloadType) ||
> +            payloadType > UINT8_MAX) {
> +          JSEP_SET_ERROR("audio/video payload type is not an 8 bit unsigned: "
> +                         << fmt);
> +          return NS_ERROR_INVALID_ARG;
> +        }
> +      }
> +    }

I'm wondering if there isn't another refactoring we could add here, an iterator that iterates over integer pt values and asserts if it fails to get the right PT.  Maybe keep that in mind for later.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:36
(Diff revision 2)
> +  }
> +  Maybe<std::string>

Some blank lines might help.

::: media/webrtc/signaling/src/jsep/JsepTrack.h:38
(Diff revision 2)
> +  GetBandwidth(const std::string& type) const

A comment explaining that we only support b=AS (or whatever it is that we actually support) would be good.  Then you should probably check that type is what we expect.

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

Can you add a note about the range we should avoid for RTCP?  And maybe a bug #.  Or do we have something to safeguard that already?

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:198
(Diff revision 2)
> +  // TODO: Remove this once we're ready to put multiple codecs in an answer

Bug #?
Attachment #8659582 - Flags: review?(martin.thomson) → review+
https://reviewboard.mozilla.org/r/18973/#review16923

> I'm starting to think that bool is a poor choice for this sort of thing.  Would a new enum be especially problematic here?
> 
> enum class CodecDirection { kSend, kRecv };

I avoided that because there's already this proliferation of exactly this enum. However, I think that tells me we need to have one such enum that gets reused. This should shave some code off.

> A comment explaining that we only support b=AS (or whatever it is that we actually support) would be good.  Then you should probably check that type is what we expect.

I only moved this code from JsepTrackImpl.h. We don't support this at all right now, I think it was just there so it was part of the interface. I'm just going to remove this. Also, GetProtocol doesn't seem to be used by anything, so I'll remove that also.

> Can you add a note about the range we should avoid for RTCP?  And maybe a bug #.  Or do we have something to safeguard that already?

I'll just restrict this to the dynamic range (96-127). That should be sufficient for now.
Comment on attachment 8659582 [details]
MozReview Request: Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.

Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.
Check back on try
Flags: needinfo?(docfaraday)
Hmm, jetpack is permaorange on linux debug on that try push. I'm going to rebase and try again.
Comment on attachment 8659582 [details]
MozReview Request: Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.

Bug 1203246: Factor track negotiation stuff out of JsepSessionImpl, and other simplification.
Ok, it looks like jetpack is permaorange on m-c now. Same with those bc3 oranges.
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e189be469ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.