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 711560 - rtpsession: ssrc collision improvements
rtpsession: ssrc collision improvements
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
http://cgit.collabora.com/git/user/gk...
Depends on: 711084
Blocks:
 
 
Reported: 2013-11-06 17:40 UTC by George Kiagiadakis
Modified: 2013-12-12 14:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstrtpsession: send custom upstream event "GstRTPCollision" on send_rtp_sink pad (1.57 KB, patch)
2013-11-06 17:40 UTC, George Kiagiadakis
committed Details | Review
tests/check: add rtpcollision::test_master_ssrc_collision unit test (11.08 KB, patch)
2013-11-06 17:41 UTC, George Kiagiadakis
none Details | Review
tests/check: add rtpcollision::test_rtx_ssrc_collision unit test (7.13 KB, patch)
2013-11-06 17:42 UTC, George Kiagiadakis
none Details | Review
rtpsession: do not call "rtp_session_schedule_bye_locked" on collision (899 bytes, patch)
2013-11-06 17:42 UTC, George Kiagiadakis
needs-work Details | Review
tests/check: improve rtpcollision::test_master_ssrc_collision to ensure that a collision does not BYE the whole session (2.82 KB, patch)
2013-11-06 17:42 UTC, George Kiagiadakis
none Details | Review
doc: add design-rtpcollision.txt that explains when GstRTPCollision is created (2.76 KB, patch)
2013-11-06 17:43 UTC, George Kiagiadakis
none Details | Review
gstrtpbasepayload: regenerate ssrc when receiving GstRTPCollision event (4.61 KB, patch)
2013-11-06 17:57 UTC, George Kiagiadakis
needs-work Details | Review
gstrtpbasepayload: regenerate ssrc when receiving GstRTPCollision event (4.91 KB, patch)
2013-11-12 08:39 UTC, George Kiagiadakis
needs-work Details | Review
rtpsession: do not stop completely when a source schedules to send a BYE (3.19 KB, patch)
2013-11-18 12:36 UTC, George Kiagiadakis
needs-work Details | Review
tests/check: add rtpcollision::test_master_ssrc_collision unit test (11.09 KB, patch)
2013-11-20 12:42 UTC, George Kiagiadakis
none Details | Review
rtpsession: allow individual sources to send BYE without disrupting the rest of the session (9.96 KB, patch)
2013-11-29 14:48 UTC, George Kiagiadakis
none Details | Review

Description George Kiagiadakis 2013-11-06 17:40:01 UTC
Here are some patches to enable rtpsession to work well with ssrc-multiplexed streams. The session will now send an upstream event when there is an ssrc collision and the payloader (or rtprtxsend) will choose another ssrc. In addition, the session will only send RTCP BYE to the collided stream and will not shut down completely.
Comment 1 George Kiagiadakis 2013-11-06 17:40:32 UTC
Created attachment 259125 [details] [review]
gstrtpsession: send custom upstream event "GstRTPCollision" on send_rtp_sink pad
Comment 2 George Kiagiadakis 2013-11-06 17:41:09 UTC
Created attachment 259126 [details] [review]
tests/check: add rtpcollision::test_master_ssrc_collision unit test
Comment 3 George Kiagiadakis 2013-11-06 17:42:01 UTC
Created attachment 259127 [details] [review]
tests/check: add rtpcollision::test_rtx_ssrc_collision unit test
Comment 4 George Kiagiadakis 2013-11-06 17:42:27 UTC
Created attachment 259128 [details] [review]
rtpsession: do not call "rtp_session_schedule_bye_locked" on collision
Comment 5 George Kiagiadakis 2013-11-06 17:42:52 UTC
Created attachment 259129 [details] [review]
tests/check: improve rtpcollision::test_master_ssrc_collision to ensure that a collision does not BYE the whole session
Comment 6 George Kiagiadakis 2013-11-06 17:43:31 UTC
Created attachment 259130 [details] [review]
doc: add design-rtpcollision.txt that explains when GstRTPCollision is created
Comment 7 George Kiagiadakis 2013-11-06 17:57:41 UTC
Created attachment 259131 [details] [review]
gstrtpbasepayload: regenerate ssrc when receiving  GstRTPCollision event

Note that this patch is against gst-plugins-base
Comment 8 Wim Taymans 2013-11-11 14:40:32 UTC
Review of attachment 259128 [details] [review]:

That does not seem right. When a collision is detected, a BYE must be scheduled, so this function must be
called. IIRC the session does not stop completely, only the sources that are marked BYE.
Comment 9 Wim Taymans 2013-11-11 15:13:09 UTC
Review of attachment 259131 [details] [review]:

This does not seem right:

 - you call gst_rtp_base_payload_set_outcaps() with NULL, this removes the previously configured
   properties by the subclass.
 - gst_rtp_base_payload_set_outcaps() negotiates the SSRC with downstream, which may or may not have
   changed.

