Closed Bug 952145 Opened 9 years ago Closed 7 years ago

JSEP support for state rollbacks

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jesup, Assigned: bwc)

References

Details

(Keywords: feature)

Attachments

(1 file, 1 obsolete file)

Implement support for state rollbacks in JSEP

Adam has a proposal for this in the standards bodies (link?)
Let me see how hard this will be (doesn't seem hard).
Assignee: nobody → docfaraday
Attached file MozReview Request: bz://952145/bwc (obsolete) —
/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull review -r 050b0f75cbf33450c744b2b3edfc4a99733b6641
Need to write some tests.
The tricky bit here is probably going to be the rollback of remote offers, since that requires doing stuff like firing onremovetrack, possibly getting ourselves tangled in bug 1122306.
See Also: → 1122306
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull review -r cfa1bd558d4c5bfe7007d287f12f9571e9a5d792
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r af00f2e011cd8ef10a17268a8c3aed45c3d66695 https://reviewboard-hg.mozilla.org/gecko/
Rank: 25
Priority: -- → P2
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r c60af694689ef1203d6f7c0bc84bf290ee861ed5 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r 494afec0d8e9f969f45fe0604296e3ebfac07059 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: (WIP) Rollback support

Pull down this commit:

hg pull -r 7c32b7f8d758ddcf02c15c99c7d500ab6d386839 https://reviewboard-hg.mozilla.org/gecko/
Duplicate of this bug: 1081257
Blocks: 1081252
backlog: --- → webRTC+
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 534a75246af61de7546812f6c8d770cf9b77e083 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8580400 - Flags: review?(martin.thomson)
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7687

I'm not happy with the test coverage here, but it looks generally OK.

I'd like to see tests of rollback when there is active communication (i.e., rollback a renegotiation offer).

When you rollback a remote offer, shouldn't there be onremovetrack events being fired?  A test to verify that would be good.

::: dom/media/tests/mochitest/test_peerConnection_remoteRollback.html:23
(Diff revision 7)
> +              new mozRTCSessionDescription({ type: "rollback", sdp: ""}),

What happens if you omit sdp from the dictionary?
Attachment #8580400 - Flags: review?(martin.thomson)
https://reviewboard.mozilla.org/r/5773/#review7689

So, as it stands we aren't firing OnAddTrack/OnAddStream until negotiation completes (known bug, will require some work to fix), so rollback isn't going to have any effects there. Once that is fixed, though...

Rolling back a renegotiation sounds like a good use of time.

> What happens if you omit sdp from the dictionary?

Not sure, is that commonly done? The w3c spec doesn't spell this out, but the JSEP draft says that you're supposed to pass an SDP with an "empty contents".
https://reviewboard.mozilla.org/r/5773/#review7691

> Not sure, is that commonly done? The w3c spec doesn't spell this out, but the JSEP draft says that you're supposed to pass an SDP with an "empty contents".

I only ask, because having to include { sdp: "" } seems like a waste of typing.  It would be nice if we allowed it to be absent, since we don't actually use the value.  See also the last email I sent to the ML.
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 9d873bb4637a6407d0e56ec9581150db38335653 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8580400 - Flags: review?(martin.thomson)
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7763

I don't know how easy it would be to add a check to see that media isn't interrupted or affected by the renegotiation.  Do the current renegotiation tests check that media is flowing after all is done?

::: dom/media/tests/mochitest/test_peerConnection_localReofferRollback.html:27
(Diff revision 8)
> +            test.pcRemote.endOfTrickleIce.then(() => {
> +              send_message({"type": "end_of_trickle_ice"});
> +            });

This doesn't enter the chain, please add a comment explaining why you need to have this message setup here (and not in PC_REMOTE_WAIT_FOR_END_OF_TRICKLE_WITH_MORE_UNDERSCORES_FOR_LUCK

::: dom/media/PeerConnection.js:700
(Diff revisions 7 - 8)
> +          if (!desc.sdp) {
> +            desc.sdp = "";
> +          }

Maybe we should add a default in the WebIDL instead.
Attachment #8580400 - Flags: review?(martin.thomson)
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

https://reviewboard.mozilla.org/r/5773/#review7765

Ship It!
Attachment #8580400 - Flags: review+
https://reviewboard.mozilla.org/r/5773/#review7769

::: dom/media/tests/mochitest/test_peerConnection_localReofferRollback.html:27
(Diff revision 8)
> +            test.pcRemote.endOfTrickleIce.then(() => {
> +              send_message({"type": "end_of_trickle_ice"});
> +            });

This just comes verbatim from a step in templates.js.
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support

Pull down this commit:

hg pull -r 085a14216551a67b4beb82fe272d90d72c702916 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8580400 - Flags: review+
https://reviewboard.mozilla.org/r/5773/#review7767

They check the media flow in the same way the normal tests do, although I don't think I've verified that sabotaging renegotiation causes these checks to fail. I've filed an investigation bug; https://bugzilla.mozilla.org/show_bug.cgi?id=1166832

> Maybe we should add a default in the WebIDL instead.

I'll do that.
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support r=mt

Pull down this commit:

hg pull -r 915e60b8fd77a80eb57d0aeaa84424898ec812f2 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

Need review for a webidl change.
Attachment #8580400 - Flags: review?(bugs)
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

I was told "rollback" will be added to the spec.

Not about this bug, but why isn't RTCSessionDescriptionInit exactly the same in
Gecko and in the spec? The spec doesn't have default values.
Attachment #8580400 - Flags: review?(bugs) → review+
Keywords: checkin-needed
Never mind, this rotted too.
Keywords: checkin-needed
Comment on attachment 8580400 [details]
MozReview Request: bz://952145/bwc

/r/5775 - Bug 952145: Rollback support r=mt, r=smaug

Pull down this commit:

hg pull -r bd1965a3d8929468e3604506c409a5d218470727 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8580400 - Flags: review+
Check back on try for unrot.
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/61511c9486a9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8580400 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.