[Bug 771525] gst-rtsp-server: poor performance while streaming RTP over RTSP

[GStreamer (GNOME Bugzilla)]

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.