Created attachment 291452 [details] [review]
rtspsrc: add retransmision support

Inspiration from the tests/examples/rtp/client-rtpaux.c example.

Review of attachment 291452 [details] [review]:

Good job, I've been wanting this for a while! I wonder if we can set the retranmission delay on rtpjitterbuffer based on the rtx-time in the SDP, I couldn't find anything obvious.

::: gst/rtsp/gstrtspsrc.c
@@ +198,3 @@
 #define DEFAULT_TLS_VALIDATION_FLAGS     G_TLS_CERTIFICATE_VALIDATE_ALL
 #define DEFAULT_TLS_DATABASE     NULL
+#define DEFAULT_DO_RETRANSMISSION        FALSE

Any reason to default to false?

@@ +3170,3 @@
+  if (g_signal_lookup ("request-aux-receiver",
+          G_OBJECT_TYPE (src->manager)) == 0)
+    return;

rtspsrc and rtpbin are both in gst-plugins-good, we don't support mixing different parts of different versions of the same package.

@@ +3209,3 @@
+  g_object_set (src->manager, "do-retransmission", TRUE, NULL);
+
+  /* enable RFC4588 retransmission handling by setting rtprtxreceive

Would it make sense to skip this if there are no RTX payloads in the SDP ? (and skip the connect too)

(In reply to comment #2)
> Review of attachment 291452 [details] [review]:
> 
> Good job, I've been wanting this for a while! I wonder if we can set the
> retranmission delay on rtpjitterbuffer based on the rtx-time in the SDP, I
> couldn't find anything obvious.

You'd need to get the jitterbuffer for the rtspstream and then get the rtx-time attribute from the ptmapitem of the rtx stream and set that on the jitterbuffer.

> ::: gst/rtsp/gstrtspsrc.c
> @@ +198,3 @@
>  #define DEFAULT_TLS_VALIDATION_FLAGS     G_TLS_CERTIFICATE_VALIDATE_ALL
>  #define DEFAULT_TLS_DATABASE     NULL
> +#define DEFAULT_DO_RETRANSMISSION        FALSE
> 
> Any reason to default to false?

Because we override the request-aux-receiver signal and the application might want to do its own thing with that signal.

> @@ +3170,3 @@
> +  if (g_signal_lookup ("request-aux-receiver",
> +          G_OBJECT_TYPE (src->manager)) == 0)
> +    return;
> 
> rtspsrc and rtpbin are both in gst-plugins-good, we don't support mixing
> different parts of different versions of the same package.

There's also rdt in -ugly that might be used which doesn't support the request-aux-receiver signal.

> @@ +3209,3 @@
> +  g_object_set (src->manager, "do-retransmission", TRUE, NULL);
> +
> +  /* enable RFC4588 retransmission handling by setting rtprtxreceive
> 
> Would it make sense to skip this if there are no RTX payloads in the SDP ? (and
> skip the connect too)

Maybe, although even if there are no rtx streams and do-retransmision=true, the rtxreceive essentially just passes through each rtp buffer.  I'm not sure its worth the complexity to be honest.

(In reply to comment #3)
> (In reply to comment #2)
> > ::: gst/rtsp/gstrtspsrc.c
> > @@ +198,3 @@
> >  #define DEFAULT_TLS_VALIDATION_FLAGS     G_TLS_CERTIFICATE_VALIDATE_ALL
> >  #define DEFAULT_TLS_DATABASE     NULL
> > +#define DEFAULT_DO_RETRANSMISSION        FALSE
> > 
> > Any reason to default to false?
> 
> Because we override the request-aux-receiver signal and the application might
> want to do its own thing with that signal.

Maybe we could use g_signal_connect_after(), so it will use any external override first and the internal one only if no external one is connected?

Making is the detault would be a big advantage for all of the playbin users.

> There's also rdt in -ugly that might be used which doesn't support the
> request-aux-receiver signal.

But that would trigger the "transport->trans != GST_RTSP_TRANS_RTP" on the previous line.

I agree that we should try to make this work out of the box without application intervention if possible at all.

I think it would be reasonable to assume that the application will hook up the request-aux-receiver signal before NULL->READY state change, so if we later find no handlers connected (we can check using the GSignal API if there are handlers), then we just use it?

g_signal_connect_after(), and from there checking if there's an aux element already seems a bit nicer. But both solutions should really be fine for autodetecting this

Created attachment 291886 [details] [review]
rtspsrc: add retransmision support

This one checks to see if there's a connected handler to the request-aux-receiver signal before attempting to add the retransmission elements.  As a result the default state of do-retransmission is now TRUE.

Review of attachment 291886 [details] [review]:

Looks good, just a question. Feel free to push it if that all makes sense :)

::: gst/rtsp/gstrtspsrc.c
@@ +3140,3 @@
+  GST_INFO_OBJECT (src, "creating retransmision receiver for session %u "
+      "with map %" GST_PTR_FORMAT, sessid, stream->rtx_pt_map);
+  bin = gst_bin_new (NULL);

Why a bin with a single element instead of just having that element directly?

@@ +3174,3 @@
+  if (g_signal_handler_find (src->manager, G_SIGNAL_MATCH_ID, signal_id, 0,
+          NULL, NULL, NULL) != 0)
+    return;

Maybe add a GST_DEBUG_OBJECT() here just in case

commit 6b2fc2de8d4adddb394e83b463146655d1d68dd7
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Dec 16 16:37:24 2014 +0100

    rtspsrc: Add something to the debug logs if an RTX AUX element can't be added
    
    ... because the application already has a signal handler set up here.

commit bf0a19bf02ecca13196bedb6c13d416481f75fd7
Author: Matthew Waters <matthew@centricular.com>
Date:   Fri Nov 21 14:13:34 2014 +1100

    rtspsrc: add retransmission support according to RFC4588
    
    Based on the client-rtpaux example