GNOME Bugzilla – Bug 792696
rtp: Implement support for generic RTP Forward Error Correction
Last modified: 2018-05-03 06:17:43 UTC
While this commit set does not implement FLEX FEC, the implementation and interface have been designed with this requirement in mind.
Created attachment 367099 [details] [review] rtpptdemux: Add ssrc to output caps It may be useful downstream
Created attachment 367100 [details] [review] rtpptdemux: Add ignored-payload-types property Packets with these payload types will be dropped. A use case for this is FEC, where we want FEC packets to go through the jitterbuffer, but not be output by rtpbin.
Created attachment 367101 [details] [review] rtp: Implement ULPFEC (RFC 5109) We expose a set of new elements: * ULPFEC encoder / decoder * A storage element, which should be placed before jitterbuffers, and is used to store packets in order to attempt reconstruction after the jitterbuffer has sent PacketLost events * RED encoder / decoder (RFC 2198), these are necessary to use FEC in webrtc, as browsers will propose and expect ulpfec packets to be wrapped in red packets With contributions from: Mathieu Duponchelle <mathieu@centricular.com> Sebastian Dröge <sebastian@centricular.com>
Created attachment 367102 [details] [review] rtpbin: Expose FEC support signals Also slightly refactor complete_session_src
A rust example has been proposed at https://github.com/sdroege/gstreamer-rs/pull/73
Created attachment 367584 [details] [review] rtp: Implement ULPFEC (RFC 5109) We expose a set of new elements: * ULPFEC encoder / decoder * A storage element, which should be placed before jitterbuffers, and is used to store packets in order to attempt reconstruction after the jitterbuffer has sent PacketLost events * RED encoder / decoder (RFC 2198), these are necessary to use FEC in webrtc, as browsers will propose and expect ulpfec packets to be wrapped in red packets With contributions from: Mathieu Duponchelle <mathieu@centricular.com> Sebastian Dröge <sebastian@centricular.com>
Created attachment 367585 [details] [review] rtpbin: Expose FEC support signals Also slightly refactor complete_session_src
I know this is a pretty large change set, but we could maybe limit the scope of the review to API concerns to make things easier?
Created attachment 367645 [details] [review] rtp: Implement ULPFEC (RFC 5109) We expose a set of new elements: * ULPFEC encoder / decoder * A storage element, which should be placed before jitterbuffers, and is used to store packets in order to attempt reconstruction after the jitterbuffer has sent PacketLost events * RED encoder / decoder (RFC 2198), these are necessary to use FEC in webrtc, as browsers will propose and expect ulpfec packets to be wrapped in red packets With contributions from: Mathieu Duponchelle <mathieu@centricular.com> Sebastian Dröge <sebastian@centricular.com>
Created attachment 367646 [details] [review] rtpbin: Expose FEC support signals Also slightly refactor complete_session_src
If no one has any objections, I propose we merge this, it's already been reviewed by a few different people (at pexip where it's been in use for quite some time, Sebastian and I)
+1
Yes, I think we should get it in. The API surface is quite small and the code base is battle tested. Only niggle I have is the name of that "percentage-imp" property ;)
Attachment 367099 [details] pushed as 36b991f - rtpptdemux: Add ssrc to output caps Attachment 367100 [details] pushed as 82d0950 - rtpptdemux: Add ignored-payload-types property Attachment 367645 [details] pushed as d5ad50b - rtp: Implement ULPFEC (RFC 5109) Attachment 367646 [details] pushed as fdf6419 - rtpbin: Expose FEC support signals
Review of attachment 367100 [details] [review]: ::: gst/rtpmanager/gstrtpptdemux.c @@ -202,0 +240,5 @@ + gobject_klass->set_property = gst_rtp_pt_demux_set_property; + gobject_klass->get_property = gst_rtp_pt_demux_get_property; + ... 2 more ... I'm not sure how, but if we could add a since marker it would be nice.
Review of attachment 367646 [details] [review]: ::: gst/rtpmanager/gstrtpbin.c @@ +2116,3 @@ + * @id: the session id + * + * Request the internal RTPStorage object as #GObject in session @id. Missing Since 1.14 here.
Review of attachment 367645 [details] [review]: These elements seems to be missing the documentation for element section and examples, and are missing from the generated documentation.
Would it be possible to document before the next release ?
Will do
(In reply to Nicolas Dufresne (stormer) from comment #18) > Would it be possible to document before the next release ? Done, thanks for pointing out!
(In reply to Nicolas Dufresne (stormer) from comment #15) > Review of attachment 367100 [details] [review] [review]: > > ::: gst/rtpmanager/gstrtpptdemux.c > @@ -202,0 +240,5 @@ > + gobject_klass->set_property = gst_rtp_pt_demux_set_property; > + gobject_klass->get_property = gst_rtp_pt_demux_get_property; > + > ... 2 more ... > > I'm not sure how, but if we could add a since marker it would be nice. Done (In reply to Nicolas Dufresne (stormer) from comment #16) > Review of attachment 367646 [details] [review] [review]: > > ::: gst/rtpmanager/gstrtpbin.c > @@ +2116,3 @@ > + * @id: the session id > + * > + * Request the internal RTPStorage object as #GObject in session @id. > > Missing Since 1.14 here. Done too, thanks!