-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IceConnection Interface #877
Conversation
std::string clientHostType; | ||
}; | ||
|
||
class IceConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lodoyun can you review the new IceConfig? I added media_type transport_name, connection_id, ice_components, username and password and removed them from the NiceConnection constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, at some point (not in this PR) we should probably refactor how we pass the configuration. Right now it comes directly from erizoAPI and is filled in progressively in several steps, there's probably a more consistent way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice! In general I think it's a good idea to progressively abstract the Ice operations in the interface and get them closer to the standard, instead of what a particular library (Libnice in this case) does.
erizo/src/erizo/DtlsTransport.cpp
Outdated
@@ -10,7 +10,8 @@ | |||
|
|||
#include "./SrtpChannel.h" | |||
#include "rtp/RtpHeaders.h" | |||
#include "lib/LibNiceInterface.h" | |||
#include "./NiceConnection.h" | |||
#include "./NicerConnection.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could find an alternative name for one of them. I'm sure I will confuse Nice
and Nicer
at some point. Maybe LibNiceConnection
and NicerConnection
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is funny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep 😄 , I renamed NiceConnection to LibNiceConnection anyway to avoid confusions in the future, as Pedro suggested before.
std::string clientHostType; | ||
}; | ||
|
||
class IceConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, at some point (not in this PR) we should probably refactor how we pass the configuration. Right now it comes directly from erizoAPI and is filled in progressively in several steps, there's probably a more consistent way to do it.
…de into add/ice_connection_interface
Description
It adds a new C++ interface for IceConnection. It will help us using libraries others than libNice in the future. I also create an empty instantiation of a NicerConnection that also inherits from IceConnection. We will be able to enable it in the future by adding -DUSE_NICER to the builds.
[] It needs and includes Unit Tests
Changes in Client or Server public APIs
Not needed.
[] It includes documentation for these changes in
/doc
.