Bug 80589 - MediaStream API: Change the PeerConnection to use JSEP instead of ROAP
Summary: MediaStream API: Change the PeerConnection to use JSEP instead of ROAP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on: 80692 80699 81206 81207 81322 81333 81339 81341 81652 81657 81924 82450 82584 85491 85988 86098 87480 92106 92380 92866 92867 93091 93117 93119 94801 94813 95064 95198 95543 95565 95734 95839 96092 96097 96268 96989 97086
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-03-08 04:55 PST by Tommy Widenflycht
Modified: 2012-09-19 07:13 PDT (History)
11 users (show)

See Also:


Attachments
Patch (196.18 KB, patch)
2012-03-08 05:05 PST, Tommy Widenflycht
abarth: review-
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-03-08 04:55:09 PST
This patch deprecates the current Peerconnection by renaming it to DeprecatedPeerConnection and adds a new PeerConnection implementation based on:

http://tools.ietf.org/html/draft-ietf-rtcweb-jsep-00

I have made the new PC quite a bit "stupider" and delegate all logic to the embedder. The reason for this is to avoid adding a huge chunk of code to webcore to do SDP parsing/generation/state handling among other things.
Comment 1 Tommy Widenflycht 2012-03-08 05:05:16 PST
Created attachment 130808 [details]
Patch
Comment 2 Tommy Widenflycht 2012-03-08 05:06:43 PST
I know the patch is quite big but all files that has the name Deprecated in them are just renames of existing files.
Comment 3 Gustavo Noronha (kov) 2012-03-08 05:13:11 PST
Comment on attachment 130808 [details]
Patch

