After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740683 - rtspsrc: add retransmission handling for rtp
rtspsrc: add retransmission handling for rtp
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-11-25 13:24 UTC by Matthew Waters (ystreet00)
Modified: 2014-12-16 15:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtspsrc: add retransmision support (7.01 KB, patch)
2014-11-25 13:26 UTC, Matthew Waters (ystreet00)
none Details | Review
rtspsrc: add retransmision support (7.19 KB, patch)
2014-12-01 12:50 UTC, Matthew Waters (ystreet00)
committed Details | Review

Description Matthew Waters (ystreet00) 2014-11-25 13:24:01 UTC
see summary
Comment 1 Matthew Waters (ystreet00) 2014-11-25 13:26:14 UTC
Created attachment 291452 [details] [review]
rtspsrc: add retransmision support

Inspiration from the tests/examples/rtp/client-rtpaux.c example.
Comment 2 Olivier Crête 2014-11-26 21:57:41 UTC
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)
Comment 3 Matthew Waters (ystreet00) 2014-11-27 00:26:24 UTC
(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.
Comment 4 Olivier Crête 2014-11-29 22:07:28 UTC
(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.
Comment 5 Tim-Philipp Müller 2014-11-30 14:56:16 UTC
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?
Comment 6 Sebastian Dröge (slomo) 2014-12-01 08:39:01 UTC
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
Comment 7 Matthew Waters (ystreet00) 2014-12-01 12:50:09 UTC
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.
Comment 8 Sebastian Dröge (slomo) 2014-12-14 11:36:45 UTC
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
Comment 9 Sebastian Dröge (slomo) 2014-12-16 15:40:24 UTC
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