GNOME Bugzilla – Bug 771525
gst-rtsp-server: Poor performance with interleaved RTSP due to missing buffer list support and merging of all memories in buffers
Last modified: 2018-11-03 15:40:50 UTC
Currently, there is no support for buffer lists in gst-rtsp-server in the tcp case. The overhead of pushing single buffers individually through the pipeline gets very high. I'll provide a patch suggesting a possible solution to this problem. I've already created another bug addressing missing buffer list functionality in appsink: https://bugzilla.gnome.org/show_bug.cgi?id=752363.
Created attachment 335699 [details] [review] Use buffer list functionality
In the proposed solution the buffer list support is used in order to improve performance in the RTP/RTSP case.
When sending single buffers in do_send_data function, the whole buffer is mapped which is not optimal in case when buffers consist of several memory blocks. This part of the code is not changed in the proposed patch. It requires some changes to GstRTSPMessage to support iovec:s ...
Comment on attachment 335699 [details] [review] Use buffer list functionality Hi, Thanks for the patch! >diff --git a/gst/rtsp-server/rtsp-stream-transport.h b/gst/rtsp-server/rtsp-stream-transport.h >index 8e2d366..2a83de5 100644 >--- a/gst/rtsp-server/rtsp-stream-transport.h >+++ b/gst/rtsp-server/rtsp-stream-transport.h >@@ -53,7 +53,7 @@ typedef struct _GstRTSPStreamTransportPrivate GstRTSPStreamTransportPrivate; > * > * Returns: %TRUE on success > */ >-typedef gboolean (*GstRTSPSendFunc) (GstBuffer *buffer, guint8 channel, gpointer user_data); >+typedef gboolean (*GstRTSPSendFunc) (GstMiniObject *obj, guint8 channel, gpointer user_data); I am not sure if this is something we can do. Not because of the type change in the header file, but is it possible that someone subclassed this externally and overrode this vfunc? And now they might suddenly get passed a buffer list instead of a buffer. Also, it makes things difficult for bindings, which don't know that GstBuffer and GstBufferList are mini objects. Don't know if that would ever make sense in practice, but it is a public header if I'm not mistaken. I think the best would be to add a new SendListFunc + vfunc, and make gst_rtsp_stream_transport_send_rtp_list() iterate using ->send() if the ->send_list() vfunc is NULL. Does that make sense?
Hi, Thank you very much for your response! Yes, it does make sense. We are working on a new patch.
Created attachment 343409 [details] [review] Add support for buffer lists
Please have a look at the suggested patch. Thank you!
Created attachment 356040 [details] [review] add support for bufferlist while streaming RTSP/TCP without data copying Modified current patch by Patricia to avoid data copying. g_socket_send_message is used so that we send vectors to the appsink without data copying.
Please have a look at the patch. Thank you!
Can you squash both patches into one? The second one seems to largely remove things introduced in the first.
Created attachment 356397 [details] [review] add support for bufferlist while streaming RTSP/TCP without data copying both the patches are stashed into one now.
Review of attachment 356397 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +44,3 @@ #include <stdio.h> #include <string.h> +#include <sys/socket.h> Why do you include this? arent all socket operations abstracted through GIO ? @@ +1037,3 @@ + data_header += 4; + } + GST_DEBUG (" mem:%d, pos: %d", mem, pos); This should be a GST_LOG, it's on every buffer. @@ +4085,3 @@ + num_written = + g_socket_send_message (socket, NULL, &vecs[i], send_len, NULL, 0, + MSG_DONTWAIT, NULL, &err); You shouldn't pull out the socket from here.. Instead you should add the proper API to gst_rtsp_connection_..() And you sholdn't use MSG_DONTWAIT, the parameter here is a GSocketMsgFlags @@ +4087,3 @@ + MSG_DONTWAIT, NULL, &err); + if (err != NULL) + GST_ERROR ("error: %s", err->message); You end up re-printing the error down there, so this one can be downgraded or removed. @@ +4088,3 @@ + if (err != NULL) + GST_ERROR ("error: %s", err->message); + } while (num_written == -1 && (errno == EINTR || errno == EAGAIN)); Instead of looking at errno, you want to look at the err with g_error_matches(). And if you want to g_clear_error(&err) if you decide to ignore the error. @@ +4091,3 @@ + + if (num_written == -1) { + GST_ERROR ("failed to send message: %s", err->message); While we're at it, maybe you should pass the client object here so you can do GST_ERROR_OBJECT().. @@ +4133,3 @@ + total_mems = gst_buffer_n_memory (buffer); + map_infos = g_new (GstMapInfo, total_mems); + vecs = g_new (GOutputVector, total_mems + num_buffers); Why not g_newa() here? @@ +4155,3 @@ + map_infos = g_new (GstMapInfo, total_mems); + vecs = g_new (GOutputVector, total_mems + num_buffers); + data_header = g_malloc (4 * num_buffers); Any reason not to use g_newa() here too ? @@ +4183,3 @@ + GST_ERROR_OBJECT (client, "waiting for backlog"); + ret = gst_rtsp_watch_wait_backlog (priv->watch, &time); + GST_ERROR_OBJECT (client, "Resend due to backlog full"); Those are not really errors, should be at DEBUG level.
Created attachment 356724 [details] [review] add support for bufferlist while streaming RTSP/TCP without data copying
(In reply to Olivier Crête from comment #12) > Review of attachment 356397 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ +44,3 @@ > #include <stdio.h> > #include <string.h> > +#include <sys/socket.h> > > Why do you include this? arent all socket operations abstracted through GIO ? > Thats true, I have removed. > @@ +1037,3 @@ > + data_header += 4; > + } > + GST_DEBUG (" mem:%d, pos: %d", mem, pos); > > This should be a GST_LOG, it's on every buffer. > > @@ +4085,3 @@ > + num_written = > + g_socket_send_message (socket, NULL, &vecs[i], send_len, NULL, 0, > + MSG_DONTWAIT, NULL, &err); > > You shouldn't pull out the socket from here.. Instead you should add the > proper API to gst_rtsp_connection_..() Added API in gst-plugin-base. > > And you sholdn't use MSG_DONTWAIT, the parameter here is a GSocketMsgFlags > Even though the parameter here is GSocketMsgFlags we use MSG_DONTWAIT in g_socket_send_message for making it non blocking. g_socket_send_message in turn uses sendmsg () which support the use of this flag. > @@ +4087,3 @@ > + MSG_DONTWAIT, NULL, &err); > + if (err != NULL) > + GST_ERROR ("error: %s", err->message); > > You end up re-printing the error down there, so this one can be downgraded > or removed. > Removed. > @@ +4088,3 @@ > + if (err != NULL) > + GST_ERROR ("error: %s", err->message); > + } while (num_written == -1 && (errno == EINTR || errno == EAGAIN)); > > Instead of looking at errno, you want to look at the err with > g_error_matches(). And if you want to g_clear_error(&err) if you decide to > ignore the error. > I have used g_error_matches() in the gstrtspconnection api for EAGAIN, After analyzing the code for g_socket_send_message, we found that the error EINTR was never returned. So I have removed the check for it. > @@ +4091,3 @@ > + > + if (num_written == -1) { > + GST_ERROR ("failed to send message: %s", err->message); > > While we're at it, maybe you should pass the client object here so you can > do GST_ERROR_OBJECT().. > > @@ +4133,3 @@ > + total_mems = gst_buffer_n_memory (buffer); > + map_infos = g_new (GstMapInfo, total_mems); > + vecs = g_new (GOutputVector, total_mems + num_buffers); > > Why not g_newa() here? > > @@ +4155,3 @@ > + map_infos = g_new (GstMapInfo, total_mems); > + vecs = g_new (GOutputVector, total_mems + num_buffers); > + data_header = g_malloc (4 * num_buffers); > > Any reason not to use g_newa() here too ? > as you know the g_newa allocate to the stackframe and it was used for allocating where small amount of memory was needed.. thats the reason we didnt use g_newa for map_infos and vecs. I can change the g_newa to g_new if you want to make the code look consistent > @@ +4183,3 @@ > + GST_ERROR_OBJECT (client, "waiting for backlog"); > + ret = gst_rtsp_watch_wait_backlog (priv->watch, &time); > + GST_ERROR_OBJECT (client, "Resend due to backlog full"); > > Those are not really errors, should be at DEBUG level. Changed to debug level.
Created attachment 360649 [details] [review] send messages to rtspconnection without blocking stashed both the patches to one
Comment on attachment 360649 [details] [review] send messages to rtspconnection without blocking This breaks API/ABI with the send functions as now sends both lists and buffers to the send_rtp functions. I have a patch that fixes this, and that as a first step does not depend on any GstRTSPMessage/Watch API additions. Those can come as a next step. I'll attach the new patch in a bit for discussion here
There's also the problem in rtsp-client.c:do_send_data() that it assumes that the buffer can be unmapped after gst_rtsp_watch_send_message(). That's not the case, it can queue up the message and send it at a later time from another thread, and there's nothing at all that ensures that the memory is actually still available or valid at all (it could be unmapped, reused, ...). I'm not sure why this didn't cause huge problems in the past yet.
Created attachment 372851 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Comment on attachment 360649 [details] [review] send messages to rtspconnection without blocking The creation of the actual message, etc should also be done inside GstRTSPConnection, and the GstRTSPMessage should just get the multiple chunks of the body attached to it somehow. I have a plan
Created attachment 373366 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Created attachment 373368 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Created attachment 373374 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Review of attachment 373368 [details] [review]: ::: gst/rtsp-server/rtsp-stream-transport.c @@ +219,3 @@ + * + * Install callbacks that will be called when data for a stream should be sent + * to a client. This is usually used when sending RTP/RTCP over TCP. This need a Since mark, same for other new symbols. ::: gst/rtsp-server/rtsp-stream.c @@ +2424,3 @@ + n_messages += 1; + if (buffer_list) + n_messages += gst_buffer_list_length (buffer_list); Can you really have both buffer and buffer_list set ? I think using else if () if not would be better. If it's the case, how do we know we should send the buffer first ?
Review of attachment 373374 [details] [review]: I've commented the previous patch, but comment still applies.
More work is needed on these patches to be useful, but thanks for the review :) What's missing is specifically: 1) Using writev() in GstRTSPConnection 2) Passing through whole bufferlists from gst-rtsp-server 3) Handling a whole bufferlist as one unit (instead of each buffer), as we now only allow one data message in-flight at a time per interleave id
Created attachment 373684 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Created attachment 373685 [details] [review] rtsp-client: Add support for sending buffer lists directly
The connection part is a bit hard to review really, proof will be in the pudding :) Maybe rename message_to_string() to message_serialize() so it's aligned with the output struct?
Comment on attachment 373684 [details] [review] rtsp-server: Add support for buffer lists Looks fine to me by and large. Some nitpicks: >--- a/gst/rtsp-server/rtsp-stream.c >+++ b/gst/rtsp-server/rtsp-stream.c >@@ -2394,6 +2394,8 @@ send_tcp_message (GstRTSPStream * stream, gint idx) > GList *walk; > GstSample *sample; > GstBuffer *buffer; >+ GstBufferList *buffer_list; >+ guint n_messages = 0; > gboolean is_rtp; > > if (priv->n_outstanding > 0 || !priv->have_buffer[idx]) { >@@ -2414,6 +2416,14 @@ send_tcp_message (GstRTSPStream * stream, gint idx) > } > > buffer = gst_sample_get_buffer (sample); >+ buffer_list = gst_sample_get_buffer_list (sample); >+ >+ /* We will get one message-sent notification per message, >+ * i.e. per buffer that is actually sent out */ >+ if (buffer) >+ n_messages += 1; >+ if (buffer_list) >+ n_messages += gst_buffer_list_length (buffer_list); Why are we doing "+=" here and not "=" and should it be "else if" here for clarity or can we get both a buffer and a buffer list? (don't think so?) "else if" also a few times in the code below. >--- a/tests/check/gst/rtspserver.c >+++ b/tests/check/gst/rtspserver.c >@@ -2117,7 +2117,7 @@ GST_START_TEST (test_record_tcp) > > mfactory = > start_record_server >- ("( rtppcmadepay name=depay0 ! appsink name=sink async=false )"); >+ ("( rtppcmadepay name=depay0 ! appsink name=sink buffer-list=true async=false )"); > > g_signal_connect (mfactory, "media-constructed", > G_CALLBACK (media_constructed_cb), &server_sink); Does this (buffer-list=true) here actually do anything? Is it needed?
Comment on attachment 373685 [details] [review] rtsp-client: Add support for sending buffer lists directly Looks reasonable to the extent that it can be reviewed. >+static gboolean >+do_send_messages (GstRTSPClient * client, GstRTSPMessage * messages, >+ guint n_messages, gboolean close, gpointer user_data) >+{ >+ ... >+ >+ for (i = 0; i < n_messages; i++) { >+ if (gst_rtsp_message_get_type (&messages[i]) == GST_RTSP_MESSAGE_DATA) { >+ guint8 channel = 0; >+ GstRTSPResult r; >+ >+ /* FIXME: We assume that all data messages in the list are for the >+ * same channel */ >+ r = gst_rtsp_message_parse_data (&messages[i], &channel); Is that something that needs fixing before merging? It seems like a reasonable assumption to me in our context? Maybe it needs documenting somewhere?
Review of attachment 373684 [details] [review]: This will require updating sections.txt, lgtm apart from a few comments ::: gst/rtsp-server/rtsp-client.c @@ +1187,3 @@ +static gboolean +do_send_data_list (GstBufferList * buffer_list, guint8 channel, The implementation seems similar to what rtsp-stream-transport will do if not callback was provided, is this mostly a placeholder for future extension? ::: gst/rtsp-server/rtsp-media.c @@ +2187,3 @@ g_object_set (appsrc, "block", TRUE, "format", GST_FORMAT_TIME, "is-live", + TRUE, "emit-signals", FALSE, NULL); + g_object_set (appsink, "sync", FALSE, "async", FALSE, "emit-signals", emit-signals is false by default for appsink ::: gst/rtsp-server/rtsp-stream-transport.c @@ +219,3 @@ + * + * Install callbacks that will be called when data for a stream should be sent + * to a client. This is usually used when sending RTP/RTCP over TCP. Since: @@ -531,0 +566,7 @@ +/** + * gst_rtsp_stream_transport_send_rtp_list: + * @trans: a #GstRTSPStreamTransport ... 4 more ... Since: @@ +614,3 @@ + * + * Send @buffer_list to the installed RTCP callback for @trans. + * Since: ::: gst/rtsp-server/rtsp-stream.c @@ +2474,3 @@ + send_ret = gst_rtsp_stream_transport_send_rtp (tr, buffer); + if (buffer_list) + send_ret = gst_rtsp_stream_transport_send_rtp_list (tr, buffer_list); this will ignore any potential error if the sample contained both a buffer and a buffer list, not sure if that is allowed to happen in practice @@ +3332,3 @@ /* make appsink */ priv->appsink[i] = gst_element_factory_make ("appsink", NULL); + g_object_set (priv->appsink[i], "emit-signals", FALSE, "buffer-list", emit-signals is FALSE by default in appsink ::: gst/rtsp-sink/gstrtspclientsink.c @@ +3853,3 @@ +static gboolean +do_send_data_list (GstBufferList * buffer_list, guint8 channel, + GstRTSPStreamContext * context) Same comment as the do_send_list in rtsp-client
Hrmph, very sorry for the spam, bugzilla was unresponsive on publish and I clicked quite a few times :(
Created attachment 373907 [details] [review] rtsp-server: Add support for buffer lists This adds new functions for passing buffer lists through the different layers without breaking API/ABI, and enables the appsink to actually provide buffer lists. This should already reduce CPU usage and potentially context switches a bit by passing a whole buffer list from the appsink instead of individual buffers. As a next step it would be necessary to a) Add support for a vector of data for the GstRTSPMessage body b) Add support for sending multiple messages at once to the GstRTSPWatch and let it be handled internally c) Adding API to GOutputStream that works like writev()
Created attachment 373908 [details] [review] rtsp-client: Add support for sending buffer lists directly
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-rtsp-server/issues/29.