GNOME Bugzilla – Bug 563323
udpsink: reduced cpu usage when using a connected socket
Last modified: 2016-05-22 17:49:07 UTC
When using udpsink with a fd which is an udp socket connected to a peer it is more efficient to use sendto(...,NULL,0) than sendto(...,&theiraddr,len);. Maybe udpsink should have an option to indicate to not use sendto() with remote addr if socket is already connected.
Here is a patch for udpsrc. It adds a isconnected property. When set, recvfrom will pass a null address. Therefore, we do not have the remote address and NetBuffer will not be filled with remote address. It's working for me with a rtpbin behind udpsrc but it is not clean. Since this remote adress will not change for a connected UDP socket, there is no need for adding it to all buffers ! There should be another way to pass the information downstream. But this is not the place to discuss this.
Created attachment 126570 [details] [review] Add property isconnected
Review of attachment 126570 [details] [review]: Regarding the functionality, what happens if one sets is-connected=TRUE and it is not connected. It just does not work, right? I guess the connection status is not detectable? Try to ping people on IRC to get feedback on the functionality, my review comment where code-style only. ::: gst/udp/gstudpsrc.c @@ +185,1 @@ Just a formality. PROP_IS_CONNECTED would be preferred. @@ +310,1 @@ And here is would be "is-connected". Also please use G_PARAM_STATIC_STRINGS. @@ +333,3 @@ udpsrc->externalfd = (udpsrc->sockfd != -1); udpsrc->auto_multicast = UDP_DEFAULT_AUTO_MULTICAST; + udpsrc->isconnected = UDP_DEFAULT_ISCONNECTED; and this could be ->is_connected @@ +477,3 @@ + len = sizeof (struct sockaddr); + } + run gst-indent before making the patch.
one more thought, can't the kernel figure this out?
Aurelien, were you able to discuss on irc and rework on patch ?
For udpsrc, recvfrom with no address is ok. When receiving RTP packets, I do not need the sender address because I already know it (that's why I connected my socket). PROP_IS_CONNECTED is therefore a mean to say we do not need the source address, and enhance performance by not copying address. Maybe a better name would be PROP_GET_SOURCE_ADDRESS ? For udpsink, sendto with no address will fail, so there is a need to know wether socket is connected or no. There is sure a way to check wether the socket is connected or not (available in /proc/net/udp for instance). Checking wether socket is connected or not each time one must send a packet will not improve performance as expected by removing the address from sendto. Checking wether the socket is connected or not the first time one send a packet could be ok, but socket can be un-connected and sendto will fail. The fall back mechanism is a bit too complicated for the goal to achieve : do not set address in sendto ... The application is the one connecting and unconnecting the socket, it should set the flag IS_CONNECTED to true or false whenever doing it if it looks for performance enhancement. This is the simpliest way to do it. In fact, I do not use udpsrc nor udpsink but appsrc fed by epoll/recv and fakesink with a handoff signal.
Now that we use gio for udpsink, is this still possible ? Is it maybe already fixed in gio ?
Doesn't we bind/connect the socket now because of the way it works on Windows ? Anyway, optimisation shall have started with a performance test no ?
What does "connected" mean here at all? UDP is connectionless. We currently bind the socket to the local sender address/port (by default ANY address and port 0) as that's required for Windows (and also seems more correct from reading the docs). GIO has no special magic, if we call sendto() it will call sendto() and pass our address there.
If you call connect() on a udp socket, it will set the default send address, then you can call send() instead of sendto(). That said, I'm not sure that any of this is relevant to 1.x.
Well, nothing has changed in that regard and GLib does not "connect" UDP sockets either. Is there interest in porting this to 1.0? It still looks conceptionally wrong to me to put a "is-connected" property there though. Was anybody doing any tests for the performance impacts of this?
Created attachment 309273 [details] [review] udpsrc: add "retrieve-sender-address" property I think there is a case for disabling retrieval of the sender address independent of whether the socket is connected, so I'd just add a property for that. GLib knows whether the socket is connected or not (if you connected it via the GLib API anyway), so arguably any other/automatic optimisations should be in GLib.
Not sure what to do with this, I think the remaining optimisations should be in GLib? Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!