Closed Bug 1133866 Opened 5 years ago Closed 5 years ago

Some light refactoring would be useful in JsepSessionImpl

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(2 files, 4 obsolete files)

There are some functions in JsepSessionImpl that should be broken down into smaller pieces.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8565713 - Attachment is obsolete: true
Attachment #8565764 - Attachment is obsolete: true
Severity: normal → major
Flags: firefox-backlog+
Priority: -- → P3
Attachment #8568278 - Attachment is obsolete: true
Attached file MozReview Request: bz://1133866/bwc (obsolete) —
/r/4311 - Bug 1133866: Some refactoring and simplification in JsepSessionImpl

Pull down this commit:

hg pull review -r 231b1da46ba4332d2de318c5e6210e6f616b2914
Attachment #8569250 - Flags: review?(martin.thomson)
Attachment #8569250 - Flags: review?(martin.thomson) → review+
Comment on attachment 8569250 [details]
MozReview Request: bz://1133866/bwc

https://reviewboard.mozilla.org/r/4309/#review3513

Looks like a strict improvement.

Looking at that header file, I'm thinking that JsepSessionImpl knows and does far too much.  Maybe that's something to look at next.  I don't know what the best thing to cut out would be though: SDP construction, negotiation pieces, and transport handling all seem like candidates.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -    ++numRecv;
> +    if (offerToReceive->isSome() && **offerToReceive) {
> +      *offerToReceive = Some(**offerToReceive - 1);
> +    }

This doesn't even scare you a little? :)

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        if (streamIdEnd == std::string::npos) {
> +  if (streamIdEnd == std::string::npos) {
> -          streamIdEnd = i->attribute.size();
> +    streamIdEnd = msidAttribute.size();
> -        }
> +  }
>  
> -        size_t trackIdStart =
> +  size_t trackIdStart =
> -          i->attribute.find_first_not_of(" \t", streamIdEnd);
> +    msidAttribute.find_first_not_of(" \t", streamIdEnd);
> -        if (trackIdStart == std::string::npos) {
> +  if (trackIdStart == std::string::npos) {
> -          trackIdStart = i->attribute.size();
> +    trackIdStart = msidAttribute.size();
> -        }
> +  }

No point looking for a trackId if the streamId consumes the whole string.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
>    for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
>      if (!i->mAssignedMLine.isSome()) {
>        continue;
>      }
>  
>      // Get rid of all m-line assignments that have not been executed by a call
>      // to SetLocalDescription.
>      if (!i->mSetInLocalDescription) {
>        i->mAssignedMLine.reset();
> +      continue;
>      }
>  
>      if (!offer.GetMediaSection(*i->mAssignedMLine).IsReceiving()) {
>        i->mAssignedMLine.reset();
>      }
>    }

I'd change this differently.

    for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
      if (TrackIsNotNegotiated(offer, *i)) {
        i->mAssignedMLine.reset();
      }
    }

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        NS_ASSERTION(false, "Failed to find remote track for level");
> +      NS_ASSERTION(false, "Failed to find remote track for level");

Are we using NS_ASSERTION and MOZ_ASSERT in the same file?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> +JsepSessionImpl::CopyStickyParams(const SdpMediaSection& source,
> +                                            SdpMediaSection* dest)

nit: align

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -        size_t streamIdStart = i->attribute.find_first_not_of(" \t", 5);
> +  size_t streamIdStart = msidAttribute.find_first_not_of(" \t", 5);

I didn't notice this before, but we should note the divergence here from the spec.  There is no space permitted after the colon.  (Also the general acceptance of tab instead of space, which I would prefer we didn't do).
https://reviewboard.mozilla.org/r/4309/#review3571

The tricky bit when breaking this up is going to be handling the error reporting in a graceful way. I'll have to think about it.

> Are we using NS_ASSERTION and MOZ_ASSERT in the same file?

Yeah, that was something jesup asked for.

> No point looking for a trackId if the streamId consumes the whole string.

I guess I could move the setting of the outparams earlier so we can bail out here.

> This doesn't even scare you a little? :)

Most of the readability problem here is the API of Maybe. It is profoundly unintuitive. I might be able to pass a size_t* (which is null if not set), but that would make the code at the callsite slightly harder to read. Let me see.

> I'd change this differently.
> 
>     for (auto i = mLocalTracks.begin(); i != mLocalTracks.end(); ++i) {
>       if (TrackIsNotNegotiated(offer, *i)) {
>         i->mAssignedMLine.reset();
>       }
>     }

Hmm. That name isn't quite right though. We could call it ShouldUnbindTrackGivenRemoteOffer, but that's getting a little wordy, and still provides little insight into when it might return true.
https://reviewboard.mozilla.org/r/4309/#review3625

> I guess I could move the setting of the outparams earlier so we can bail out here.

I tried this, and it seemed less readable to me, and was more code.
Check back on try
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8150bce392cf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8569250 [details]
MozReview Request: bz://1133866/bwc

Approval Request Comment

See approval request in bug 1144432, as this is a prerequisite.
Attachment #8569250 - Flags: approval-mozilla-aurora?
Attachment #8569250 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8569250 - Attachment is obsolete: true
Attachment #8619499 - Flags: review+
You need to log in before you can comment on or make changes to this bug.