GNOME Bugzilla – Bug 610364
udpsrc: allocates buffers with size a lot bigger than needed
Last modified: 2014-09-09 17:07:42 UTC
Background: overloaded computer receiving RTP packets through udpsrc with parameter "buffer-size" set to 1*1024*1024. Result: GLib-ERROR **: gmem.c:136: failed to allocate 1044144 bytes aborting... Personal analysis: I didn't found why g_malloc() fails here but it is not so relevant for this bug report. The bug declared here is the size of the buffer being allocated by udpsrc. In fact, each UDP packet is normally stored in one gst buffer and there is no apparent reasons to have a so big buffer. By looking inside the source code, I saw (gstudpsrc.c:470): /* ask how much is available for reading on the socket, this should be exactly * one UDP packet. We will check the return value, though, because in some * case it can return 0 and we don't want a 0 sized buffer. */ readsize = 0; if (G_UNLIKELY ((ret = IOCTL_SOCKET (udpsrc->sock.fd, FIONREAD, &readsize)) < 0)) goto ioctl_failed; This code, on Mac OS X, return the total amount of data stored in the reception buffer. This is not the size of the next UDP packet as expected in the code. In my understanding, the code is allocating a buffer based on <readsize> to store a single UDP packet. So, the buffer can be a lot bigger than needed, generating waste of memory and other side effects. On the other hand, g_malloc() have no reason to fail (my computer is not running out of memory). There is probably another bug that will be handled unlinked to this one.
On Linux, ioctl(FIONREAD) on a udp socket returns the size of the next packet, so it is correct. That said, on windows and osx, it returns the total. On Windows, we need to call WSAIoctl(FIONREAD)l, not ioctlsocket() to get the right value. On OSX, it should call getsockopt(SO_NREAD). I guess this is a case for #ifdefs
The code shown above is not the only location where readsize is computed. The same happens few line above.
An #ifdef sounds acceptable. Anyone care to make a patch for this?
Hrm, tricky. So we use g_socket_get_available_bytes() now in udpsrc, which however is implemented as: #ifndef G_OS_WIN32 if (ioctl (socket->priv->fd, FIONREAD, &avail) < 0) return -1; #else if (ioctlsocket (socket->priv->fd, FIONREAD, &avail) == SOCKET_ERROR) return -1; #endif On Windows, "FIONREAD returns the amount of data that can be read in a single call to the recv function, which may not be the same as the total amount of data queued on the socket. If s is message oriented (for example, type SOCK_DGRAM), FIONREAD still returns the amount of pending data in the network buffer, however, the amount that can actually be read in a single call to the recv function is limited to the data size written in the send or sendto function call.", which means this is not really what we want. On OSX, ioctl(FIONREAD) returns the total buffersize according to Olivier in comment #1, so also not what we want. Question is if the GLib folks let us fix _get_available_bytes(), which arguably slightly changes the semantics, or whether we need a new function for that.
Also see bug #686786.
Created attachment 232483 [details] [review] udpsrc: sanity check size of available packet data for reading to avoid memory waste Naive patch which might help a little in some cases. Still far from optimal of course. On Windows and OS/X, _get_available_bytes() may not return the size of the next pending packet, but the size of all pending packets in the kernel-side buffer, which might be rather large depending on configuration. Sanity-check the size returned by _get_available_bytes() to make sure we never allocate more memory than the max. size for a packet, if it's an IPv4 socket.
That's probably the best we can do for now, Windows does not even seem to have an API for that... and the only way seems to be to peek all the data to get the size. And then later get it. Which is quite inefficient of course
Comment on attachment 232483 [details] [review] udpsrc: sanity check size of available packet data for reading to avoid memory waste Committed this for now, with 65536-8 as max size instead of 65536 though.
I guess we did all we can do here, so let's close this unless someone has a more clever idea.
I did have a more clever idea actually, will implement that soon.
This should do the trick: commit e6f77948acb5fdd5bac673a4ad669ed4360a6952 Author: Tim-Philipp Müller <tim@centricular.com> Date: Tue Sep 9 13:46:56 2014 +0100 udpsrc: more efficient memory handling Drop use of g_socket_get_available_bytes() which is not useful on all systems (where it returns the size of the entire buffer not that of the next pending packet), and is yet another syscall and apparently very inefficient on Windows in the UDP case. Instead, when reading UDP packets, use the more featureful g_socket_receive_message() call that allows to read into scattered memory, and allocate one memory chunk which is likely to be large enough for a packet, while also providing a larger allocated memory chunk just in case the packet is larger than expected. If the received data fits into the first chunk, we'll just add that to the buffer we return and re-use the fallback buffer for next time, otherwise we add both chunks to the buffer. This reduces memory waste more reliably on systems where get_available_bytes() doesn't work properly. In a multimedia streaming scenario, incoming UDP packets are almost never fragmented and thus almost always smaller than the MTU size, which is also why we don't try to do something smarter with more fallback memory chunks of different sizes. The fallback scenario is just for when someone built a broken sender pipeline (not using a payloader or somesuch) https://bugzilla.gnome.org/show_bug.cgi?id=610364