[Bug 770934] rtsp: Make RTSP TIMEOUT in RTP/RTSP/TCP transport deterministic

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=770934

Tim-Philipp Müller <t.i.m at zen.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #344847|none                        |reviewed
             status|                            |

--- Comment #30 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 344847
  --> https://bugzilla.gnome.org/attachment.cgi?id=344847
New solution rtsp server

>From 4ffbd89b7451efb40e6c8b583dd5af6bbc55f636 Mon Sep 17 00:00:00 2001
>From: Dag Gullberg <dagg at axis.com>
>Date: Fri, 3 Feb 2017 13:05:22 +0100
>Subject: [PATCH 1/1] RTSP TIMEOUT in RTP/RTSP/TCP transport not deterministic
>
>In using TCP as transport, the resetting of the RTSP session
>timeout is handled via call to do_keepalive() in
>gst_rtsp_stream_transport_send_rtp(). The do_keep_alive function
>does only reset timestamps for the timeout timer.
>
>When write_bytes() in gstrtspconnection.c ends up in
>RESOURCE_TEMPORARILY_UNAVAILABLE - typically caused by the socket
>buffer being full - while doing its non-blocking writes, it starts
>to fill the BACKLOG queue. At this point it would be good to have
>the TIMEOUT started, and not reset until conditions are OK.
>
>Currently, the TIMEOUT starts when the BACKLOG is FULL, which may
>add seconds to the actual TIMEOUT.
>
>The solution is to use the watch to propagate the true value of
>the write operaion. A connloss flag is defined and propagated via
>callbacks to the transport layer.

Why are BACKLOG and TIMEOUT capitalised here? :)

It sounds like you're saying we want to start the timeout when the backlog
queue (in GstRTSPConnection) is starting to fill up, but I was under the
impression the whole point of this patchset was to start the timeout when the
socket buffer starts building up beyond some normal limit (CA_LOSS), i.e. even
earlier ?



>+#include <netinet/tcp.h>
>+#include <gio/gnetworking.h>

I think these includes belong into the other patch that does the tcp socket
stuff, and first one should probably be guarded by G_OS_UNIX? Is the first
needed given the second? Do we need to bump glib req for the second?


>@@ -90,6 +92,8 @@ struct _GstRTSPClientPrivate
>+  gboolean connloss;
>+  gboolean tcp_connloss_detect;

Please add a comment for each boolean describing what they represent, one is
the property, the other one is 'TRUE if we have detected a potential loss of
the connection' or somesuch.


