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 720435 - rtpbasepayload: Prefer downstream SSRC after collision if possible
rtpbasepayload: Prefer downstream SSRC after collision if possible
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-13 22:27 UTC by Olivier Crête
Modified: 2015-11-09 19:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtpbasepayload: Prefer downstream SSRC after collision if possible (4.16 KB, patch)
2013-12-13 22:27 UTC, Olivier Crête
reviewed Details | Review
tea (8.63 KB, text/html)
2015-11-09 19:08 UTC, leonidwxxinosov
  Details

Description Olivier Crête 2013-12-13 22:27:15 UTC
The collision handling code added in bug #711560 doesn't give the application
any way to select the SSRC after a collision, by prefering downstream SSRC if possible,
we can allow this to happen.
Comment 1 Olivier Crête 2013-12-13 22:27:16 UTC
Created attachment 264173 [details] [review]
rtpbasepayload: Prefer downstream SSRC after collision if possible

The collision event handling introduced in commit 71788c14 ignored
downstream preference, still prefer it to make it possible for the application
to select the SSRC itself.
Comment 2 Sebastian Dröge (slomo) 2013-12-14 09:54:49 UTC
Review of attachment 264173 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +461,3 @@
+          /* Ignore if we're not currently negotiated */
+          if (!caps)
+            break;

... but there was a collision so it would be good to get a new ssrc?

@@ +473,3 @@
+                " ignoring ssrc collision");
+            res = FALSE;
+            break;

And the same here?

@@ +482,3 @@
+            res = FALSE;
+            gst_caps_unref (peercaps);
+            break;

