GNOME Bugzilla – Bug 658419
Add FIR support to rtpsession
Last modified: 2011-11-14 11:33:31 UTC
I have two sets of patches to add support for RFC 5104 RTCP FIR (Full Intra Request). The first set is for receiving FIR requests and just emits an upstream GstForceKeyUnit event with all-headers=TRUE. It also includes a patch to -base to add the numbers from RFC 5104 to the rtcpbuffer.h header. The second set adds support for sending FIR requests. I think it should only be used when the RTP depayloader detects that the codec data is missing. Which would happen if you switch sources or something. The problem is that the FIR request has a sequence number that should not be incremented if the same request is being re-transmitted. So I think this requires adding a count number to the GstForceKeyUnit event (see bug #607742 comment #24).
Created attachment 195830 [details] [review] base: rtcpbuffer: Add feedback message types from RFC 5104
Created attachment 195831 [details] [review] rtpsession: Process received Full Intra Requests Process FIR requests according to RFC 5104
Created attachment 195832 [details] [review] rtpsession: Hack to FIR because Google doesn't set the sender ssrc correctly
Created attachment 195833 [details] [review] rtpsession: Put the PLI requests in each RTPSource Also refactor a bit and put all the keyframe request code in one place inside rtpsession.c
Created attachment 195834 [details] [review] rtpsession: Send FIR requests in response to key unit requests with all-headers=TRUE
Created attachment 195835 [details] [review] gstrtpsession: Add special mode to use FIR as repair as Google does
Comment #1 to #3 contain the first series of patches and comments #4, #5 and #6 have the seconid.
(In reply to comment #5) > Created an attachment (id=195834) [details] [review] > rtpsession: Send FIR requests in response to key unit requests with > all-headers=TRUE Looks like there is a started_fir = TRUE; missing (probably in the first if alongside ret = TRUE; if I get the drift).
(In reply to comment #6) > Created an attachment (id=195835) [details] [review] > gstrtpsession: Add special mode to use FIR as repair as Google does Not sure about the semantics/specs or whatever the sender of the even is doing here, but the comment says "even if we just want regular PLI" while the code checks for if (!pli ...) (which does not seem to match "wanting regular PLI").
Created attachment 200725 [details] [review] rtpsession: Send FIR requests in response to key unit requests with all-headers=TRUE Original patch had a tiny bug, here is an updated version
Any comments on previous comments/nitpicks ? In particular, latest patch still has a started_fir that looks pretty (a)stray.
Created attachment 201260 [details] [review] rtpsession: Send FIR requests in response to key unit requests with all-headers=TRUE The Google hack is a bit special.. It's because Google uses FIR request when they should be using PLI. So basically what we mean is, if you didn'T negotiate pli, but you negotiated the Google FIR, then send a FIR. Hopefully we can remove it when we Google updates their stuff. I also updated the other patch to actually set started_fir, good catch.
Committed to -base: commit 82827df405170035213dcef062cf9e6d1a4cdc6e Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue Aug 30 18:27:09 2011 -0400 rtcpbuffer: Add feedback message types from RFC 5104 These are Codec Control messages (CCM) https://bugzilla.gnome.org/show_bug.cgi?id=658419 Committed to -good: commit 1169bb05af25d6f0902a8d29c067b237ebfd6849 Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Sep 2 19:20:07 2011 -0400 gstrtpsession: Add special mode to use FIR as repair as Google does https://bugzilla.gnome.org/show_bug.cgi?id=658419 commit 79a9564c688773aae3ed1df006f82beb6e2d038e Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Sep 1 17:47:38 2011 -0400 rtpsession: Send FIR requests in response to key unit requests with all-headers=TRUE https://bugzilla.gnome.org/show_bug.cgi?id=658419 commit 12a6b9613bd081f16719734a1706c8f2b9d6b36f Author: Olivier Crête <olivier.crete@collabora.com> Date: Thu Sep 1 16:25:21 2011 -0400 rtpsession: Put the PLI requests in each RTPSource Also refactor a bit and put all the keyframe request code in one place inside rtpsession.c https://bugzilla.gnome.org/show_bug.cgi?id=658419 commit 59c028a4ceb96a71e7d37c965ff72d9bc2cc4641 Author: Olivier Crête <olivier.crete@collabora.com> Date: Wed Aug 31 14:35:33 2011 -0400 rtpsession: Hack to FIR because Google doesn't set the sender ssrc correctly https://bugzilla.gnome.org/show_bug.cgi?id=658419 commit 0ad78db0a30bda6b9be9b674c4b1ecf90a8a69c0 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue Aug 30 19:06:13 2011 -0400 rtpsession: Process received Full Intra Requests Process FIR requests according to RFC 5104 https://bugzilla.gnome.org/show_bug.cgi?id=658419