>+  g_object_class_install_property (gobject_class, PROP_TCP_CONNLOSS_DETECT,
>+      g_param_spec_boolean ("tcp-connloss-detect", "TCP Connection Loss Detect",
>+          "Early detection of connection loss to exactly time the RTSP timeout",
>+          DEFAULT_TCP_CONNLOSS_DETECT,
>+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Should we mention that it might cost some performance?

There should be a gtk-doc blurb for the property as well, that copies the
description and also includes a 'Since: 1.14' marker (check
gstreamer/gst/gstpipeline.c for example).




>@@ -729,6 +740,9 @@ gst_rtsp_client_get_property (GObject * object, guint propid,
>+    case PROP_TCP_CONNLOSS_DETECT:
>+      g_value_set_boolean (value, priv->tcp_connloss_detect);
>+      break;

Why no locking here, but locking in the setter below? Should both take lock
then imho.



>+static void
>+connloss_status (GstRTSPWatch * watch, GstRTSPResult result,
>+    GSocket * write_socket, gpointer user_data)
>+{
>+  GstRTSPClient *client = GST_RTSP_CLIENT (user_data);
>+  GstRTSPClientPrivate *priv = client->priv;
>+
>+  priv->connloss = (result == GST_RTSP_EINTR);
>+
>+  GST_LOG ("client %p: connloss_status %s", client,
>+      (priv->connloss ? "CONNECTION LOST" : "OK"));
>+}

That's quite a strong message ("connection lost") - is it really accurate here?
(Also taking into account the follow-up patch that checks the socket). It might
just indicate a potential problem, could be bandwidth too low or a short-lived
networking issue or something, no?

> static gboolean
> handle_tunnel (GstRTSPClient * client)
> {
>@@ -4279,7 +4325,8 @@ static GstRTSPWatchFuncs watch_funcs = {
>   tunnel_post,
>   error_full,
>   tunnel_lost,
>-  tunnel_http_response
>+  tunnel_http_response,
>+  connloss_status
> };
> 
> static void
>@@ -4330,6 +4377,7 @@ gst_rtsp_client_attach (GstRTSPClient * client, GMainContext * context)
>   /* create watch for the connection and attach */
>   priv->watch = gst_rtsp_watch_new (priv->connection, &watch_funcs,
>       g_object_ref (client), (GDestroyNotify) client_watch_notify);
>+  gst_rtsp_watch_set_connloss_detect (priv->watch, priv->tcp_connloss_detect);
>   gst_rtsp_client_set_send_func (client, do_send_message, priv->watch,
>       (GDestroyNotify) gst_rtsp_watch_unref);


>--- a/gst/rtsp-server/rtsp-stream-transport.c
>+++ b/gst/rtsp-server/rtsp-stream-transport.c
>@@ -52,6 +52,7 @@ struct _GstRTSPStreamTransportPrivate
> 
>   GstRTSPSendFunc send_rtp;
>   GstRTSPSendFunc send_rtcp;
>+  GstRTSPSendConnLossFunc send_connloss;
>   gpointer user_data;
>   GDestroyNotify notify;

See comment about name below please.



>+/**
>+ * gst_rtsp_stream_transport_set_callbacks:
>+ * @trans: a #GstRTSPStreamTransport
>+ * @send_connloss: a callback called to check lates low level connloss status
>+ *
>+ * 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.
>+ */

Wrong name -> gst_rtsp_stream_transport_set_connloss_callback

Typo: -> "the latest"

Add 'Since: 1.14' here please.


>+void
>+gst_rtsp_stream_transport_set_connloss_callback (GstRTSPStreamTransport * trans,
>+    GstRTSPSendConnLossFunc send_connloss)
>+{
>+  g_return_if_fail (GST_IS_RTSP_STREAM_TRANSPORT (trans));
>+  trans->priv->send_connloss = send_connloss;
>+}

Why is this called *send*_conloss? I find this confusing seeing what
send_rtp/send_rtcp do. We're not sending anything here, we're checking
something or getting some status, no?



>@@ -460,7 +491,7 @@ gst_rtsp_stream_transport_send_rtp (GstRTSPStreamTransport * trans,
>
>-  if (res)
>+  if (res && !gst_rtsp_stream_low_level_connloss (trans))
>     gst_rtsp_stream_transport_keep_alive (trans);
> 
>@@ -489,7 +520,7 @@ gst_rtsp_stream_transport_send_rtcp (GstRTSPStreamTransport * trans,
> 
>-  if (res)
>+  if (res && !gst_rtsp_stream_low_level_connloss (trans))
>     gst_rtsp_stream_transport_keep_alive (trans);

Would be nice to have a short comment here (something like that from the patch
in the other bug), e.g. "Reset timeout whenever we managed to successfully send
some data, but don't reset it if we have a send backlog building up in the tcp
stack, which might indicate a problem."



>--- a/gst/rtsp-server/rtsp-stream-transport.h
>+++ b/gst/rtsp-server/rtsp-stream-transport.h

> /**
>+ * GstRTSPSendConnLoss:

Which one is it - GstRTSPSendConnLossFunc or GstRTSPSendConnLoss ? :)

>+ * @user_data: user data
>+ *
>+ * Function registered with gst_rtsp_stream_transport_set_callbacks() and called
>+ * when data is sent to check low level connection loss.
>+ */
>+typedef gboolean (*GstRTSPSendConnLossFunc) (gpointer user_data);

Please add a 'Since: 1.14' marker to gtk-doc chunk, and document what the
boolean return value indicates.


>+void                     gst_rtsp_stream_transport_set_connloss_callback (GstRTSPStreamTransport *trans,
>+                                                                  GstRTSPSendConnLossFunc send_connloss);

Needs to be updated for git master (needs a GST_EXPORT decorator in front now).
Please also run make update-exports so it gets added to the .def file.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.