WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 150165
WebRTC: Add MediaEndpoint interface (WebRTC backend abstraction)
https://bugs.webkit.org/show_bug.cgi?id=150165
Summary
WebRTC: Add MediaEndpoint interface (WebRTC backend abstraction)
Adam Bergkvist
Reported
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).
Attachments
Proposed patch
(53.32 KB, patch)
2016-02-19 11:50 PST
,
Adam Bergkvist
no flags
Details
Formatted Diff
Diff
Updated patch
(54.74 KB, patch)
2016-02-19 13:50 PST
,
Adam Bergkvist
eric.carlson
: review+
Details
Formatted Diff
Diff
Updated patch for landing
(54.51 KB, patch)
2016-02-24 02:44 PST
,
Adam Bergkvist
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Adam Bergkvist
Comment 1
2016-02-19 11:50:18 PST
Created
attachment 271769
[details]
Proposed patch
Adam Bergkvist
Comment 2
2016-02-19 13:50:14 PST
Created
attachment 271795
[details]
Updated patch Fixed debug build issues.
Eric Carlson
Comment 3
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.
Adam Bergkvist
Comment 4
2016-02-24 02:44:47 PST
Created
attachment 272108
[details]
Updated patch for landing
Adam Bergkvist
Comment 5
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
WebKit Commit Bot
Comment 6
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug