Project: webrtc Issues People Development process History Sign in
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 10 users
Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
OS: ----
Pri: 3
Type: Enhancement



Sign in to add a comment
Allow overriding PeerConnectionFactory methods [PATCH]
Reported by m...@joachim-bauch.de, 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").
 
webrtc_overridable_peerconnectionfactory.diff
1.5 KB View Download
Project Member Comment 1 by braveyao@webrtc.org, Sep 27 2013
Owner: braveyao@webrtc.org
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 braveyao@webrtc.org, Sep 27 2013
Cc: wu@webrtc.org juberti@webrtc.org
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 wu@webrtc.org, 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 juberti@webrtc.org, 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 wu@webrtc.org, Sep 27 2013
I feel the same way Justin. Let's go for the injection.
Project Member Comment 10 by juberti@webrtc.org, Sep 27 2013
Labels: -Type-Bug Type-Enhancement Area-PeerConnection
Project Member Comment 11 by braveyao@webrtc.org, Oct 2 2013
Cc: -wu@webrtc.org braveyao@webrtc.org
Owner: wu@webrtc.org
Status: Assigned
Thanks for the comments!
Comment 12 by juberti@google.com, Mar 10 2014
Did we ever get an updated patch for this issue?
Comment 13 by wu@webrtc.org, Mar 12 2014
We didn't get updated patch. I can start work on it if it's important.
Comment 14 by vrk@webrtc.org, Oct 16 2014
Labels: icebox EngTriaged
Project Member Comment 15 by braveyao@webrtc.org, Jan 12 2015
Owner: pthatcher@webrtc.org
reassign to peter for re-arrangement.
Project Member Comment 16 by pthatcher@webrtc.org, Nov 8 2016
Labels: Pri-3
Sign in to add a comment