GNOME Bugzilla – Bug 749581
rtpbasepayload: Try harder to reuse previously configured caps values and give more preference to anything set as properties
Last modified: 2015-06-05 14:46:21 UTC
See commit message
Created attachment 303594 [details] [review] rtpbasepayload: Try harder to reuse previously configured caps values and give more preference to anything set as properties This affects the pt, ssrc, seqnum-offset and timestamp-offset properties. If they were set from a property, or we configured caps before, we try to use that value for them. Even if the first structure of the downstream caps specifies a different value, we check if the value is supported by other structures. Only if all this fails, we use the values given by downstream in the first structure, i.e. if no properties were set and these are the first caps we negotiate or downstream does not support our values. By doing this we ensure that we don't spuriously change ssrcs or other fields in the middle of the stream (and also consider property values more). Ssrc changes would currently happen after sending an RTX packet (thus creating a new internal source inside the rtpsession), and then renegotiating the payloader (which then gets the RTX ssrc from rtpsession).
Created attachment 303595 [details] [review] rtpbasepayload: Try harder to reuse previously configured caps values and give more preference to anything set as properties This affects the pt, ssrc, seqnum-offset and timestamp-offset properties. If they were set from a property, or we configured caps before, we try to use that value for them. Even if the first structure of the downstream caps specifies a different value, we check if the value is supported by other structures. Only if all this fails, we use the values given by downstream in the first structure, i.e. if no properties were set and these are the first caps we negotiate or downstream does not support our values. By doing this we ensure that we don't spuriously change ssrcs or other fields in the middle of the stream (and also consider property values more). Ssrc changes would currently happen after sending an RTX packet (thus creating a new internal source inside the rtpsession), and then renegotiating the payloader (which then gets the RTX ssrc from rtpsession).
commit faafaaec56de31ebecf02b87e1fc0e4c07ed9ecc Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue May 19 16:32:38 2015 +0300 rtpbasepayload: Try harder to reuse previously configured caps values and give more preference to anything set as properties This affects the pt, ssrc, seqnum-offset and timestamp-offset properties. If they were set from a property, or we configured caps before, we try to use that value for them. Even if the first structure of the downstream caps specifies a different value, we check if the value is supported by other structures. Only if all this fails, we use the values given by downstream in the first structure, i.e. if no properties were set and these are the first caps we negotiate or downstream does not support our values. By doing this we ensure that we don't spuriously change ssrcs or other fields in the middle of the stream (and also consider property values more). Ssrc changes would currently happen after sending an RTX packet (thus creating a new internal source inside the rtpsession), and then renegotiating the payloader (which then gets the RTX ssrc from rtpsession). https://bugzilla.gnome.org/show_bug.cgi?id=749581
It should default to the downstream recommendation. Farstream relies on this behavior, so we can just change the "internal-ssrc" property on rtpsession and have it propagated upstream. Suggestion to fix your problem, changing rtpsession to, for caps negotiation, ignore the SSRC inside the packets and just take it from the caps event, then return that in the caps query. I assume that should prevent the rtprtxsend from interfering and causing useless renegotiations?
The patch from this bug breaks the rtp/sendcodecs test in Farstream, reverting it makes it work again.
(In reply to Olivier Crête from comment #4) > It should default to the downstream recommendation. Farstream relies on this > behavior, so we can just change the "internal-ssrc" property on rtpsession > and have it propagated upstream. Is this only a problem with the SSRC, or do the changes to the pt/seqnum/clockbase fields negotiation also look problematic to you? > Suggestion to fix your problem, changing rtpsession to, for caps > negotiation, ignore the SSRC inside the packets and just take it from the > caps event, then return that in the caps query. I assume that should prevent > the rtprtxsend from interfering and causing useless renegotiations? That should work, yes. That would mean that rtpsession is always proposing - a new SSRC if there was a collision - the SSRC from the property if it was set - the SSRC from the caps if there are caps and they contain an ssrc - a random SSRC (whichever condition is true first). Additionally rtpbasepayload would always prefer a downstream SSRC if there is one, which means that the property is completely ignored if downstream proposes an SSRC. So with rtpbin, the SSRC property on rtpbasepayload has absolutely no effect (as before the breaking change). Seems ok to you?
(In reply to Sebastian Dröge (slomo) from comment #6)> > Is this only a problem with the SSRC, or do the changes to the > pt/seqnum/clockbase fields negotiation also look problematic to you? Just ssrc, seqnum/clockbase are random (in Farstream) and pt is always fixed. > > Suggestion to fix your problem, changing rtpsession to, for caps > > negotiation, ignore the SSRC inside the packets and just take it from the > > caps event, then return that in the caps query. I assume that should prevent > > the rtprtxsend from interfering and causing useless renegotiations? > > That should work, yes. That would mean that rtpsession is always proposing > - a new SSRC if there was a collision > - the SSRC from the property if it was set > - the SSRC from the caps if there are caps and they contain an ssrc > - a random SSRC > > (whichever condition is true first). > > Additionally rtpbasepayload would always prefer a downstream SSRC if there > is one, which means that the property is completely ignored if downstream > proposes an SSRC. So with rtpbin, the SSRC property on rtpbasepayload has > absolutely no effect (as before the breaking change). > > > Seems ok to you? Seems ok to me. In 2.0, we can drop the ssrc property on rtpsession and control everything from the payloaders !
Please also port farstream away from the deprecated internal-ssrc property :) commit 3113e341eac26b1766ad01a04db690926b12d165 Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jun 5 16:44:08 2015 +0200 rtpbasepayload: Always prefer downstream's ssrc suggestion if any Otherwise ssrc changes via rtpsession's (deprecated!) internal-ssrc property are not possible anymore. rtpsession was now patched to only suggest an ssrc if it makes sense to do so. In 2.0 we should get rid of all the properties that are also negotiated via caps, the code and behaviour is too confusing otherwise. https://bugzilla.gnome.org/show_bug.cgi?id=749581 commit d650a310da7768f775080278b9e0c5d26f95a2ab Author: Sebastian Dröge <sebastian@centricular.com> Date: Fri Jun 5 16:43:08 2015 +0200 rtpsession: Only suggest our internal ssrc if it's not a random one and was selected as internal ssrc https://bugzilla.gnome.org/show_bug.cgi?id=749581