Bug 80589

Summary: MediaStream API: Change the PeerConnection to use JSEP instead of ROAP
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebKit APIAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adam.bergkvist, donggwan.kim, gns, harald, hta, ojan, paulirish, s.choi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug 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    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch abarth: review-, gns: commit-queue-

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.