Attachment 130808 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11865276
Comment 4 Adam Bergkvist 2012-03-08 06:24:27 PST
(In reply to comment #0)
> This patch deprecates the current Peerconnection by renaming it to DeprecatedPeerConnection and adds a new PeerConnection implementation based on:

I would have preferred to call the new files JSEPPeerConnection or ExperimentalPeerConnection since the API discussion is still ongoing in W3C. The one you call DeprecatedPeerConnection is the one in the current specification (http://dev.w3.org/2011/webrtc/editor/webrtc.html).
Comment 5 Harald Tveit Alvestrand 2012-03-08 06:37:23 PST
The RTCWEB / WEBRTC consensus seems to go with JSEP:

http://www.w3.org/2012/02/01-webrtc-minutes.html#item03 (Mountain View meeting) ended up with going with JSEP unless alternatives were found. No alternatives have been proposed to date, and the main ROAP author is now a co-author of JSEP.

See http://www.ietf.org/proceedings/interim/2012/01/31/rtcweb/minutes/rtcweb.txt for the RTCWEB discussion.

It's taken a while to get the editors to do the editing.
Comment 6 Adam Bergkvist 2012-03-08 07:35:03 PST
(In reply to comment #5)
> The RTCWEB / WEBRTC consensus seems to go with JSEP:
> 
> http://www.w3.org/2012/02/01-webrtc-minutes.html#item03 (Mountain View meeting) ended up with going with JSEP unless alternatives were found. No alternatives have been proposed to date, and the main ROAP author is now a co-author of JSEP.

It's true that there's consensus around the JSEP model. However the discussion around the API, to interface with JSEP, is still ongoing - that's why I suggested to use the "Experimental" prefix at this point.
Comment 7 Tommy Widenflycht 2012-03-08 07:46:38 PST
My take on this is that the API will change to a new model and therefore the old API will be deprecated. Therefore the renaming of current PeerConnection to DeprecatedPeerConnection is correct imho.
Comment 8 Adam Barth 2012-03-08 14:27:21 PST
> It's true that there's consensus around the JSEP model. However the discussion around the API, to interface with JSEP, is still ongoing - that's why I suggested to use the "Experimental" prefix at this point.

The risk of using a name like JSEPPeerConnection is that we'll be stuck with that as the final name for the API.  If we us PeerConnection for JSEP and change the existing API to DeprecatedPeerConnection, there's no risk of getting stuck with the wrong name for the final API.

Also, using this naming arrangement encourages folks to move over to the new API, making it easier for us to remove DeprecatedPeerConnection when that's appropriate.
Comment 9 Adam Barth 2012-03-08 14:38:57 PST
Comment on attachment 130808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130808&action=review

This patch is much to big to review.  I noted a few minor issues.  Mostly we need to divide this patch into many smaller pieces.  For example, I'd start with one patch that just renames the existing APIs.  Then I'd introduce the new interfaces in a series of patches.

> Source/WebCore/Modules/mediastream/IceCallback.h:43
> +class IceCallback : public RefCounted<IceCallback> {

I would have expected this to inherit from one of our callback base classes, but maybe that's not necessary anymore.

> Source/WebCore/Modules/mediastream/IceCandidate.h:61
> +    RefPtr<IceCandidateBase> m_base;

Why isn't this a base class?

> Source/WebCore/Modules/mediastream/IceCandidate.idl:41
> +        // the m= line this candidate is associated with
> +        readonly attribute DOMString label;
> +
> +        // creates a SDP-ized form of this candidate
> +        DOMString toSdp();

Generally we write comments like these in IDL files.  That's what the specs are for.  :)

> Source/WebCore/Modules/mediastream/MediaHints.h:60
> +    RefPtr<MediaHintsBase> m_base;

I don't really understand this pattern.  Why are these "base" objects being held as members?

> Source/WebCore/Modules/mediastream/PeerConnection.cpp:375
> +bool PeerConnection::canSuspend() const
> +{
> +    return true;
> +}

Suspending is really complex.  Are you sure you're implementing this correctly?  I'd recommend just returning false for now and worrying about suspension later.
Comment 10 Adam Barth 2012-03-08 14:40:35 PST
It looks like your patch is 196.18 KB.  The tool that creates the patch file is supposed to warning you whenever you create a patch that's larger than 20KB.  It looks like you're over that guideline by an order of magnitude.

(Of course, not all patches can be small.  For example, I'd expect the renaming patch to be larger---but that's easy to rubber-stamp as long as you're just moving code around and not introducing new code.)
Comment 11 Tommy Widenflycht 2012-03-09 05:16:56 PST
Comment on attachment 130808 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130808&action=review

>> Source/WebCore/Modules/mediastream/IceCallback.h:43
>> +class IceCallback : public RefCounted<IceCallback> {
> 
> I would have expected this to inherit from one of our callback base classes, but maybe that's not necessary anymore.

No, the person that worked on the IDL parser simplified things a lot. Specifying "Callback" in the IDL fixes everything else.

>> Source/WebCore/Modules/mediastream/IceCandidate.h:61
>> +    RefPtr<IceCandidateBase> m_base;
> 
> Why isn't this a base class?

It is the split into WebCore proper and WebCore platform that complicates things. Since IceCandidates and SessionDescriptions can be created both from the embedder and JavaScript this kind of pattern is necessary. We do the same with MediaStream and MediaStreamDescriptor but since SessionDescriptionDescriptor is kind of clunky I choose to use *Base instead. In hindsight that was probably more confusing than helpful.

>> Source/WebCore/Modules/mediastream/IceCandidate.idl:41
>> +        DOMString toSdp();
> 
> Generally we write comments like these in IDL files.  That's what the specs are for.  :)

Yeah! Comments in WebKit. Will fix.

>> Source/WebCore/Modules/mediastream/MediaHints.h:60
>> +    RefPtr<MediaHintsBase> m_base;
> 
> I don't really understand this pattern.  Why are these "base" objects being held as members?

See above.

>> Source/WebCore/Modules/mediastream/PeerConnection.cpp:375
>> +}
> 
> Suspending is really complex.  Are you sure you're implementing this correctly?  I'd recommend just returning false for now and worrying about suspension later.

Fixed.
Comment 12 Tommy Widenflycht 2012-03-09 05:20:17 PST
The renaming patch is out now (bug 80962) and is ~145KB.

And btw, the script doesn't complain in the least.

(In reply to comment #10)
> It looks like your patch is 196.18 KB.  The tool that creates the patch file is supposed to warning you whenever you create a patch that's larger than 20KB.  It looks like you're over that guideline by an order of magnitude.
> 
> (Of course, not all patches can be small.  For example, I'd expect the renaming patch to be larger---but that's easy to rubber-stamp as long as you're just moving code around and not introducing new code.)
Comment 13 Adam Bergkvist 2012-03-09 06:56:31 PST
(In reply to comment #8)
> The risk of using a name like JSEPPeerConnection is that we'll be stuck with that as the final name for the API.  If we us PeerConnection for JSEP and change the existing API to DeprecatedPeerConnection, there's no risk of getting stuck with the wrong name for the final API.

Isn't the final name pending until the prefixes are removed anyhow? But I agree that JSEPPeerConnection is bad. ExperimentalPeerConnection would be better IMO.

> Also, using this naming arrangement encourages folks to move over to the new API, making it easier for us to remove DeprecatedPeerConnection when that's appropriate.

The discussion is still ongoing in W3C on which changes JSEP will have on the API. It shouldn't prevent anyone from prototyping though.
Comment 14 Adam Barth 2012-03-09 09:27:10 PST
Adam: I'm going to reply to your comment on Bug 80692 because that's where the rename patch is now.
Comment 15 Tommy Widenflycht 2012-09-19 07:13:25 PDT
Closing this bug since RTCPeerConnection has now landed, and the old ROAD based implementation has been deleted.