GNOME Bugzilla – Bug 769698
sdp: add support for rtcp-fb attributes
Last modified: 2016-11-01 17:58:22 UTC
Created attachment 333045 [details] [review] Set of patches There is no implementation for rtcp-fb-nack-pli and rtcp-fb-ccm-fir (rfc5104) in both RTSP client and server. I have implemented it in following patches: 1) 0001-Parse-rtcp-fb-media-attributes.patch (sdpmessage, for rtsp client) Adds parsing a=rtcp-fb SDP media attributes into relevant RTP CAPS (like rtcp-fb-nack-pli) See: https://bugzilla.gnome.org/show_bug.cgi?id=769628 2) 0001-Add-rtcp-fb-SDP-attributes-based-on-caps.patch (rtsp-server) Adds rtcp-fb media attributes based on RTP CAPS. 3) 0001-gstrtpvp8pay-add-rtcp-fb-nack-pli-recp-fb-ccm-fir.patch (rtpvp8pay) Adds rtcp-fb-nack-pli and rtcp-fb-ccm-fir into vp8 payloader. Similiar patches are needed for other codes, perhaps it should be implemented in RTP payloader base class. With above set of patches user is able to create end-to-end RTSP connection with correctly working nack-pli with VP8 codec. Since I am relatively new to gstreamer, I am not sure how my changes fit into architecture and whether it is a desired way of implementing it. Alternatively I was thinking about adding new property to RTSPMediaFactory to control adding these SDP attributes, but I guess that looking for this in CAPS is cleaner.
Created attachment 333046 [details] [review] RTSP server: add rtcp-fb SDP attributes based on caps
Created attachment 333047 [details] [review] Client: parse rtcp-fb media attributes
Created attachment 333048 [details] [review] gstrtpvp8pay: add rtcp-fb-nack-pli & recp-fb-ccm-fir
Review of attachment 333048 [details] [review]: Doing this in every payloader makes no sense, you should just add a capsfilter after the payloader to set those.
Review of attachment 333047 [details] [review]: This patch looks fine, but having a couple unit tests for both valid and slightly invalid SDPs, therre are already some in tests/check/libs/sdp.c
Review of attachment 333046 [details] [review]: I wonmder if it wouldn't be better to just put this into gst_sdp_media_set_media_from_caps() instead to have symmetry with the other patch? And we should allow setting them even with the non-AVPF profiles, some "legacy" clients just ignore the profile and use it anyway. .
(In reply to Olivier Crête from comment #4) > Review of attachment 333048 [details] [review] [review]: > > Doing this in every payloader makes no sense, you should just add a > capsfilter after the payloader to set those. I agree, but I think it is currently not possible to insert capsfilter after the payloader with the RTSP server API as the payloader needs to be the last element of the pipeline. Either I am missing some way of setting it, or there is need for adding such capability into server's API then.
Created attachment 333059 [details] [review] Server: add rtcp-fb media attributes based on caps
(In reply to Olivier Crête from comment #6) > Review of attachment 333046 [details] [review] [review]: > > I wonmder if it wouldn't be better to just put this into > gst_sdp_media_set_media_from_caps() instead to have symmetry with the other > patch? And we should allow setting them even with the non-AVPF profiles, > some "legacy" clients just ignore the profile and use it anyway. > . You are absolutely right. Please check the new patch.
Review of attachment 333059 [details] [review]: Patch look good, would be nice to have unit tests, those could nicely be combined with the parser ones as they'll be super simple...
Created attachment 335076 [details] [review] Add rtcp-fb media attributes based on caps
Created attachment 335077 [details] [review] Parse rtcp-fb media attributes
Created attachment 335078 [details] [review] Add tests for rtcp-fb parsing
(In reply to Olivier Crête from comment #10) > Review of attachment 333059 [details] [review] [review]: > > Patch look good, would be nice to have unit tests, those could nicely be > combined with the parser ones as they'll be super simple... I have uploaded new set of patches along with some simple unit tests. Please have a look.
Review of attachment 335077 [details] [review]: You're missing parsing of '*' meaning that it applies to all formats.
Review of attachment 335076 [details] [review]: Looks good
Review of attachment 335078 [details] [review]: Just missing a test for the '*' case, otherwise looks good.
Created attachment 338672 [details] [review] Parse rtcp-fb media attributes Added '*' parsing for all formats
Created attachment 338673 [details] [review] Add tests for rtcp-fb parsing Added '*' parsing for all formats
(In reply to Olivier Crête from comment #15) > Review of attachment 335077 [details] [review] [review]: > > You're missing parsing of '*' meaning that it applies to all formats. Thank you, I have added parsing all formats now.
commit 028b16bb673649ac35ced3904abe3ca9cc130f98 Author: Tomasz Zajac <tomasz.zajac@motorolasolutions.com> Date: Fri Oct 28 08:47:40 2016 +0200 sdp: Add tests for rtcp-fb parsing https://bugzilla.gnome.org/show_bug.cgi?id=769698 commit 4d4f3c3cc4647b1534e3bbe857ecc4f7ebf8b979 Author: Tomasz Zajac <tomasz.zajac@motorolasolutions.com> Date: Fri Oct 28 08:47:01 2016 +0200 sdp: Parse rtcp-fb media attributes https://bugzilla.gnome.org/show_bug.cgi?id=769698 commit 5424357ffa6e041fe773a9b5551ec738ff1bdca2 Author: Tomasz Zajac <tomasz.zajac@motorolasolutions.com> Date: Wed Aug 10 11:38:58 2016 +0200 sdp: Add rtcp-fb media attributes based on caps https://bugzilla.gnome.org/show_bug.cgi?id=769698