[Bug 788340] Dynamically reconfigure pipeline in PLAY based on transports

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #360667|none                        |needs-work
             status|                            |

--- Comment #2 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 360667:
 --> (https://bugzilla.gnome.org/review?bug=788340&attachment=360667)

Why are you adding the receiver/sender parts after PLAY and not during SETUP?
SETUP is basically when the client tells the server to set up these pieces.
In PLAY/RECORD you would only finish the setup and consider the session
complete


Also all this seems like an ABI change at least, behaviour is different now
(see e.g. the stream.c test changes) and other users of the rtsp infrastructure
will need updating. rtspclientsink probably needs updating for example.
We should probably increase the soname from 0 to 1 here.

::: gst/rtsp-server/rtsp-media.c
@@ -595,2 @@
   if (gst_rtsp_stream_query_position (stream, &tmp)) {
-    data->position = MAX (data->position, tmp);

The part about the position reporting could probably be a separate commit

@@ +611,3 @@
   priv = media->priv;

+  data.position = G_MAXINT64;

Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing
else can be found

@@ +2181,3 @@
+
+      if (gst_rtsp_stream_is_complete (stream)) {
+        complete = TRUE;

Shouldn't it only be considered complete once *all* streams are complete?

Also this part with the seeking looks like a separate, independent change that
could be a separate commit

@@ +2711,3 @@
+
+  /* start blocked since it is possible that there are no sink elements yet */
+  media_streams_set_blocked (media, TRUE);

Previously this was only done for non-RECORD streams. It seems problematic to
block and wait for RECORD streams as they would only receive packets once the
other side got the RECORD response.

@@ +3655,2 @@
   switch (priv->suspend_mode) {
     case GST_RTSP_SUSPEND_MODE_NONE:

Why go through blocking and preparing state here, instead of directly going to
prepared as before? This seems to change behaviour, which might be unexpected.
And could also be a separate change probably

::: gst/rtsp-server/rtsp-session-media.c
@@ +415,3 @@
+ *
+ * Returns: (transfer none): a list of #GstRTSPStreamTransport that is valid
+ * until the session of @media is unreffed.

This is not thread-safe: the transport could be changed at any time after they
were returned to the caller in theory. You need to return a full copy of the
list

::: gst/rtsp-server/rtsp-stream.c
@@ +311,3 @@
+    g_object_unref (priv->mcast_socket_v6[0]);
+  if (priv->mcast_socket_v6[1])
+    g_object_unref (priv->mcast_socket_v6[1]);

By using a little for loop, this can be half the number of lines ;)
Why was this not necessary before, what exactly changed here?

@@ +1260,3 @@
 static gboolean
+create_and_configure_udpsources (GstElement ** udpsrc,
+    GSocket * socket)

It's only configuring 1 udpsource now

@@ +1381,3 @@
+      g_clear_object (&inetaddr);
+      if (multicast)
+        inetaddr = g_inet_address_new_any (family);

The problem with allocating ports ourselves is that the IP_MULTICAST_ALL and
control message stuff from udpsrc won't work properly, or is it still doing
that here?

@@ +2678,3 @@
+    g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL);
+  if (priv->appsink[1])
+    g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL);

Why don't we do this right after creation of the sinks?
Why for both indizes?

@@ +2722,3 @@
+
+static void
+plug_mcast_sink (GstRTSPStream * stream, guint index)

plug_udp_sink() and plug_mcast_sink() look very similar. Maybe code can be
shared?

@@ +2734,3 @@
+    g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL);
+  if (priv->appsink[1])
+    g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL);

Why for both indizes?

@@ +2796,3 @@
+      g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL);
+    if (priv->appsink[1])
+      g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL);

Why for both indizes?

@@ +2826,3 @@
+    /* when its only TCP, we need to set sync and preroll to FALSE
+     * for the sink to avoid deadlock. And this is only needed for
+     * sink used for RTCP data, not the RTP data. */

Elsewhere you do it for all appsinks

@@ +2903,3 @@
+     * | rtpbin |      | tee |    |  sink   |
+     * |       send->sink   src->sink       |
+     * '--------'      '-----'    '---------'

Isn't the tee also only added when needed below?

@@ +3012,3 @@
+  tcp = transport->lower_transport == GST_RTSP_LOWER_TRANS_TCP;
+  udp = transport->lower_transport == GST_RTSP_LOWER_TRANS_UDP;
+  mcast = transport->lower_transport == GST_RTSP_LOWER_TRANS_UDP_MCAST;

Maybe with is_ prefix for these kind of flags

@@ +4255,3 @@
   priv->blocking = TRUE;
+
+  if (info->type & GST_PAD_PROBE_TYPE_BUFFER)

Add some additional parenthesis here, some compilers warn about this otherwise.
(i.e. do "if ((a & b))")

@@ +4257,3 @@
+  if (info->type & GST_PAD_PROBE_TYPE_BUFFER)
+    buffer = gst_pad_probe_info_get_buffer (info);
+  else if (info->type & GST_PAD_PROBE_TYPE_BUFFER_LIST) {

Add some {} above

@@ +4261,3 @@
+    buffer = gst_buffer_list_get (list, 0);
+  } else
+    g_assert_not_reached ();

... and a } here

@@ +4351,3 @@
+  g_mutex_lock (&priv->lock);
+  if (priv->send_src[0] && gst_pad_is_linked (priv->send_src[0]))
+    set_blocked (stream, FALSE);

Why is it important that it is linked? and when is it important?

@@ +4444,3 @@
+    }
+    gst_event_parse_segment (event, &segment);
+    gst_event_parse_segment (event, &segment);

Double line

@@ +4447,3 @@
+    if (segment->format != GST_FORMAT_TIME)
+      *position = -1;
+    else {

Some {} above

@@ +4498,3 @@
+  else if (priv->send_src[0])
+    pad = gst_object_ref (priv->send_src[0]);
+  else {

{} above

@@ +4578,3 @@
+    goto create_receiver_error;
+
+  /* in the RECORD case, we only create the receiver part */

And the sender part (RTCP) is added where? :) Just add that to the comment here

@@ +4604,3 @@
+ * @stream: a #GstRTSPStream
+ *
+ * Checks whether the stream is complete, contains at least one sink element.

Why does that make it complete? Was does complete mean?

Would it be different for RECORD, probably not?

::: tests/check/gst/client.c
@@ +731,3 @@
+ * The client transport settings is allowed and the requested destination
+ * is allocated by the server regardless if the requested address is present
in
+ * the configured address pool or not. */

Why is this correct and why was the previous behaviour (to reject this) wrong?
You change tests from previously testing failure to testing for success here

::: tests/check/gst/media.c
@@ +207,3 @@
   g_object_set (G_OBJECT (media), "reusable", TRUE, NULL);

+  pool = gst_rtsp_thread_pool_new ();

A thread pool shouldn't really be needed, the server will create one. Why is it
now needed?

@@ +272,3 @@
   g_object_unref (factory);
+if (0) {
+}

Why?

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