[Bug 771525] gst-rtsp-server: poor performance while streaming RTP over RTSP
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Tue Aug 1 13:03:20 UTC 2017
https://bugzilla.gnome.org/show_bug.cgi?id=771525
--- Comment #14 from anila <anila.balavan at axis.com> ---
(In reply to Olivier CrĂȘte from comment #12)
> 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 ?
>
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.
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
More information about the gstreamer-bugs
mailing list