And in this case you should probably additionally do an GST_ELEMENT_ERROR(). As the caps are empty, dataflow will fail later and posting a more useful error here than in the source might be a good idea
Comment 3 Wim Taymans 2013-12-16 08:00:51 UTC
(In reply to comment #1)
> Created an attachment (id=264173) [details] [review]
> rtpbasepayload: Prefer downstream SSRC after collision if possible
> 
> The collision event handling introduced in commit 71788c14 ignored
> downstream preference, still prefer it to make it possible for the application
> to select the SSRC itself.

I would like to deprecated the negotiation of SSRC via caps because it doesn't work when a session can have multiple SSRCs as senders. I would simply let the elements that produce the RTP packets choose and SSRC and then have the RTPCollision event make it choose a new SSRC. If anything, the session could suggest a free SSRC as part of the event.

So, selection of the SSRC is then done by setting the SSRC properties on the relevant elements.
Comment 4 Olivier Crête 2013-12-16 15:01:36 UTC
(In reply to comment #3)
> So, selection of the SSRC is then done by setting the SSRC properties on the
> relevant elements.

The how do you handle the case where a receiver become a sender later, how do you prevent a second useless SSRC from being added ? And the original sender-only one never times out.

This is also an API break from 1.0, at least Farstream relies on the "internal-ssrc" property of the internal-source to set the SSRC..

As I said on the other bug, I'm not convinced sending multiple sender-side SSRCs over the same pad was the best idea.
Comment 5 Wim Taymans 2013-12-17 14:32:14 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > So, selection of the SSRC is then done by setting the SSRC properties on the
> > relevant elements.
> 
> The how do you handle the case where a receiver become a sender later, how do
> you prevent a second useless SSRC from being added ? And the original
> sender-only one never times out.

When a session needs to generate RTCP and it was only a receiver, it generates an internal source and SSRC. If you later want to make a sender with this same SSRC you will have to configure it that way (currently it's set on the sinkpad caps so new senders can pick it up for backwards compatibility). This doesn't work for multiple senders.

The original receiver-only SSRC should time out when the session has a (different) sender SSRC, I guess that's not implemented yet.

> 
> This is also an API break from 1.0, at least Farstream relies on the
> "internal-ssrc" property of the internal-source to set the SSRC..

I'm sure we can fix that.
Comment 6 Olivier Crête 2013-12-17 20:48:07 UTC
(In reply to comment #5)
> (In reply to comment #4)
> The original receiver-only SSRC should time out when the session has a
> (different) sender SSRC, I guess that's not implemented yet.

Currently, all of the "internal" rtpsources never time out, we should let them time out like any other rtpsource? But we need one to never time out (for receiving). Maybe check the one in the currently set caps, which I suspect shoudl be the suggested_ssrc, which should be also settable from the property? And let the rest time out normally?

> > This is also an API break from 1.0, at least Farstream relies on the
> > "internal-ssrc" property of the internal-source to set the SSRC..
> 
> I'm sure we can fix that.

How should we do that? If the payloader ignores the SSRC from downstream, the setting this "internal-ssrc" does nothing. Previously, rtpsession would even overwrite the SSRC in outgoing packets from this internal-ssrc, but that API I can live with having broken.
Comment 7 Wim Taymans 2013-12-18 11:19:12 UTC
(In reply to comment #6)
> Currently, all of the "internal" rtpsources never time out, we should let them
> time out like any other rtpsource? But we need one to never time out (for
> receiving). Maybe check the one in the currently set caps, which I suspect
> shoudl be the suggested_ssrc, which should be also settable from the property?
> And let the rest time out normally?

> 
> > > This is also an API break from 1.0, at least Farstream relies on the
> > > "internal-ssrc" property of the internal-source to set the SSRC..
> > 
> > I'm sure we can fix that.
> 
> How should we do that? If the payloader ignores the SSRC from downstream, the
> setting this "internal-ssrc" does nothing. Previously, rtpsession would even
> overwrite the SSRC in outgoing packets from this internal-ssrc, but that API I
> can live with having broken.

what about this:

internal-ssrc alows you to configure an SSRC, it is then used in the caps to suggest an SSRC upstream. It is only changed automatically when a collision happens.

If RTCP happens and we have no internal SSRC, we create a source with the internal-ssrc for RR RTCP.

If a new internal source is created (with different SSRC, it also has to be a sender), we timeout the old SSRC and update the caps to the new SSRC.

Other internal (sender) SSRCs can be created and will time out when no data has been sent for them for a long time.
Comment 8 Wim Taymans 2013-12-18 13:49:48 UTC
(In reply to comment #4)

> As I said on the other bug, I'm not convinced sending multiple sender-side
> SSRCs over the same pad was the best idea.

Pro and cons of sending multiple sender over different pads into the same session:

Pro:

 - Each sinkpad has its own SSRC caps field suggestion
 - possibility to EOS per sender SSRC

Con:
 
 - Need to change rtpsession to allow multiple request pads for send_rtp_sink, need to add a unique number to each sender pad, it would break ABI/API. Can we do it without problems (add new send_rtp_sink_%u, would it break things?)
 - Configuration of AUX elements (session or SSRC multiplexing) needs to be done
in the AUX element because we would now always request multiple srcpads from the sender AUX element, how do we link the srcpads to a session? It would need something like src_%u_%u with ssrc number and session number to link to. The AUX element should make different SSRCs for different sessions.
 - asymetric with receiver session until the ssrcdemux and jitterbuffer are merged into session. Merging ssrcdemux and jitterbuffer into session might be
impossible because AUX receiver need to be placed before jitterbuffer.
 - how does the AUX receiver element work? it needs sink_%u_%u as well and expect different SSRC for same sessions, same SSRC for different sessions.
Comment 9 George Kiagiadakis 2013-12-26 15:56:30 UTC
(In reply to comment #7)
> what about this:
> 
> internal-ssrc alows you to configure an SSRC, it is then used in the caps to
> suggest an SSRC upstream. It is only changed automatically when a collision
> happens.
> 
> If RTCP happens and we have no internal SSRC, we create a source with the
> internal-ssrc for RR RTCP.
> 
> If a new internal source is created (with different SSRC, it also has to be a
> sender), we timeout the old SSRC and update the caps to the new SSRC.
> 
> Other internal (sender) SSRCs can be created and will time out when no data has
> been sent for them for a long time.

Hi,

I just tried to implement this logic. I have patches here:

http://cgit.collabora.com/git/user/gkiagia/gst-plugins-good.git/log/?h=session-fixes
http://cgit.collabora.com/git/user/gkiagia/gst-plugins-base.git/log/?h=1.0

it's untested and probably needs a second look & cleanups, which I will do tomorrow. Just letting you know that I have done some work on it.
Comment 10 Olivier Crête 2013-12-27 20:11:15 UTC
I think there are other conditions where you don't want to timeout an internal source completely, like if it's the only one left (even if it doesn't match the suggested ssrc).

Also, I just though of this... I'm not not sure we want to timeout the internal-ssrc from the RTX session even if it is idle for a longish time. Maybe internal ssrcs should just BYE having having received a new GstRTPSsrcEOS "partial-EOS" event?
Comment 11 Wim Taymans 2013-12-30 12:51:21 UTC
commit 6108407db1b2d4e7dd9d41244f2f555cc57c0e43
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Thu Dec 26 17:47:46 2013 +0200

    gstrtpbasepayload: use the session's suggested ssrc after a collision, if the session provides one
    
    Conflicts:
        gst-libs/gst/rtp/gstrtpbasepayload.c
Comment 12 Wim Taymans 2013-12-30 13:04:01 UTC
commit 5ddf6a5e3274d433409f4cbd7d4c6a942df1376e
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Thu Dec 26 12:05:19 2013 +0200

    gstrtpsession: suggest upstream to use the new "internal-ssrc" after a collision
    
    When a collision is found on the internal ssrc, we have to change it.
    Ideally, we want also the payloader upstream to follow this change and use
    the new internal ssrc. Ideally we want this condition to be always met:
    if there is one payloader sending on this session, its ssrc should match the
    internal ssrc.

commit 17517ca491e5bc07d36613112a4e987e7bb16609
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Thu Dec 26 11:04:29 2013 +0200

    rtpsession: allow setting internal-ssrc again
Comment 13 George Kiagiadakis 2013-12-31 15:23:21 UTC
(In reply to comment #10)
> I think there are other conditions where you don't want to timeout an internal
> source completely, like if it's the only one left (even if it doesn't match the
> suggested ssrc).

The last source will always match the suggested ssrc, because there is a check in add_source() that changes the suggested_ssrc to be the same as the ssrc of the last added source. Together with rtpbasepayload using the suggested ssrc after a collision, one can assert that the last source left in the session always uses the suggested ssrc.

> Also, I just though of this... I'm not not sure we want to timeout the
> internal-ssrc from the RTX session even if it is idle for a longish time. Maybe
> internal ssrcs should just BYE having having received a new GstRTPSsrcEOS
> "partial-EOS" event?

I also thought of this. It doesn't necessarily harm to send BYE and re-establish a connection later, but it sure doesn't look very nice.

I think that this is all unrelated to this bug report, though. This one is about rtpbasepayload using the suggested ssrc after a collision, which afaict is fixed now. I will put the rest of the patches in bug 720440, together with a unit test.
Comment 14 leonidwxxinosov 2015-11-09 19:08:20 UTC
Created attachment 315151 [details]
tea