Created attachment 168963 [details] [review]
enable rtspsrc to handle the SDP protocol

Created attachment 168964 [details] [review]
Bring the SDP protocol into exhistence

Created attachment 168965 [details] [review]
use rtspsrc as session element is sdpdemux when needed

Created attachment 168991 [details] [review]
use rtspsrc as session element is sdpdemux when needed

Improved the number of streams handling.

> Still the key issue of state change violations in sdpdemux are remaining. 

What do you mean with that?

Review of attachment 168963 [details] [review]:

This patch needs some work

Comment on attachment 168963 [details] [review]
enable rtspsrc to handle the SDP protocol 

> static gchar **
> gst_rtspsrc_uri_get_protocols (void)
> {
>-  static const gchar *protocols[] = { "rtsp", "rtspu", "rtspt", "rtsph", NULL };
>+  static const gchar *protocols[] =
>+      { "sdp", "rtsp", "rtspu", "rtspt", "rtsph", NULL };
> 
>   return (gchar **) protocols;
> }

Please make a new rtsp-sdp:// uri which is less likely to conflict with an existing
uri.

>+static const gchar *
>+gst_rtspsrc_init_from_sdp (GstRTSPSrc * src, const gchar * sdpstr)
>+{
>+  int i, l = strlen (sdpstr);
>+  gchar *string = g_malloc (l + 1);
>+  const gchar *control_url = NULL;
>+  strcpy (string, sdpstr);
>+
>+  for (i = 0; i < l; i++)
>+    if (string[i] == ';' && string[i + 1] > 'a' && string[i + 1] < 'z'
>+        && string[i + 2] == '=')
>+      string[i] = '\n';

Why this cleanup of the SDP? I would pass a base64 encoded version of the raw
SDP in the rtsp-sdp:// uri.

>+
>+  if (src->sdp != NULL) {
>+    GST_ERROR_OBJECT (src, "SDP already existing");
>+  } else {

Setting a new rtsp-sdp:// overrides the previous one.

>+    GST_DEBUG_OBJECT (src, "SDP protocol, extracting SDP data");
>+    if (gst_sdp_message_new (&src->sdp) == GST_SDP_OK &&
>+        gst_sdp_message_parse_buffer ((guchar *) string, l,
>+            src->sdp) == GST_SDP_OK) {
>+      GST_DEBUG_OBJECT (src, "got SDP message:\n%s", string);
>+
>+      for (i = 0;; i++) {
>+        control_url =
>+            gst_sdp_message_get_attribute_val_n (src->sdp, "control", i);
>+        if (control_url == NULL)
>+          break;
>+
>+        /* only take fully qualified urls */
>+        if (g_str_has_prefix (control_url, "rtsp://"))
>+          break;
>+      }
>+    } else {
>+      GST_ERROR_OBJECT (src, "unable to create SDP message");
>+    }
>+  }

There is no need to parse the control url, this will happen later.

> 
>+  if (g_str_has_prefix (uri, "sdp://")) {
>+    if ((uri = gst_rtspsrc_init_from_sdp (src, uri + 6)) == NULL)
>+      GST_ERROR ("unable to initialize from give SDP data");
>+  }
>+
>   /* try to parse */
>   if ((res = gst_rtsp_url_parse (uri, &newurl)) < 0)
>     goto parse_error;
> 

There is no need to retrieve the control url from the SDP and so there is also
no need to parse the control url.

(In reply to comment #4)
> Created an attachment (id=168991) [details] [review]
> use rtspsrc as session element is sdpdemux when needed
> 
> Improved the number of streams handling.

This should simply use a redirect to an rtsp-sdp:// url.

(In reply to comment #7)

Thank you for the review, my notes below.

> (From update of attachment 168963 [details] [review])
> > static gchar **
> > gst_rtspsrc_uri_get_protocols (void)
> > {
> >-  static const gchar *protocols[] = { "rtsp", "rtspu", "rtspt", "rtsph", NULL };
> >+  static const gchar *protocols[] =
> >+      { "sdp", "rtsp", "rtspu", "rtspt", "rtsph", NULL };
> > 
> >   return (gchar **) protocols;
> > }
> 
> Please make a new rtsp-sdp:// uri which is less likely to conflict with an
> existing
> uri.
> 

Agreed.

> >+static const gchar *
> >+gst_rtspsrc_init_from_sdp (GstRTSPSrc * src, const gchar * sdpstr)
> >+{
> >+  int i, l = strlen (sdpstr);
> >+  gchar *string = g_malloc (l + 1);
> >+  const gchar *control_url = NULL;
> >+  strcpy (string, sdpstr);
> >+
> >+  for (i = 0; i < l; i++)
> >+    if (string[i] == ';' && string[i + 1] > 'a' && string[i + 1] < 'z'
> >+        && string[i + 2] == '=')
> >+      string[i] = '\n';
> 
> Why this cleanup of the SDP? I would pass a base64 encoded version of the raw
> SDP in the rtsp-sdp:// uri.

In sdpdemux the '\n' are converted with ';' . It's useful, for instance, to easily pass the uri from command line. They're then converted back to '\n' to let SDPMessage parse the content properly. It's trivial for me to remove all the mangling/demangling if we so decide.

> 
> >+
> >+  if (src->sdp != NULL) {
> >+    GST_ERROR_OBJECT (src, "SDP already existing");
> >+  } else {
> 
> Setting a new rtsp-sdp:// overrides the previous one.

But it's not clear to me what would happen if we set the SDP content after the streaming has already been initiated. I guess it would be more proper to check the state of the element here.

> 
> >+    GST_DEBUG_OBJECT (src, "SDP protocol, extracting SDP data");
> >+    if (gst_sdp_message_new (&src->sdp) == GST_SDP_OK &&
> >+        gst_sdp_message_parse_buffer ((guchar *) string, l,
> >+            src->sdp) == GST_SDP_OK) {
> >+      GST_DEBUG_OBJECT (src, "got SDP message:\n%s", string);
> >+
> >+      for (i = 0;; i++) {
> >+        control_url =
> >+            gst_sdp_message_get_attribute_val_n (src->sdp, "control", i);
> >+        if (control_url == NULL)
> >+          break;
> >+
> >+        /* only take fully qualified urls */
> >+        if (g_str_has_prefix (control_url, "rtsp://"))
> >+          break;
> >+      }
> >+    } else {
> >+      GST_ERROR_OBJECT (src, "unable to create SDP message");
> >+    }
> >+  }
> 
> There is no need to parse the control url, this will happen later.

If the control url is not set properly the function gst_rtspsrc_open_from_sdp will not find the control information and thus attempt to open a connection on the 'sdp://' uri (which, of course, cannot be handled). After this fails, no connection will be available to propagate further RTSP messages (e.g. SETUP).

> 
> > 
> >+  if (g_str_has_prefix (uri, "sdp://")) {
> >+    if ((uri = gst_rtspsrc_init_from_sdp (src, uri + 6)) == NULL)
> >+      GST_ERROR ("unable to initialize from give SDP data");
> >+  }
> >+
> >   /* try to parse */
> >   if ((res = gst_rtsp_url_parse (uri, &newurl)) < 0)
> >     goto parse_error;
> > 
> 
> There is no need to retrieve the control url from the SDP and so there is also
> no need to parse the control url.

See my comment above.

(In reply to comment #8)
> (In reply to comment #4)
> > Created an attachment (id=168991) [details] [review] [details] [review]
> > use rtspsrc as session element is sdpdemux when needed
> > 
> > Improved the number of streams handling.
> 
> This should simply use a redirect to an rtsp-sdp:// url.

So are you suggesting to:

1) handle rtsp in sdpdemux sending a redirection message
AND
2) handle redirection in uridecodebin?

(In reply to comment #5)
> > Still the key issue of state change violations in sdpdemux are remaining. 
> 
> What do you mean with that?

The 'session' element is added when the pipeline is already in PLAYING state and the set to PAUSED and PLAYING by the sdpdemux (not by the pipeline). The way through this suggested by Mark would be to use redirection, but this would mean to implement the handling of the signal in e.g. uridecodebin if we want to make the operation transparent to the application.

(In reply to comment #9)
> (In reply to comment #7)
> 
> Thank you for the review, my notes below.
> 
> 
> In sdpdemux the '\n' are converted with ';' . It's useful, for instance, to
> easily pass the uri from command line. They're then converted back to '\n' to
> let SDPMessage parse the content properly. It's trivial for me to remove all
> the mangling/demangling if we so decide.

OK, I didn't realize that you want to pass an SDP from the command line like that. You can just as well cut and paste an SDP into a property with \n and all. Also, unfortunately, ; can be part of any string in the SDP (Appendix A, safe) 

> 
> But it's not clear to me what would happen if we set the SDP content after the
> streaming has already been initiated. I guess it would be more proper to check
> the state of the element here.

Yeah, just like any property that doesn't take immediate effect after a certain state change (like the location property) we mark them with the right flags on the paramspec. Checking the state of the element is not needed. 

> > There is no need to parse the control url, this will happen later.
> 
> If the control url is not set properly the function gst_rtspsrc_open_from_sdp
> will not find the control information and thus attempt to open a connection on
> the 'sdp://' uri (which, of course, cannot be handled). After this fails, no
> connection will be available to propagate further RTSP messages (e.g. SETUP).

The second thing that _open_from_sdp does is parse the control url from the SDP.
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtsp/gstrtspsrc.c#n5172

(In reply to comment #12)
> 
> OK, I didn't realize that you want to pass an SDP from the command line like
> that. You can just as well cut and paste an SDP into a property with \n and
> all. Also, unfortunately, ; can be part of any string in the SDP (Appendix A,
> safe) 

I agree with you, fwiw it's more complex to pass from the command-line a string with '\n' through an option than to translate them with ';' and then re-convert back to '\n' with the heuristic in my implementation. To conclude I must admit that, being it an heuristic, it will fail in some possible cases, so I'll go the way you've proposed.

> 
> > 
> > But it's not clear to me what would happen if we set the SDP content after the
> > streaming has already been initiated. I guess it would be more proper to check
> > the state of the element here.
> 
> Yeah, just like any property that doesn't take immediate effect after a certain
> state change (like the location property) we mark them with the right flags on
> the paramspec. Checking the state of the element is not needed. 
> 
> > > There is no need to parse the control url, this will happen later.
> > 
> > If the control url is not set properly the function gst_rtspsrc_open_from_sdp
> > will not find the control information and thus attempt to open a connection on
> > the 'sdp://' uri (which, of course, cannot be handled). After this fails, no
> > connection will be available to propagate further RTSP messages (e.g. SETUP).
> 
> The second thing that _open_from_sdp does is parse the control url from the
> SDP.
> http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/rtsp/gstrtspsrc.c#n5172

Yes, but the problem here is originated from gst_rtspsrc_uri_set_uri which sets the uri containing the sdp:// address in the first stance. After that, gst_rtsp_conninfo_connect attempts to connect to that URI which is, as I already pointed out, quite difficult. The control URI parsing is the "less invasive" approach to address the issue.

Created attachment 169754 [details] [review]
enable rtspsrc to handle the SDP protocol

For the happiness of the command-liners, removed the de-mangling from ';' to '\n' ;).

Modified the protocol to use rtsp-sdp.

Other minor cosmetic improvements.

Created attachment 169755 [details] [review]
Bring the SDP protocol into existence

modified the uri identifier from "sdp://" to "rtsp-sdp://"

Created attachment 169756 [details] [review]
use rtspsrc as session element is sdpdemux when needed

modified the protocol identifier and some minor cosmetic changes.

Created attachment 169936 [details] [review]
sdpdemux: redirect SDP with rtsp control url

FWIW, attached patch issues a plain rtsp:// redirect for some cases that might allow it.  Other cases may still need more "sdp-based" redirecting as discussed above.

commit ae84ae1b36201f997fed6cabd3322fe37c103174
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Sep 10 11:55:26 2010 +0200

    rtspsrc: add rtsp-sdp protocol support
    
    Allow setting an SDP with the rtsp-sdp:// url.
    
    Based on patch from Marco Ballesio.
    
    See #628214

interestingly: http://tools.ietf.org/html/draft-fujikawa-sdp-url-01

OK, the way forward here is to use something resembling that (old) draft. The following patch allows conversion between SDP and an uri:

commit f5cbb6047fec82c72eff9965413ecb3e7c502eda
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Sep 10 17:55:46 2010 +0200

    sdp: add methods to convert between uri and message
    
    Add methods to convert between uri and sdpmessages, loosly based on
    http://tools.ietf.org/html/draft-fujikawa-sdp-url-01
    
    API: GstSDPMessage::gst_sdp_message_parse_uri
    API: GstSDPMessage::gst_sdp_message_as_uri

And finally redirect in sdpdemux with the rtsp-sdp:// url.

commit 3daea4a085d34d5c1bc33fec2f2c04206621b41b
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Fri Sep 10 18:35:27 2010 +0200

    sdpdemux: redirect SDP with an rtsp control URL
    
    When we find an SDP with an rtsp:// url as the global control attribute or when
    all streams have an rtsp:// control attribute, post an redirect message with an
    rtsp-sdp:// url containing the SDP.
    
    Fixes #628214

So is the final decision to rely on the application to handle the redirection? Or is the redirection already handled at uridecodebin level? Or do we need a bug to add the latter?

(In reply to comment #22)
> So is the final decision to rely on the application to handle the redirection?

Applications 'should' already do this now. The problem is that the redirect is not an official GStreamer core message so we can't really officially demand this.
Most notably qtdemux and mmssrc will post redirects on some files that will otherwise just fail.

> Or is the redirection already handled at uridecodebin level? Or do we need a
> bug to add the latter?

These redirects are usually caused by something inside the datastream and are thus emitted from the streaming threads. They however need to be handled from the main thread and that makes it difficult to implement this in uridecodebin.

In this particular case, I would not find it too disgusting to embed the rtspsrc inside sdpdemux (it already embeds the rtpbin and it's just one element and there are no application decisions to be made about bitrates). It just makes things more complicated wrt state changes and errors caused by the embedded elements that it makes me avoid it.

(In reply to comment #23)
> In this particular case, I would not find it too disgusting to embed the
> rtspsrc inside sdpdemux (it already embeds the rtpbin and it's just one element
> and there are no application decisions to be made about bitrates). It just
> makes things more complicated wrt state changes and errors caused by the
> embedded elements that it makes me avoid it.

I agree, so imho the patch:

https://bugzilla.gnome.org/attachment.cgi?id=169756

should follow the traditional implementation in sdpdemux more closely than:

https://bugzilla.gnome.org/attachment.cgi?id=169936
(which I understand you've applied).

moreover the former would not require applications to handle redirections (aka "it's transparent for the application").

I think it may be easy to make the two patches survive together if we want to: we may add a property in sdpdemux in order to make it either sending a redirection signal or handling rtsp contents the "traditional" way depending on the applications' needs.

Wim, loos like you forgot to push:

https://bugzilla.gnome.org/attachment.cgi?id=169755

without it the whole rtsp session in sdpdemux exercise is useless ;)

Ok, after a few I understand it's not because of this missing patch the feature is not working. Interestingly enough, launching:

gst-launch playbin2 uri=file:///media/devpen/dev/multimedia/hacknest/sdptests/sdptest.sdp

leaves the pipeline stuck right before going to playing. Trying to debug it with gdb:

gdb --args gst-launch playbin2 uri=file:///media/devpen/dev/multimedia/hacknest/sdptests/sdptest.sdp

instead works fine. I'd bet we've a race condition. Fortunately, it appears not to happen with my original patches.

Seems to work fine here. Please attach backtrace of when it hangs.

Here it is:

+ Trace 223913

The process has been terminated with a SIGABRT. Unfortunately, it appears the stack is not much indicative though.

The logic of the bug statuses in GStreamer surpasses me. And I got really good evaluations at Computer Science Engineering though :D.

You posted a backtrace of the mainloop (that is waiting for something to happen). You probably want a backtrace of the other threads like with: 

thr apply all bt

Created attachment 171284 [details]
stack trace taken for multiple threads at the deadlock point

Thanks for the hint. Here's the complete stack trace.

Looks like the problem can be reproduced (almost certainly) by adding a 
  sleep (2);
at the top of gst_sdp_demux_start.
That will delay the streaming thread and arrange for the mainloop to have completed state change to PLAYING before other stuff happens.

The other stuff happening is then sdpdemux picking rtspsrc as manager, performing setup and setting it to state PLAYING.  It turns out that this does not make it past PAUSED as follows.  At some point, rtspsrc adds udpsrc, which is live, which makes gst_bin_add_func call upon bin_handle_async_done.
AFAICS, the latter then decides that rtspsrc is not a top level bin and discards the pending PLAYING state, leaving up to parents to do so.  However, those do not particularly care, as they have no pending state either (already reached PLAYING).

I can reproduce it easily without the need of any artificial delay on my development laptop (asus eeepc 701) which is pretty slow. Replacing the last patches on sdpdemux from Wim with mine (attached to bug 630046) the deadlock does not occur anymore. So far I haven't had much time to check why though.

See bug #630046 for additional comment/patch.

Race as discussed above has been settled in bug #630046.