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

Added port-range property to PlayerEndpoint #14

Merged
merged 4 commits into from Nov 23, 2018

Conversation

Projects
None yet
3 participants
@chax
Copy link
Contributor

chax commented Nov 22, 2018

This PR resolves issue Kurento/bugtracker#310

It has new config file PlayerEndpoint.conf.ini
with one property which is currently commented out rtspSrcPortRange
This new property defines port range for streaming from RTSP source and restricts Kurento to using only ports within that range.

For example, if you put
rtspSrcPortRange=30000-30012
Kurento PlayerEndpoint will be restricted to use only ports from that range.
This is useful if you want to implement stricter firewall rules.

@jenkinskurento

This comment has been minimized.

Copy link

jenkinskurento commented Nov 22, 2018

Hi there. Thanks for your PR.

I'm waiting for a Kurento member to verify that this patch is reasonable to test. If it is, they should reply with check out please on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

@jenkinskurento

This comment has been minimized.

Copy link

jenkinskurento commented Nov 22, 2018

There were errors, for info, please see...

@j1elo
Copy link
Contributor

j1elo left a comment

Better to not expose too much info about the internal implementation. This setting is meant to control RTSP client ports, so "RTSP_CLIENT" is better naming than "RTSP_SRC", which could be confusing for users who don't necessarily know that the internal object is named "GstRtspSrc".

@@ -1089,6 +1099,11 @@ kms_player_endpoint_class_init (KmsPlayerEndpointClass * klass)
0, G_MAXINT, NETWORK_CACHE_DEFAULT,
G_PARAM_READWRITE | GST_PARAM_MUTABLE_READY));

g_object_class_install_property (gobject_class, PROP_PORT_RANGE,
g_param_spec_string ("port-range", "UDP Port range for RTSP source",

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
g_param_spec_string ("port-range", "UDP Port range for RTSP source",
g_param_spec_string ("port-range", "Port range for RTSP client",
@@ -1089,6 +1099,11 @@ kms_player_endpoint_class_init (KmsPlayerEndpointClass * klass)
0, G_MAXINT, NETWORK_CACHE_DEFAULT,
G_PARAM_READWRITE | GST_PARAM_MUTABLE_READY));

g_object_class_install_property (gobject_class, PROP_PORT_RANGE,
g_param_spec_string ("port-range", "UDP Port range for RTSP source",
"When using rtsp sources, what range of UDP ports can be allocated",

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
"When using rtsp sources, what range of UDP ports can be allocated",
"Range of ports that can be allocated when acting as RTSP client",
@@ -0,0 +1,3 @@
;; This is setting from gstreamer plugin gst-plugins-good

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
;; This is setting from gstreamer plugin gst-plugins-good
;; Range of ports that can be allocated when acting as RTSP client
@@ -0,0 +1,3 @@
;; This is setting from gstreamer plugin gst-plugins-good
;; https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good/html/gst-plugins-good-plugins-rtspsrc.html#GstRTSPSrc--port-range

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
;; https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good/html/gst-plugins-good-plugins-rtspsrc.html#GstRTSPSrc--port-range
@@ -0,0 +1,3 @@
;; This is setting from gstreamer plugin gst-plugins-good
;; https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-good/html/gst-plugins-good-plugins-rtspsrc.html#GstRTSPSrc--port-range
;rtspSrcPortRange=<PortMin-PortMax>

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
;rtspSrcPortRange=<PortMin-PortMax>
;rtspClientPortRange=<PortMin-PortMax>
@@ -36,6 +36,7 @@ GST_DEBUG_CATEGORY_STATIC (GST_CAT_DEFAULT);
#define PIPELINE "pipeline"
#define SET_POSITION "set-position"
#define NS_TO_MS 1000000
#define RTSP_SRC_PORT_RANGE "rtspSrcPortRange"

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
#define RTSP_SRC_PORT_RANGE "rtspSrcPortRange"
#define RTSP_CLIENT_PORT_RANGE "rtspClientPortRange"
@@ -106,6 +107,13 @@ PlayerEndpointImpl::PlayerEndpointImpl (const boost::property_tree::ptree &conf,

g_object_set (G_OBJECT (element), "use-encoded-media", useEncodedMedia,
"network-cache", networkCache, NULL);

try {
std::string portRange = getConfigValue <std::string, PlayerEndpoint> (RTSP_SRC_PORT_RANGE);

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
std::string portRange = getConfigValue <std::string, PlayerEndpoint> (RTSP_SRC_PORT_RANGE);
std::string portRange = getConfigValue <std::string, PlayerEndpoint> (RTSP_CLIENT_PORT_RANGE);
std::string portRange = getConfigValue <std::string, PlayerEndpoint> (RTSP_SRC_PORT_RANGE);
g_object_set (G_OBJECT (element), "port-range", portRange.c_str(), NULL);
} catch (boost::property_tree::ptree_error &) {
GST_DEBUG ("PlayerEndpoint config file doesn't contain property %s. Ignoring.", RTSP_SRC_PORT_RANGE);

This comment has been minimized.

@j1elo

j1elo Nov 23, 2018

Contributor
Suggested change Beta
GST_DEBUG ("PlayerEndpoint config file doesn't contain property %s. Ignoring.", RTSP_SRC_PORT_RANGE);
GST_DEBUG ("PlayerEndpoint config file doesn't contain property %s. Ignoring.", RTSP_CLIENT_PORT_RANGE);
@chax

This comment has been minimized.

Copy link
Contributor

chax commented Nov 23, 2018

@j1elo thanks for the review, i will try to fix all these things right away so you can review it again

@chax

This comment has been minimized.

Copy link
Contributor

chax commented Nov 23, 2018

@j1elo please re-review, i also added cleanup of port_range property in kms_player_endpoint_finalize() and kms_player_endpoint_dispose() methods.

@j1elo

This comment has been minimized.

Copy link
Contributor

j1elo commented Nov 23, 2018

Hi, thanks for these latest changes. Just a minor detail:
dispose() is to unref all referenced objects (if any), and finalize() is meant to free all memory or close file descriptors, if any.
So, the g_free() and NULL assignment only need to be done in finalize().

@chax

This comment has been minimized.

Copy link
Contributor

chax commented Nov 23, 2018

@j1elo removed from _dispose()

@j1elo

j1elo approved these changes Nov 23, 2018

Copy link
Contributor

j1elo left a comment

Seems pretty good now, ready to merge

@j1elo j1elo merged commit f722ee3 into Kurento:master Nov 23, 2018

@chax chax deleted the HomeControlAS:issue_310_expose_gstrtspsrc_port_range_property branch Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment