Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingRemoteDescription

RESOLVED FIXED in Firefox 56

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
18
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: sheppy, Assigned: drno)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla56
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
The spec now has these attributes on RTCPeerConnection to allow ICE renegotiation while retaining the current connection. We should implement them.
Important for renegotation apps; shouldn't be that hard to implement
Rank: 18
Priority: -- → P1
(Assignee)

Updated

a year ago
Summary: Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingLocalDescription → Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingRemoteDescription
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8882525 - Attachment is obsolete: true

Comment 10

a year ago
mozreview-review
Comment on attachment 8882524 [details]
Bug 1264479: added [current|pending][Local|Remote]Description to WebIDL.

https://reviewboard.mozilla.org/r/153648/#review159088

This adds attributes to a chrome only interface, so fine.
Attachment #8882524 - Flags: review?(bugs) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8882734 [details]
Bug 1264479: added implementation for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153828/#review159940

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:2495
(Diff revision 2)
>    AddTracks(*mSessionOff);
>    std::string offer = CreateOffer();
>    SetLocalOffer(offer);
>    mOffCandidates->Gather(*mSessionOff, types);
>  
> -  UniquePtr<Sdp> localOffer(Parse(mSessionOff->GetLocalDescription()));
> +  UniquePtr<Sdp> localOffer(Parse(mSessionOff->GetLocalDescription(kJsepDescriptionBoth)));

This should be the current only, right? Same for pretty much all the rest of the places we use this API in here.

::: media/webrtc/signaling/src/jsep/JsepSession.h:45
(Diff revision 2)
>  };
>  
> +enum JsepDescriptionType {
> +  kJsepDescriptionCurrent,
> +  kJsepDescriptionPending,
> +  kJsepDescriptionBoth

kJsepDescriptionPendingOrCurrent I think. The only place we should use this enum value is for the deprecated JS API, so it isn't like we would need it to be short.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:522
(Diff revision 2)
>      GetLocalDescription(&tmp);
>      aSDP.AssignASCII(tmp);
>      delete[] tmp;
>    }
>  
> +  NS_IMETHODIMP GetCurrentLocalDescription(char** aSDP);

