Created attachment 259125 [details] [review]
gstrtpsession: send custom upstream event "GstRTPCollision" on send_rtp_sink pad

Created attachment 259126 [details] [review]
tests/check: add rtpcollision::test_master_ssrc_collision unit test

Created attachment 259127 [details] [review]
tests/check: add rtpcollision::test_rtx_ssrc_collision unit test

Created attachment 259128 [details] [review]
rtpsession: do not call "rtp_session_schedule_bye_locked" on collision

Created attachment 259129 [details] [review]
tests/check: improve rtpcollision::test_master_ssrc_collision to ensure that a collision does not BYE the whole session

Created attachment 259130 [details] [review]
doc: add design-rtpcollision.txt that explains when GstRTPCollision is created

Created attachment 259131 [details] [review]
gstrtpbasepayload: regenerate ssrc when receiving  GstRTPCollision event

Note that this patch is against gst-plugins-base

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.

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.

Created attachment 259641 [details] [review]
gstrtpbasepayload: regenerate ssrc when receiving GstRTPCollision event

(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

(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.

(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.

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.

(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.

Created attachment 260303 [details] [review]
tests/check: add rtpcollision::test_master_ssrc_collision unit test

This patch removes logic required by the spec.

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.

(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.

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.

I'm not sure why there is a special RTPCollision event instead of just using the RECONFIGURE event ?

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.

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.

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