Bug 150165 – WebRTC: Add MediaEndpoint interface (WebRTC backend abstraction)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Bergkvist]

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).

[Adam Bergkvist]

Created attachment 271769 [details]
Proposed patch

[Adam Bergkvist]

Created attachment 271795 [details]
Updated patch

Fixed debug build issues.

[Eric Carlson]

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.

[Adam Bergkvist]

Created attachment 272108 [details]
Updated patch for landing

[Adam Bergkvist]

(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

[WebKit Commit Bot]

Comment on attachment 272108 [details]
Updated patch for landing

Clearing flags on attachment: 272108

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