We would save a lot of boilerplate if we used an asAString outparam here, and copied the contents of the std::string returned by JsepSession into it.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1512
(Diff revision 2)
>    }
>  
>    CSFLogDebug(logTag, "CreateOffer()");
>  
>    nsresult nrv;
> -  if (restartIce && !mJsepSession->GetLocalDescription().empty()) {
> +  if (restartIce && !mJsepSession->GetLocalDescription(kJsepDescriptionBoth).empty()) {

I think this should be Current only. If we have a pending offer, but no current, there is no ICE to restart.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3569
(Diff revision 2)
> -      std::string localDescription = mJsepSession->GetLocalDescription();
> -      std::string remoteDescription = mJsepSession->GetRemoteDescription();
> +      std::string localDescription = mJsepSession->GetLocalDescription(kJsepDescriptionBoth);
> +      std::string remoteDescription = mJsepSession->GetRemoteDescription(kJsepDescriptionBoth);

Let's add a TODO here for putting both pending and current in the stats.
Attachment #8882734 - Flags: review?(docfaraday) → review-

Comment 12

a year ago
mozreview-review
Comment on attachment 8882899 [details]
Bug 1264479: added unit tests for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153924/#review159954
Attachment #8882899 - Flags: review?(docfaraday) → review+
(Assignee)

Updated

a year ago
Assignee: nobody → drno
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8882734 [details]
Bug 1264479: added implementation for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153828/#review159940

> This should be the current only, right? Same for pretty much all the rest of the places we use this API in here.

The reason I chose Both was that is basically the old behavior before this change. But you are right that it should be Current.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8882734 [details]
Bug 1264479: added implementation for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153828/#review159940

> kJsepDescriptionPendingOrCurrent I think. The only place we should use this enum value is for the deprecated JS API, so it isn't like we would need it to be short.

I was proposing renaming kJsepDescriptionBoth to kJsepDescriptionPendingOrCurrent, to better describe its behavior (pending, and if that is not set, current).
(Assignee)

Comment 18

a year ago
mozreview-review-reply
Comment on attachment 8882734 [details]
Bug 1264479: added implementation for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153828/#review159940

> I was proposing renaming kJsepDescriptionBoth to kJsepDescriptionPendingOrCurrent, to better describe its behavior (pending, and if that is not set, current).

Sorry about that. That makes sense. I'm not too happy about JsepDescriptionType, but have failed to come up with a better name so far. Any suggestions on what to name it?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Nils Ohlmeier [:drno] from comment #18)
> Comment on attachment 8882734 [details]
> Bug 1264479: added implementation for
> [current|pending][Local|Remote]Description.
> 
> https://reviewboard.mozilla.org/r/153828/#review159940
> 
> > I was proposing renaming kJsepDescriptionBoth to kJsepDescriptionPendingOrCurrent, to better describe its behavior (pending, and if that is not set, current).
> 
> Sorry about that. That makes sense. I'm not too happy about
> JsepDescriptionType, but have failed to come up with a better name so far.
> Any suggestions on what to name it?

Yeah, anything I can think of is going to be pretty wordy, and not help readability much (since it will typically be seen with the more self-explanatory enum values).

Comment 22

a year ago
mozreview-review
Comment on attachment 8882734 [details]
Bug 1264479: added implementation for [current|pending][Local|Remote]Description.

https://reviewboard.mozilla.org/r/153828/#review160718
Attachment #8882734 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

a year ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/3f965dd570d3
added [current|pending][Local|Remote]Description to WebIDL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6e1a4b5f7420
added implementation for [current|pending][Local|Remote]Description. r=bwc
https://hg.mozilla.org/integration/autoland/rev/83ba04669582
added unit tests for [current|pending][Local|Remote]Description. r=bwc

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f965dd570d3
https://hg.mozilla.org/mozilla-central/rev/6e1a4b5f7420
https://hg.mozilla.org/mozilla-central/rev/83ba04669582
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 28

11 months ago
We forgot to update dom/webidl/RTCPeerConnection.webidl. We probably need a mochitest for this too.
Status: RESOLVED → REOPENED
Flags: needinfo?(drno)
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8882524 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8882734 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8882899 - Attachment is obsolete: true

Comment 31

11 months ago
mozreview-review
Comment on attachment 8902914 [details]
Bug 1264479: added mochitest to test current and pending descriptions from JS.

https://reviewboard.mozilla.org/r/174628/#review179700
Attachment #8902914 - Flags: review?(docfaraday) → review+

Comment 32

11 months ago
mozreview-review
Comment on attachment 8902913 [details]
Bug 1264479: added current and pending descriptions to PeerConnection.webidl.

https://reviewboard.mozilla.org/r/174626/#review179732
Attachment #8902913 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8903027 - Flags: review?(docfaraday)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

11 months ago
mozreview-review
Comment on attachment 8903027 [details]
Bug 1264479: adapt WPT to interface changes.

https://reviewboard.mozilla.org/r/174800/#review180224

I don't think I'm supposed to review this?
Attachment #8903027 - Flags: review?(docfaraday)

Comment 37

11 months ago
mozreview-review
Comment on attachment 8903330 [details]
Bug 1264479: don't crash on disappearing data channels.

https://reviewboard.mozilla.org/r/175134/#review180226

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1988
(Diff revision 1)
>    for (auto& removedTrack : removedTracks) {
>      const std::string& streamId = removedTrack->GetStreamId();
>      const std::string& trackId = removedTrack->GetTrackId();
>  
> +    if (removedTrack->GetMediaType() == SdpMediaSection::kApplication) {
> +      mMedia->RemoveRemoteTrack(streamId, trackId);

Is this going to do anything?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1990
(Diff revision 1)
>      const std::string& trackId = removedTrack->GetTrackId();
>  
> +    if (removedTrack->GetMediaType() == SdpMediaSection::kApplication) {
> +      mMedia->RemoveRemoteTrack(streamId, trackId);
> +      // TODO do we need to notify content somehow here?
> +      return;

continue, not return
Attachment #8903330 - Flags: review?(docfaraday) → review-
(Assignee)

Comment 38

11 months ago
mozreview-review
Comment on attachment 8903330 [details]
Bug 1264479: don't crash on disappearing data channels.

https://reviewboard.mozilla.org/r/175134/#review180296

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1988
(Diff revision 1)
>    for (auto& removedTrack : removedTracks) {
>      const std::string& streamId = removedTrack->GetStreamId();
>      const std::string& trackId = removedTrack->GetTrackId();
>  
> +    if (removedTrack->GetMediaType() == SdpMediaSection::kApplication) {
> +      mMedia->RemoveRemoteTrack(streamId, trackId);

You are right. It just fails internally, because it does the same GetRemoteStreamById() as below.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1990
(Diff revision 1)
>      const std::string& trackId = removedTrack->GetTrackId();
>  
> +    if (removedTrack->GetMediaType() == SdpMediaSection::kApplication) {
> +      mMedia->RemoveRemoteTrack(streamId, trackId);
> +      // TODO do we need to notify content somehow here?
> +      return;

Good catch, thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Flags: needinfo?(drno)

Comment 42

11 months ago
mozreview-review
Comment on attachment 8903330 [details]
Bug 1264479: don't crash on disappearing data channels.

https://reviewboard.mozilla.org/r/175134/#review180486
Attachment #8903330 - Flags: review?(docfaraday) → review+

Comment 43

11 months ago
mozreview-review
Comment on attachment 8903027 [details]
Bug 1264479: adapt WPT to interface changes.

https://reviewboard.mozilla.org/r/174800/#review181274
Attachment #8903027 - Flags: review?(james) → review+

Comment 44

11 months ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/ed6a08331f08
added current and pending descriptions to PeerConnection.webidl. r=smaug
https://hg.mozilla.org/integration/autoland/rev/6aa9cedf2be0
added mochitest to test current and pending descriptions from JS. r=bwc
https://hg.mozilla.org/integration/autoland/rev/b2c6734e843e
don't crash on disappearing data channels. r=bwc
https://hg.mozilla.org/integration/autoland/rev/08d59706aac1
adapt WPT to interface changes. r=jgraham
You need to log in before you can comment on or make changes to this bug.