New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 2438 link

Starred by 15 users

Issue metadata

Status: Assigned
Last visit > 30 days ago
NextAction: ----
OS: ----
Pri: 3
Type: Enhancement

Sign in to add a comment

Allow overriding PeerConnectionFactory methods [PATCH]

Reported by, Sep 26 2013

Issue description

Currently all internal methods of the PeerConnectionFactory implementation lack the "virtual" keyword and are marked as "private", so they can't be overwritten from subclasses if somebody wants to extend the existing code.

The attached patch against r4592 changes this (moves into "protected" and marks as "virtual").
1.5 KB View Download
Project Member

Comment 1 by, Sep 27 2013

Those are PRIVATE methods. What's the necessity to overwrite them?
One use case is to use a different MediaEngine (for example a FileMediaEngine) for peerconnections. Currently the engine is created inside "PeerConnectionFactory::Initialize_s" (private and non-virtual).

You can't just implement a completely custom "PeerConnectionFactory" class, as the "PeerConnection" objects need the original "PeerConnectionFactory" passed to the constructor.

That's why my patch changes all previously "private" methods/properties to be "protected" and marks these methods "virtual", so a subclass can re-implement them where necessary. Of course, you could also switch to use factory methods or similar, but I didn't want to introduce larger code changes...
Project Member

Comment 3 by, Sep 27 2013

I'm not sure whether we would consider such a case. 
@ronghua, what's your comments?
The methods are not called very often, so imho the overhead introduced by "virtual" can be ignored. Having this change would really help in using webrtc as a library in other projects!
It's pretty rare for us to provide virtual internal methods that we don't plan on using ourselves.

Typically the way we would deal with this is to provide a virtual for the specific behavior to be overridden, e.g. a CreateMediaEngine method.
That would be fine. Should I update the patch accordingly?

Comment 7 by, Sep 27 2013

I think #5 is a good idea. An alternative is to create a new ctor to take an external MediaEngine, it will be useful for testing as well.
Project Member

Comment 8 by, Sep 27 2013

Ronghua, which do you prefer? If it is simple, I prefer the injection approach to the virtual approach, since it avoids the need to subclass.

Comment 9 by, Sep 27 2013

I feel the same way Justin. Let's go for the injection.
Project Member

Comment 10 by, Sep 27 2013

Labels: -Type-Bug Type-Enhancement Area-PeerConnection
Project Member

Comment 11 by, Oct 2 2013

Status: Assigned
Thanks for the comments!

Comment 12 by, Mar 10 2014

Did we ever get an updated patch for this issue?

Comment 13 by, Mar 12 2014

We didn't get updated patch. I can start work on it if it's important.

Comment 14 by, Oct 16 2014

Labels: icebox EngTriaged
Project Member

Comment 15 by, Jan 12 2015

reassign to peter for re-arrangement.
Project Member

Comment 16 by, Nov 8 2016

Labels: Pri-3

Sign in to add a comment