GNOME Bugzilla – Bug 622553
rtpmanager: Implement RFC 4585 (AVPF / early feedback)
Last modified: 2011-04-15 11:57:05 UTC
I've written an implemention of RFC 4585 (RTP AV profile with early feedback aka AVPF) for rtpmanager. Branch is at http://git.collabora.co.uk/?p=user/tester/gst-plugins-good.git;a=shortlog;h=refs/heads/avpf-2010-06-23 Between master and rtpbin-fixes-2010-06-23, the patches are fixes to what I think are bugs in rtpmanager. Between rtpbin-fixes-2010-06-23 and rtp-pre-avpf-2010-06-23, I've added a number of improvements that AVPF support is built on. The patches up to avpf-timing-2010-06-23 implement the actual modified timing rules for AVPF. The two patches on top implement a simple type of feedback (Picture Loss Indication) as an example. I've implemented it inside gstrtpsession and rtpsession, but I'm really not certain that it is where it belongs. Possibly the tracking of requested PLIs should be moved to rtpsource. Maybe the whole thing should be done outside of rtpsession. I'm not certain at all. I don't think its ready for merging yet (I'd like to test it further first), but I'd still like to know if I'm heading in the right direction (or not).
It certainly looks right on track and I'm thinking of merging a bit of it. I'll comment while I go.
commited some fixes: commit 306ee454c6f206810b234129c7f0fa73753ac96a Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Tue Jun 1 19:51:34 2010 -0400 gstrtpsession: Don't unref pads in finalize The gstrtpsession object is not holding any reference to them directly commit 64e4ffa25baf5e35d6f4b6f3f6a8aa4ceabf730b Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Tue Jun 1 20:31:18 2010 -0400 rtpsession: Count sent RTCP packets after they have been finished If they are counted before calling gst_rtcp_buffer_end(), then the size is way too big. commit 00fd89c0742f93e4cbfea59ff37c24269f44857b Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Wed Jun 23 16:13:01 2010 -0400 rtpstats: Rectify description of current_time in RTPArrivalStats It is the current time, it is unrelated to when the packet was actually received.
> @@ -78,11 +78,11 @@ enum > * precision. */ > #define UPDATE_AVG(avg, val) \ > if ((avg) == 0) \ > (avg) = (val) << 4; \ > else \ >- (avg) = ((val) + (15 * (avg))) >> 4; >+ (avg) = ((val << 4) + (15 * (avg))) >> 4; Is not quite right, it adds 1/16th of the previous average to the last value. Changed to: (avg) = ((val) + (15 * (avg)));
Updated branch at: http://git.collabora.co.uk/?p=user/tester/gst-plugins-good.git;a=shortlog;h=refs/heads/avpf-timing The top commit on that branch is not totally related, but it is a good use case (it makes the theora depayloader emit a GstForceKeyUnit when a packet loss is detected). I guess this is probably ready to be merged Also, this branch of farsight2 can make use of it: http://git.collabora.co.uk/?p=user/tester/farsight2.git;a=shortlog;h=refs/heads/avpf-nego
I reviewed and merged the branch, it all looked good. These are some comments that probably need to be cleared up before the release: * the on-sending-rtcp signal uses gpointer for the buffer, might use a miniobject too (is a bit more annoying to generate the marshaller but presumably handles the refcount better). * the on-feedback-rtcp partially parses the FB RTCP packet and puts various bits in the signal arguments. Would we do it like this or simply pass the buffer, like in on-sending-rtcp? how is this function used, mostly? * rtp_session_process_feedback() lots of peeking into the private bits of GstRTCPPacket. Can we use the public functions instead? maybe we need some helper functions? * gst_rtp_session_request_time() returns the absolute systemclock time. Is this right? I have not looked closer at what clock is used to compare against. * something is wrong with commit 1bde4272509c30f0818af865c2f8f51014f7ac14. I commited a patch to fix this commit (there was a bracket too much somewhere) to something that makes sense to me but you want to recheck if this is what you intended.
Created attachment 179835 [details] [review] rtpsession: Use existing functions to parse RTCP FB packets (In reply to comment #5) > * rtp_session_process_feedback() lots of peeking into the private bits of > GstRTCPPacket. Can we use the public functions instead? maybe we need some > helper functions? Here is a patch to use existing functions to do the parsing
Created attachment 179836 [details] [review] rtpsession: Marshal GstBuffer as a MiniObject instead of a pointer (In reply to comment #5) > * the on-sending-rtcp signal uses gpointer for the buffer, might use a > miniobject too (is a bit more annoying to generate the marshaller but > presumably handles the refcount better). Attached patch adds a hand-made marshaller like playsink has.
(In reply to comment #5) > * the on-feedback-rtcp partially parses the FB RTCP packet and puts various > bits > in the signal arguments. Would we do it like this or simply pass the buffer, > like in on-sending-rtcp? how is this function used, mostly? The idea was that the parsing always needs to be done, so we may as well do it in common place. If we simply pass the buffer, we may want to do on-rtcp-received or something like that and call it on every buffer. > * gst_rtp_session_request_time() returns the absolute systemclock time. Is this > right? I have not looked closer at what clock is used to compare against. GstRTPSession already gives the system clock to RTPSession in many places. I'm not sure why you didnt make RTPSEssion read it directly ?
Not quite sure why I marked this as a blocker, but the two remaining patches look fine/harmless to me (nitpick: maybe s/length/fci_length/?)... What else is left? Wim? Olivier?
I think Wim wanted to get these two patches in before releasing the stuff ? But since they're mostly cosmetic, I don't care too much.
I think you should just go ahead and commit them if you've tested them and they work for you.
Pushed those two now, but renamed variables in the first patch after I discovered a GST_BUFFER_TIMESTAMP (fci) = arrival->running_time; which hadn't been changed to fcibuffer. commit 9d9257916bf913908cfee140f8c2683ae85ceeae Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Tue Feb 1 15:57:01 2011 -0500 rtpsession: Use existing functions to parse RTCP FB packets Use existing functions to get the FCI from FB packets. https://bugzilla.gnome.org/show_bug.cgi?id=622553 commit 5ccd964d86ba24629187ba513387b96c0db3a4c5 Author: Olivier Crête <olivier.crete@collabora.co.uk> Date: Tue Feb 1 16:23:52 2011 -0500 rtpsession: marshal GstBuffer as a MiniObject instead of a pointer https://bugzilla.gnome.org/show_bug.cgi?id=622553 Hope this resolves this bug now. If not, please re-open.
Comment on attachment 179835 [details] [review] rtpsession: Use existing functions to parse RTCP FB packets Committed with changes, most notably GST_BUFFER_TIMESTAMP(fci) => GST_BUFFER_TIMESTAMP(fcibuffer)