Bug 150165

Summary: WebRTC: Add MediaEndpoint interface (WebRTC backend abstraction)
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, commit-queue, eric.carlson, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 143211    
Attachments:
Description Flags
Proposed patch
none
Updated patch
eric.carlson: review+
Updated patch for landing none

Description Adam Bergkvist 2015-10-15 06:10:24 PDT 
The MediaEndpoint interface is a lower level WebRTC backend abstraction than RTCPeerConnectionHandler (in the repo today) which is pretty close to the JavaScript API.

MediaEndpoint makes it possible to use different backends, compatible with WebRTC on the media plane, without having to re-implement all the WebRTC API details (since the API code lives in WebCore).
Comment 1 Adam Bergkvist 2016-02-19 11:50:18 PST 
Created attachment 271769 [details]
Proposed patch
Comment 2 Adam Bergkvist 2016-02-19 13:50:14 PST 
Created attachment 271795 [details]
Updated patch

Fixed debug build issues.
Comment 3 Eric Carlson 2016-02-23 13:31:04 PST 
Comment on attachment 271795 [details]
Updated patch

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

> Source/WebCore/ChangeLog:11
> +        configure the WebRTC backend how to send and receive. It also abstracts

Nit: "configure the WebRTC backend how to send and receive" -> something like "configure how the the WebRTC backend sends and receives"

> Source/WebCore/platform/mediastream/IceCandidate.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Nit: you might want to update this.

> Source/WebCore/platform/mediastream/IceCandidate.h:103
> +        : m_componentId(0)
> +        , m_priority(0)
> +        , m_port(0)
> +        , m_relatedPort(0)

These should be initialized in the class definition:

    unsigned m_componentId { 0 };
    int m_priority { 0 };

etc.

> Source/WebCore/platform/mediastream/MediaEndpoint.cpp:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaEndpoint.cpp:40
> +    return nullptr;

Should this ASSERT?

> Source/WebCore/platform/mediastream/MediaEndpoint.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaEndpointConfiguration.cpp:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaEndpointConfiguration.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:72
> +        : m_sessionId(cryptographicallyRandomNumber()) // FIXME: should be 64 bits

Can you initialize this by bit-shifting two random 32 bit numbers?

> Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:73
> +        , m_sessionVersion(0)

This should be initialized in the class definition.

> Source/WebCore/platform/mediastream/MediaPayload.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/MediaPayload.h:102
> +        : m_type(0)
> +        , m_clockRate(0)
> +        , m_channels(0)
> +        , m_ccmfir(false)
> +        , m_nackpli(false)
> +        , m_nack(false)

These should be initialized in the class definition.

> Source/WebCore/platform/mediastream/PeerMediaDescription.h:2
> + * Copyright (C) 2015 Ericsson AB. All rights reserved.

Update?

> Source/WebCore/platform/mediastream/PeerMediaDescription.h:154
> +        : m_port(0)
> +        , m_rtcpMux(false)
> +        , m_rtcpPort(0)
> +        , m_source(nullptr)

Ditto.
Comment 4 Adam Bergkvist 2016-02-24 02:44:47 PST 
Created attachment 272108 [details]
Updated patch for landing
Comment 5 Adam Bergkvist 2016-02-24 02:52:10 PST 
(In reply to comment #3)
> Comment on attachment 271795 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271795&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        configure the WebRTC backend how to send and receive. It also abstracts
> 
> Nit: "configure the WebRTC backend how to send and receive" -> something
> like "configure how the the WebRTC backend sends and receives"

Fixed.

> > Source/WebCore/platform/mediastream/IceCandidate.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Nit: you might want to update this.

This code has been available open source since 2015 and hasn't changed much (<10 lines) since. I can follow up with an update if it's wrong to have it 2015 only.

> > Source/WebCore/platform/mediastream/IceCandidate.h:103
> > +        : m_componentId(0)
> > +        , m_priority(0)
> > +        , m_port(0)
> > +        , m_relatedPort(0)
> 
> These should be initialized in the class definition:
> 
>     unsigned m_componentId { 0 };
>     int m_priority { 0 };
> 
> etc.

Fixed.

Update style guide lines if this is the new preference?

> 
> > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaEndpoint.cpp:40
> > +    return nullptr;
> 
> Should this ASSERT?

This factory function is used when there are no implementations. The value is asserted in the MediaEndpointPeerConnection implementation.

> 
> > Source/WebCore/platform/mediastream/MediaEndpoint.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.cpp:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaEndpointConfiguration.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:72
> > +        : m_sessionId(cryptographicallyRandomNumber()) // FIXME: should be 64 bits
> 
> Can you initialize this by bit-shifting two random 32 bit numbers?

That should work. Fixed.

> > Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:73
> > +        , m_sessionVersion(0)
> 
> This should be initialized in the class definition.

Fixed.

> > Source/WebCore/platform/mediastream/MediaPayload.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/MediaPayload.h:102
> > +        : m_type(0)
> > +        , m_clockRate(0)
> > +        , m_channels(0)
> > +        , m_ccmfir(false)
> > +        , m_nackpli(false)
> > +        , m_nack(false)
> 
> These should be initialized in the class definition.

Fixed.

> > Source/WebCore/platform/mediastream/PeerMediaDescription.h:2
> > + * Copyright (C) 2015 Ericsson AB. All rights reserved.
> 
> Update?
> 
> > Source/WebCore/platform/mediastream/PeerMediaDescription.h:154
> > +        : m_port(0)
> > +        , m_rtcpMux(false)
> > +        , m_rtcpPort(0)
> > +        , m_source(nullptr)
> 
> Ditto.

Fixed
Comment 6 WebKit Commit Bot 2016-02-24 14:23:49 PST 
Comment on attachment 272108 [details]
Updated patch for landing

Clearing flags on attachment: 272108

Committed r197053: <http://trac.webkit.org/changeset/197053>