GNOME Bugzilla – Bug 795139
Various patches on rtpsession
Last modified: 2018-11-03 15:28:39 UTC
Created attachment 370740 [details] [review] Running gst-indent on the rtpsession tests A collection of patches for rtpsession, to be applied in order.
Created attachment 370741 [details] [review] patch with test rtpsession: Add signals for handling profile-specific extension
Created attachment 370742 [details] [review] rtpsession: Don't start the RTCP thread until it's needed rtpsession: Don't start the RTCP thread until it's needed Always wait with starting the RTCP thread until either a RTP or RTCP packet is sent or received. Special handling is needed to make sure the RTCP thread is started when requesting an early RTCP packet. We want to wait with starting the RTCP thread until it's needed in order to not send RTCP packets for an inactive source.
Created attachment 370743 [details] [review] rtpsession: Allow instant transmission of RTCP packets rtpsession: Allow instant transmission of RTCP packets By specifying max_delay of 0, it effectively bypasses the RTCP throttling mechanism, and tries to get an RTCP packet on the wire as quickly as possible.
Created attachment 370744 [details] [review] rtpsession: Try media_ssrc if no src can be found for PLI sender_ssrc rtpsession: Try media_ssrc if no src can be found for PLI sender_ssrc Some RTP stacks out there does not set the sender_ssrc. In order to be more robust, try to lookup the media_ssrc before dropping the PLI.
Created attachment 370745 [details] [review] rtpsession: Fix on-feedback-rtcp race rtpsession: Fix on-feedback-rtcp race If there is an external source which is about to timeout and be removed from the source hashtable and we receive feedback RTCP packet with the media ssrc of the source, we unlock the session in rtp_session_process_feedback before emitting 'on-feedback-rtcp' signal allowing rtcp timer to kick in and grab the lock. It will get rid of the source and rtp_session_process_feedback will be left with RTPSource with ref count 0. The fix is to grab the ref to the RTPSource object in rtp_session_process_feedback.
Created attachment 370746 [details] [review] rtpsession: Avoid unnecessary copy of stats structure rtpsession: Avoid unnecessary copy of stats structure The code before copied GstStructure twice. The first time inside gst_value_set_structure and the second time in g_value_array_append. Optimized version does no copies, just transfers ownership to GValueArray. It takes advantage of the fact that array has already enough elements preallocated and the memory is zero initialized.
Created attachment 370747 [details] [review] rtpsession: do not emit RBs for internal senders. rtpsession: do not emit RBs for internal senders. These are the sources we send from, so there is no reason to report receive statistics for them (as we do not receive on them, and the remote side has no knowledge of them).
Created attachment 370748 [details] [review] rtpsession: Add missing lock around sess->ssrcs iteration rtpsession: Add missing lock around sess->ssrcs iteration
Created attachment 370749 [details] [review] rtpsession: Add "send-bye" action-signal rtpsession: Add "send-bye" action-signal
Awesome!
Created attachment 370781 [details] [review] rtpsession: Drop packet if trying to send from non-internal source If obtain_internal_source() returns a source that is not internal it means there exists a non-internal source with the same ssrc. Such an ssrc collision should be handled by sending a GstRTPCollision event upstream and choose a new ssrc, but for now we simply drop the packet. Trying to process the packet further will cause it to be pushed usptream (!) since the source is not internal (see source_push_rtp()).
Created attachment 370787 [details] [review] rtpsession: Send and receive Microsoft x-pli rtpsession: Send and receive Microsoft x-pli This patch checks RTP send and receive caps for the field 'rtcp-fb-x-message-x-pli' in the same manner as other rtcp feedback key unit methods are checked for. The only difference from a standard PLI is the extra FCI blob which includes a 16bit request id and a 64 bit so called SFR. This patch ignores retransmissions of equal request ids when x-pli is turned on. The patch includes tests for fir, pli and x-pli, as well as a fix for making clear-pt-map actually work for gstrtpsession.
Created attachment 370793 [details] [review] rtpsession: process RB by internal sources When we deal with receiver reports containing information about multiple internal SSRCs we used to save data only for the SSRC in the last report block. Thus the stats would never reflect receiver reports of other SSRCs. For example. Non internal source 0xAAAA receives RR with 3 RBs about internal sources 0x1111, 0x2222 and 0x3333. Because 0x3333 was the last processed RB we will never see the receiver report of 0x1111 and 0x2222 in the RTP session stats. With this patch we save receiver report information in the corresponding internal source.
Created attachment 370794 [details] [review] rtpsession: Add property "stats-min-interval" and notify "stats" Notify about changed stats when RTCP is sent. Since notifying on every RTCP packet (when early RTCP is used) may introduce significant overhead, so the property "stats-min-interval" is added in order to control how often this notification can happen.
Created attachment 370842 [details] [review] rtpsession: Try media_ssrc if no src can be found for PLI sender_ssrc
Created attachment 370843 [details] [review] rtpsession: Fix on-feedback-rtcp race
Created attachment 370844 [details] [review] rtpsession: Add "send-bye" action-signal
Created attachment 370845 [details] [review] rtpsession: Drop packet if trying to send from non-internal source
Created attachment 370846 [details] [review] rtpsession: Send and receive Microsoft x-pli
Created attachment 370847 [details] [review] rtpsession: process RB by internal sources
Created attachment 370848 [details] [review] rtpsession: Add property "stats-min-interval" and notify "stats"
I updated some of the patches with fixes for valgrind, autotools, gst-indent, and determinism.
Review of attachment 370740 [details] [review]: can't object to this!
Review of attachment 370741 [details] [review]: Can you add slightly more descriptive signal names? Otherwise looks good ::: gst/rtpmanager/rtpsession.c @@ +246,3 @@ + */ + rtp_session_signals[SIGNAL_ON_SSRC_PSE] = + g_signal_new ("on-ssrc-pse", G_TYPE_FROM_CLASS (klass), maybe on-ssrc-profile-specific-ext ? A couple extra characters are not that expensive! @@ +309,3 @@ + */ + rtp_session_signals[SIGNAL_ON_CREATING_SRRR] = + g_signal_new ("on-creating-srrr", G_TYPE_FROM_CLASS (klass), can we do on-creating-sr-rr? I couldn't guess was srrr was!
Review of attachment 370743 [details] [review]: I like this, but can we add some documentation to the property on this special 0 value?
Review of attachment 370746 [details] [review]: This is unsafe.. It only works if the array is already big enough.. it should try to grow it. Instead, you can try to add a new API to GLib.. even though it will be a pain.. Maybe a hack is to add a int GValue copied from the stack, then reset it, then re-init it...
Review of attachment 370747 [details] [review]: looks like a good idea
Review of attachment 370748 [details] [review]: Indeed
Review of attachment 370842 [details] [review]: Looks ok
Review of attachment 370843 [details] [review]: Looks ok, but can you add a comment in the code too?
Review of attachment 370844 [details] [review]: How is this action signal different from pushing a EOS on the pad?
Review of attachment 370845 [details] [review]: I guess this is better than nothing...
Review of attachment 370847 [details] [review]: ++
Review of attachment 370848 [details] [review]: My only problem is the name.. Maybe "stats-notify-min-interval" ? ::: gst/rtpmanager/rtpsession.c @@ +78,3 @@ #define DEFAULT_RTP_PROFILE GST_RTP_PROFILE_AVP #define DEFAULT_RTCP_REDUCED_SIZE FALSE +#define DEFAULT_STATS_MIN_INTERVAL 3000 I think the default should be 0.. otherwise it's an API change
Review of attachment 370846 [details] [review]: ::: gst/rtpmanager/rtpsession.c @@ +2699,3 @@ + rtp_session_request_local_key_unit (sess, src, media_ssrc, FALSE, + current_time); + } else if (fci_length == 3 * sizeof (guint32)) { Any reason to do this recv_xpli thing instead of auto-detecting if it's Microsoft special or standard based on the packet itself? ::: gst/rtpmanager/rtpsource.h @@ +209,3 @@ GQueue *retained_feedback; + RTPKeyUnitType key_unit_type; This won't work.. you can currently request a mix of FIR and of PLI, so it you can't pick a single type..
(In reply to Olivier Crête from comment #26) > Review of attachment 370746 [details] [review] [review]: > > This is unsafe.. It only works if the array is already big enough.. it > should try to grow it. > > Instead, you can try to add a new API to GLib.. even though it will be a > pain.. Maybe a hack is to add a int GValue copied from the stack, then reset > it, then re-init it... I agree. Actually, GLib has an API for it. g_value_array_append (arr, NULL) allocates and zero fills the memory for the new value if needed, but does not init it. I will update the patch.
Created attachment 371533 [details] [review] rtpsession: Add signals for handling profile-specific extension Name-change as requested
Created attachment 371534 [details] [review] rtpsession: Avoid unnecessary copy of stats structure updated as specified
Created attachment 371535 [details] [review] rtpsession: process RB by internal sources updated
Created attachment 371536 [details] [review] rtpsession: Add property "stats-min-interval" and notify "stats" Updated with name-change and suggested default value. (to mimic old behavior) I would however suggest to change this default-value, as having thousands of stats arriving every second (in the extreme cases) is not very useful and can cause a listening application to halt.
(In reply to Olivier Crête from comment #31) > Review of attachment 370844 [details] [review] [review]: > > How is this action signal different from pushing a EOS on the pad? No different, but in terms of application and bindings, being able to trigger such functionality without having to deal with pads and creating events is useful.
Created attachment 371537 [details] [review] rtpsession: Add tests for PLI and FIR Turns out we don't actually use this patch internally anymore, so instead I have simply added the tests written for PLI and FIR (as well as the fix for clear-pt-map that the tests uses).
(In reply to Olivier Crête from comment #25) > Review of attachment 370743 [details] [review] [review]: > > I like this, but can we add some documentation to the property on this > special 0 value? Where would be a good place to document this?
Created attachment 371539 [details] [review] rtpsession: Fix race when sending PLI, FIR and NACK packets Calling rtp_session_send_rtcp before marking the source as requiring a pli/fir/nack meant the rtcp_thread could be scheduled and start running before the source was updated. This meant the request would not be sent early but instead was transmitted with the next regular RTCP packet. Add test for nack generation.
*** Bug 746814 has been marked as a duplicate of this bug. ***
*** Bug 791075 has been marked as a duplicate of this bug. ***
*** Bug 791076 has been marked as a duplicate of this bug. ***
Created attachment 371992 [details] [review] rtpsession: Add signals for handling profile-specific extension Updated patch after rebase
Created attachment 371993 [details] [review] rtpsession: Don't start the RTCP thread until it's needed Updated patch after rebase
Created attachment 371994 [details] [review] rtpsession: Allow instant transmission of RTCP packets Updated patch after rebase
Created attachment 371995 [details] [review] rtpsession: Avoid unnecessary copy of stats structure Updated patch after rebase
Created attachment 371996 [details] [review] rtpsession: Add "send-bye" action-signal Updated patch after rebase
Created attachment 371997 [details] [review] rtpsession: Drop packet if trying to send from non-internal source Updated patch after rebase
Created attachment 371998 [details] [review] rtpsession: Add tests for PLI and FIR Updated patch after rebase
Created attachment 371999 [details] [review] rtpsession: process RB by internal sources Updated patch after rebase
Created attachment 372000 [details] [review] rtpsession: Add property "stats-min-interval" and notify "stats" Updated patch after rebase
Created attachment 372001 [details] [review] rtpsession: Fix race when sending PLI, FIR and NACK packets Updated patch after rebase
Comment on attachment 371994 [details] [review] rtpsession: Allow instant transmission of RTCP packets Need to add docs about special 0 value. Need to figure out where to add that. (From previous review)
Comment on attachment 371996 [details] [review] rtpsession: Add "send-bye" action-signal (From previous review comment) Question was how this is different from sending an EOS on the pad. Answer: not functionally different, but nicer API, app doesn't have to mess with pads and events and such.
Comment on attachment 371998 [details] [review] rtpsession: Add tests for PLI and FIR Split this in two (one for the clear-pt-map fix one for the tests).
(In reply to Tim-Philipp Müller from comment #59) Can we write that in the doc then. "This is the same behaviour as sending an EOS event to the pad."
Question about this ("send-bye" action signal) though: does it really behave the same way? What happens if data arrives after/while it's been emitted? With an EOS event GstPad will set the EOS flag and all further data will be dropped automatically until the pad gets flushed or STREAM_START arrives. Do we need something similar here too to at least drop data?
Review of attachment 371993 [details] [review]: Looks good to me, tests are passing, and it fixes a race condition I was observing, rebasing and pushing, thanks!
Comment on attachment 371993 [details] [review] rtpsession: Don't start the RTCP thread until it's needed Attachment 371993 [details] pushed as ac6e77a - rtpsession: Don't start the RTCP thread until it's needed
Just wanted to mention that this patch: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/rtpmanager/gstrtpsession.c?id=ac6e77acad79907564c96383f87d50bcf82d9e33 Fixes an issue in webrtcbin where an initial SSRC is negotiated in the offer but then a different SSRC is used in the RTP packets. I found this on an application that establishes a webrtc connection then closes it and opens another one. The first one succeeds, but after the second one I always hit the issue. From the logs I can see how the ICE connection establishment succeeds earlier in the second time. I believe this is because a new SSRC from the RTCP was being negotiated instead of the original one coming from the payloader. Does this make any sense?
Also, not sure if you would like to consider this patch to be included in 1.14 since it's a very easy to hit bug.
Picked into 1.14 (for 1.14.3) thanks.
(In reply to Aleix Conchillo Flaqué from comment #66) > Just wanted to mention that this patch: > > https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/gst/ > rtpmanager/gstrtpsession.c?id=ac6e77acad79907564c96383f87d50bcf82d9e33 > > Fixes an issue in webrtcbin where an initial SSRC is negotiated in the offer > but then a different SSRC is used in the RTP packets. That is indeed the issue I was having :)
Review of attachment 371992 [details] [review]: Is the new "on-creating-srrr" strictly necessary, or could "on-sending-rtcp" be used instead? If not, why, and if yes, shouldn't we have a more generic signal emitted when any RTCP packet is created?
(In reply to Mathieu Duponchelle from comment #70) > Review of attachment 371992 [details] [review] [review]: > > Is the new "on-creating-srrr" strictly necessary, or could "on-sending-rtcp" > be used instead? If not, why, and if yes, shouldn't we have a more generic > signal emitted when any RTCP packet is created? this should read: "and in that case, shouldn't we have", my bad
Review of attachment 371999 [details] [review]: This looks like the right thing to do indeed, it's surprising that we did obtain the correct source but then proceeded to update the stats of the sender of the packet. Also I don't think the comment in the commit is correct, afaict we previously used the "SSRC of packet sender" obtained with obtain_source(gst_rtcp_packet_rr_get_ssrc()), not with the ssrc of the last report block. ::: gst/rtpmanager/rtpsession.c @@ +2273,3 @@ * the other sender to see if we are better or worse. */ /* FIXME, need to keep track who the RB block is from */ + rtp_source_process_rb (src, pinfo->ntpnstime, fractionlost, Does that mean we can remove the FIXME right above? Also the comment will need updating, as it says "we update the stats of the sender of the RTCP message.", which will no longer be true
Review of attachment 372000 [details] [review]: Only minor comments, this is OK to commit for me once these are addressed ::: gst/rtpmanager/rtpsession.c @@ +612,3 @@ + g_param_spec_uint ("stats-notify-min-interval", + "Stats notify minimum interval", + "Minimum interval between emitting notify signal for stats (in ms)", between emissions of the notify signal rather? ::: tests/check/elements/rtpsession.c @@ +1239,3 @@ +count_report_stats (GObject * object, GParamSpec * spec, gint * counter) +{ + (void) object; Any reason not to use G_GNUC_UNUSED? @@ +1297,3 @@ + + /* verify we have generated less than 3 stats for all these packets */ + fail_unless (stats_callback_count < 3); Can we not make a strict comparison here?
Review of attachment 372001 [details] [review]: Looks good to me
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/460.