What I think should happen is that it changes the SSRC and then updates the current caps with
the new value.
Comment 10 George Kiagiadakis 2013-11-12 08:39:04 UTC
Created attachment 259641 [details] [review]
gstrtpbasepayload: regenerate ssrc when receiving GstRTPCollision event
Comment 11 George Kiagiadakis 2013-11-12 10:36:57 UTC
(In reply to comment #8)
> Review of attachment 259128 [details] [review]:
> 
> That does not seem right. When a collision is detected, a BYE must be
> scheduled, so this function must be
> called. IIRC the session does not stop completely, only the sources that are
> marked BYE.

Looks like you are right. The real problem is that when BYE is scheduled, sess->scheduled_bye is set to TRUE, which then prevents the session from working further. Now if we remove the scheduled_bye variable, I think it would work, but then why is it there in the first place? :S
Comment 12 Julien Isorce 2013-11-12 10:50:28 UTC
(In reply to comment #9)
> Review of attachment 259131 [details] [review]:
> 
> This does not seem right:
> 
>  - you call gst_rtp_base_payload_set_outcaps() with NULL, this removes the
> previously configured
>    properties by the subclass.
>  - gst_rtp_base_payload_set_outcaps() negotiates the SSRC with downstream,
> which may or may not have
>    changed.
> 
> What I think should happen is that it changes the SSRC and then updates the
> current caps with
> the new value.

but rtp_base_payload has to get the new ssrc generated form the rtpsession (see rtpsession:: suggested ssrc) so at some point the rtpbasepayloader has to qurery caps and this is done in gst_rtp_base_payload_set_outcaps.
Comment 13 Wim Taymans 2013-11-14 08:49:52 UTC
(In reply to comment #12)
> 
> but rtp_base_payload has to get the new ssrc generated form the rtpsession (see
> rtpsession:: suggested ssrc) so at some point the rtpbasepayloader has to
> qurery caps and this is done in gst_rtp_base_payload_set_outcaps.

The session suggesting an SSRC on the sinkpad caps is a backward compatibility thing but not something that can work for multiple senders because it can only suggest 1 SSRC.

You would have the change the base class to generate the SSRC and let the session take whatever it receives as input.
Comment 14 George Kiagiadakis 2013-11-18 12:36:36 UTC
Created attachment 260106 [details] [review]
rtpsession: do not stop completely when a source schedules to send a BYE

This is an attempt to fix BYE scheduling. I am not totally sure why the code previously ignored incoming RTCP packets except BYE when a sess->scheduled_bye was TRUE internally and I am also not totally sure what is the purpose of sess->allow_early = TRUE, because it seems it is set to TRUE in rtp_session_init() and never set to FALSE, so it's always TRUE.
Comment 15 Wim Taymans 2013-11-19 10:52:40 UTC
(In reply to comment #14)
> Created an attachment (id=260106) [details] [review]
> rtpsession: do not stop completely when a source schedules to send a BYE
> 
> This is an attempt to fix BYE scheduling. I am not totally sure why the code
> previously ignored incoming RTCP packets except BYE when a sess->scheduled_bye
> was TRUE internally and I am also not totally sure what is the purpose of

RFC 3550 section 6.3.7 describes this rule for sending a BYE packet and updating the average packet size.

> sess->allow_early = TRUE, because it seems it is set to TRUE in
> rtp_session_init() and never set to FALSE, so it's always TRUE.

Yes, it's not configurable yet. Early packet can be suppressed with a signal only for now.
Comment 16 George Kiagiadakis 2013-11-20 12:42:59 UTC
Created attachment 260303 [details] [review]
tests/check: add rtpcollision::test_master_ssrc_collision unit test
Comment 17 Wim Taymans 2013-11-22 14:52:52 UTC
This patch removes logic required by the spec.
Comment 18 Wim Taymans 2013-11-22 15:03:51 UTC
Comment on attachment 260106 [details] [review]
rtpsession: do not stop completely when a source schedules to send a BYE

>diff --git a/gst/rtpmanager/rtpsession.c b/gst/rtpmanager/rtpsession.c
>index 49baeaa..b01f186 100644
>--- a/gst/rtpmanager/rtpsession.c
>+++ b/gst/rtpmanager/rtpsession.c
>@@ -2480,12 +2480,6 @@ rtp_session_process_rtcp (RTPSession * sess, GstBuffer * buffer,
> 
>     type = gst_rtcp_packet_get_type (&packet);
> 
>-    /* when we are leaving the session, we should ignore all non-BYE messages */
>-    if (sess->scheduled_bye && type != GST_RTCP_TYPE_BYE) {
>-      GST_DEBUG ("ignoring non-BYE RTCP packet because we are leaving");
>-      goto next;
>-    }
>-

When a source is sending BYE, all RTCP that is not BYE should be ignored (for that
source only). 6.3.7 says that no other RTCP packets can influence the average
packet size calculation and the member database.

>-  /* if we are scheduling a BYE, we only want to count bye packets, else we
>-   * count everything */
>-  if (sess->scheduled_bye) {
>-    if (is_bye) {
>-      sess->stats.bye_members++;
>-      UPDATE_AVG (sess->stats.avg_rtcp_packet_size, pinfo.bytes);
>-    }
>-  } else {
>-    /* keep track of average packet size */
>-    UPDATE_AVG (sess->stats.avg_rtcp_packet_size, pinfo.bytes);
>-  }
>+  if (is_bye)
>+    sess->stats.bye_members++;
>+  /* keep track of average packet size */
>+  UPDATE_AVG (sess->stats.avg_rtcp_packet_size, pinfo.bytes);

Not correct, can only update the size for BYE packets when sending BYE.

> 
>-  /* nothing to do it we already scheduled bye */
>-  if (sess->scheduled_bye)
>-    goto done;
>-
>   /* we schedule BYE now */
>   sess->scheduled_bye = TRUE;
>-  /* at least one member wants to send a BYE */
>-  INIT_AVG (sess->stats.avg_rtcp_packet_size, 100);
>-  sess->stats.bye_members = 1;
>-  sess->first_rtcp = TRUE;
>+
>+  sess->stats.bye_members++;
>   sess->allow_early = TRUE;

Not correct, 6.3.7, first_rtcp needs to be set to schedule this packet
as if it was the first packet.

I'm starting to think that many of these stats should not be on the session
but on the source so that when one source is sending BYE, it can keep
it's own stats for doing so.
Comment 19 George Kiagiadakis 2013-11-26 10:17:53 UTC
(In reply to comment #18)
> I'm starting to think that many of these stats should not be on the session
> but on the source so that when one source is sending BYE, it can keep
> it's own stats for doing so.

True, but it's not only the stats that are the problem. The RTCP timeout is also dependent on those stats and it affects all the sources. We would actually need to have the timeout logic moved into RTPSource as well and I'm not exactly sure where is the boundary.
Comment 20 George Kiagiadakis 2013-11-29 14:48:02 UTC
Created attachment 263126 [details] [review]
rtpsession: allow individual sources to send BYE without disrupting the rest of the session

This is another attempt to fix rtpsession. It's a bit hacky, but it should be good enough. Instead of moving (and duplicating) all these stats to every RTPSource, it keeps two sets of stats in rtpsession, stats and bye_stats. When a BYE is scheduled, bye_stats are used to caclulate the next timeout and only BYE packets are allowed to be sent. In addition, during this time, non-BYE packets for RTPSources that are marked BYE are ignored. For other RTPSources, though, they are processed as normal. Finally, once the BYE packets are sent, everything returns to normal.
Comment 21 Olivier Crête 2013-12-12 00:39:36 UTC
I'm not sure why there is a special RTPCollision event instead of just using the RECONFIGURE event ?
Comment 22 Olivier Crête 2013-12-12 00:49:01 UTC
Review of attachment 259641 [details] [review]:

You must do a downstream caps query first, to try to get the new SSRC from the rtp session (otherwise the RTP session will have to override it).

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +442,3 @@
+
+        if (!gst_structure_get_uint (s, "ssrc", &ssrc))
+          ssrc = -1;

ssrc is a guint, so you can't make it -1. You need to add another boolean variable like ssrc_exists

@@ +492,3 @@
+  if (rtpbasepayload_class->src_event)
+    res = rtpbasepayload_class->src_event (rtpbasepayload, event);
+  else

You have a default handler, so this should never be NULL.

@@ +964,3 @@
 
+  if (g_atomic_int_compare_and_exchange (&payload->
+          priv->notified_first_timestamp, 1, 0)) {

Separate patch.
Comment 23 Olivier Crête 2013-12-12 00:54:05 UTC
Possibly, the custom rtpcollision event should only be used for SSRCs which are not the primary SSRC of the session (the one in the caps), and then payloaders can completely ignore it (and just use reconfigure), only AUX senders will care.
Comment 24 Wim Taymans 2013-12-12 14:19:00 UTC
Should all be commited now

commit b32fc6f41629237a0c98435bbb9d3c00a167851f
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Fri Nov 1 17:04:28 2013 +0000

    gstrtpsession: send custom upstream event "GstRTPCollision" on send_rtp_sink pad
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=711560


commit 71788c143296fc681dfdab95d138022b3fbb735b
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Thu Dec 12 13:42:59 2013 +0100

    rtpbasepayload: change SSRC on GstRTPCollision event
    
    Change our SSRC and update the caps when we receive a GstRTPCollision
    event from downstream.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=711560