Closed Bug 1264479 Opened 10 years ago Closed 9 years ago

Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingRemoteDescription

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: sheppy, Assigned: drno)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 4 obsolete files)

Bug 1264479: added current and pending descriptions to PeerConnection.webidl.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1264479: added mochitest to test current and pending descriptions from JS.
59 bytes, text/x-review-board-request
bwc
: review+
Details
Bug 1264479: adapt WPT to interface changes.
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1264479: don't crash on disappearing data channels.
59 bytes, text/x-review-board-request
bwc
: review+
Details
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
Summary: Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingLocalDescription → Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingRemoteDescription
Attachment #8882525 - Attachment is obsolete: true
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 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 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: nobody → drno
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 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).
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?
(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 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+
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
We forgot to update dom/webidl/RTCPeerConnection.webidl. We probably need a mochitest for this too.
Status: RESOLVED → REOPENED
Flags: needinfo?(drno)
Resolution: FIXED → ---
Attachment #8882524 - Attachment is obsolete: true
Attachment #8882734 - Attachment is obsolete: true
Attachment #8882899 - Attachment is obsolete: true
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 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+
Attachment #8903027 - Flags: review?(docfaraday)
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 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-
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.
Flags: needinfo?(drno)
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 on attachment 8903027 [details] Bug 1264479: adapt WPT to interface changes. https://reviewboard.mozilla.org/r/174800/#review181274
Attachment #8903027 - Flags: review?(james) → review+
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.

Attachment

General

Creator:
Created:
Updated:
Size: