Closed
Bug 1264479
Opened 8 years ago
Closed 7 years ago
Implement RTCPeerConnection attributes: currentLocalDescription pendingLocalDescription currentRemoteDescription pendingRemoteDescription
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: sheppy, Assigned: drno)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 4 obsolete files)
The spec now has these attributes on RTCPeerConnection to allow ICE renegotiation while retaining the current connection. We should implement them.
Comment 1•8 years ago
|
||
Important for renegotation apps; shouldn't be that hard to implement
Rank: 18
Priority: -- → P1
Updated•8 years ago
|
Blocks: webrtc_spec
Assignee | ||
Updated•7 years 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•7 years ago
|
Attachment #8882525 -
Attachment is obsolete: true
Comment 10•7 years 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•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 13•7 years 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•7 years 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•7 years 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) |
Comment 21•7 years ago
|
||
(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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Reporter | ||
Comment 27•7 years ago
|
||
Documentation was added some time ago; now updated to reflect shipping in Firefox 56: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/currentLocalDescription https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/pendingLocalDescription https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/currentRemoteDescription https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/pendingRemoteDescription Also added to https://developer.mozilla.org/en-US/Firefox/Releases/56.
Keywords: dev-doc-needed → dev-doc-complete
Comment 28•7 years 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•7 years ago
|
Attachment #8882524 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8882734 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8882899 -
Attachment is obsolete: true
Comment 31•7 years 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•7 years 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•7 years ago
|
Attachment #8903027 -
Flags: review?(docfaraday)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 36•7 years 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•7 years 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•7 years 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•7 years ago
|
Flags: needinfo?(drno)
Comment 42•7 years 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•7 years 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•7 years 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
Comment 45•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ed6a08331f08 https://hg.mozilla.org/mozilla-central/rev/6aa9cedf2be0 https://hg.mozilla.org/mozilla-central/rev/b2c6734e843e https://hg.mozilla.org/mozilla-central/rev/08d59706aac1
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•