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).
Created attachment 271769 [details] Proposed patch
Created attachment 271795 [details] Updated patch Fixed debug build issues.
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.
Created attachment 272108 [details] Updated patch for landing
(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 on attachment 272108 [details] Updated patch for landing Clearing flags on attachment: 272108 Committed r197053: <http://trac.webkit.org/changeset/197053>