1203246 - Track negotiation logic should be factored out of JsepSessionImpl

Status: RESOLVED FIXED

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

Comment 1

[Byron Campen [:bwc]]

Attachment: 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 2

[Byron Campen [:bwc]]

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 3

[Byron Campen [:bwc]]

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 4

[Martin Thomson [:mt:]]

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 #?

Comment 5

[Byron Campen [:bwc]]

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 6

[Byron Campen [:bwc]]

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 7

[Byron Campen [:bwc]]

Check back on try

Comment 8

[Byron Campen [:bwc]]

Hmm, jetpack is permaorange on linux debug on that try push. I'm going to rebase and try again.

Comment 9

[Byron Campen [:bwc]]

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 10

[Byron Campen [:bwc]]

Ok, it looks like jetpack is permaorange on m-c now. Same with those bc3 oranges.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e189be469ed

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e189be469ed