GNOME Bugzilla – Bug 720435
rtpbasepayload: Prefer downstream SSRC after collision if possible
Last modified: 2015-11-09 19:08:20 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.
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.
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
(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.
(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.
(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.
(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.
(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.
(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.
(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.
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?
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
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
(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.
Created attachment 315151 [details] tea