GNOME Bugzilla – Bug 784737
NetTimeProvider & NetClientClock QoS DSCP support
Last modified: 2017-11-01 09:01:19 UTC
Feature request to support for DiffServ QoS on clock data packets, by adding possibility to set DSCP value in NetTimeProvider and NetClientClock. Attaching patches for review.
Created attachment 355235 [details] [review] nettimeprovider
Created attachment 355236 [details] [review] netclientclock
Hey guys, any chance someone have given this a look even if sunny summer weather? :)
Follow up comment due to missing feedback. Please have a look. Setting QoS DSCP pattern is more or less taken from udpsink, so not big difference in code.
commit 4c8058272edf77766fa51d3a55d4cdc2fa3c816d (HEAD -> master) Author: Robert Rosengren <robertr@axis.com> Date: Fri Jun 2 16:27:29 2017 +0200 netclientclock: Add possibility to set QoS DSCP value https://bugzilla.gnome.org/show_bug.cgi?id=784737 commit a3e1fca7eee4a1330d2b62ea136aa475a3c5dc57 Author: Robert Rosengren <robertr@axis.com> Date: Thu Jun 1 15:48:16 2017 +0200 nettimeprovider: Add possibility to set QoS DSCP value https://bugzilla.gnome.org/show_bug.cgi?id=784737
This breaks windows build. https://ci.gstreamer.net/job/cerbero-cross-mingw32/6703/console G_OS_WIN32 is *not* defined before you import glibconfig.h => Move the #ifndef G_OS_WIN32 to after the gst includes => Wrap all the setsockopt code with #ifndef #G_OS_WIN32
Robert, can you provide an additional patch for the above?
I see that I missed "#ifdef IP_TOS" around the socket code, as done in gstmultiudpsink.c. Sorry for that, but I don't have time to prepare patch until Monday.
Created attachment 362532 [details] [review] Fix for build error and move duplicated code to util function
regarding util function placement, used the closest common place for both nettimeprovider and netclientclock. Another choice would possibly be gstutil, making it possible for multiudpsink to use same function? But, this will give dependency to gio library throughout gstreamer...
That function is network related, putting it there is fine. And the udp plugin already links to libgstnet, so all good. Not sure what gio has to do with this ?
GSocket comes from glib's gio. Made quick test putting it into gstutil, but it gave me linker errors that I suppose was due to this new dependency. Didn't investigate it more though...
Here is the error: ./../../gst/.libs/libgstreamer-1.0.so: undefined reference to `g_socket_get_fd'
Put it in gstreamer/libs/gst/net somewhere as an utility function
Comment on attachment 362532 [details] [review] Fix for build error and move duplicated code to util function obsolete patch
Created attachment 362600 [details] [review] gstnetutils added Adds gstnetutils as internal helpers. Some thoughts/questions: - Maybe to be exposed header? To be accessible and used from multiudpsink. - Copyright header correct in new c/h-file? Added slomo as the code in gstnetutil is mostly copied from multiudpsink, where git blame pointed in that direction :) Please review and test build on Windows.
Review of attachment 362600 [details] [review]: ::: libs/gst/net/gstnetutils.c @@ +17,3 @@ + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ #ifdef HAVE_CONFIG_H #include "config.h" #endif @@ +40,3 @@ + * Returns: TRUE if successful, FALSE in case an error occurred. + */ +gboolean gst_net_utils_set_socket_dscp (GSocket * socket, gint qos_dscp) Please run gst-indent over the new .c file ::: libs/gst/net/gstnetutils.h @@ +28,3 @@ +G_BEGIN_DECLS + +gboolean gst_net_utils_set_socket_dscp (GSocket * socket, G_GNUC_INTERNAL if this is not installed
Created attachment 362719 [details] [review] New patch with few review comments fixed
(In reply to Sebastian Dröge (slomo) from comment #17) > ::: libs/gst/net/gstnetutils.h > @@ +28,3 @@ > +G_BEGIN_DECLS > + > +gboolean gst_net_utils_set_socket_dscp (GSocket * socket, > > G_GNUC_INTERNAL if this is not installed Or should we make it external then? Fixed the review comments in new pathc
commit 5d885b9dc73d3007ea04e96c24d0ef30c365bc4d (HEAD -> master) Author: Robert Rosengren <robertr@axis.com> Date: Mon Oct 30 10:49:06 2017 +0100 netutils: Add util for setting socket DSCP Util function for setting QoS DSCP added, to remove duplicated code in netclientclock and nettimeprovider. Fix build error if missing IP_TOS. https://bugzilla.gnome.org/show_bug.cgi?id=784737