New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 11 users
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 Back to list
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