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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #29 from Patricia Muscalu <patricia at axis.com> ---
(In reply to Sebastian Dröge (slomo) from comment #2)
> @@ +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.

Now, the RECORD and LIVE streams are treated in the same way, meaning that
media streams are blocked in DESCRIBE and unblocked when PLAY comes.

> @@ +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

Added a comment in the code. This change is logically depended on the whole
dynamic-pipeline change so it's difficult to provide a separate change for it. 

> ::: 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 ;)

Done.

> Why was this not necessary before, what exactly changed here?

It's a new socket attribute.

> @@ +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?

Added a FIXME note.

> 
> @@ +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?

Fixed: stream-set-async-sync-false-only-for-RTCP-appsink.patch
> 
> @@ +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?

Fixed, however I think that the code is less readable after this change.

> @@ +2903,3 @@
> +     * | rtpbin |      | tee |    |  sink   |
> +     * |       send->sink   src->sink       |
> +     * '--------'      '-----'    '---------'
> 
> Isn't the tee also only added when needed below?

The tee element is always present. The code is less complected that way. 

> @@ +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?

It covers the case when not all media streams are active. 

> @@ +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?

Added a more descriptive information. 
> 
> Would it be different for RECORD, probably not?

Not really, removed an unnecessary if statement . 

> 
> ::: 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

Reverted this change. If we need more changes a new bug will be filed as this
change is not really relevant to the current problem.

> 
> ::: 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?

The thread pool is actually created in the test code at line 173 (master
version). The pool is created by the server, yes, but we do not have any access
to the server instance from the server code. I realize now that this change can
provided in a separate patch.

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