GNOME Bugzilla – Bug 794911
Implement support for ULP Forward Error Correction
Last modified: 2018-04-19 16:26:31 UTC
See commit message
Created attachment 370453 [details] [review] Implement support for ULP Forward Error Correction In this initial commit, interface is only exposed for RECORD, further work will be needed in rtspsrc to support this for PLAY.
(In reply to Mathieu Duponchelle from comment #1) > Created attachment 370453 [details] [review] [review] > Implement support for ULP Forward Error Correction > > In this initial commit, interface is only exposed for RECORD, > further work will be needed in rtspsrc to support this for > PLAY. But we could expose it also already for PLAY, just that rtspsrc would need some changes to handle it? Or are there other problems?
Review of attachment 370453 [details] [review]: Are we always sending FEC now if requested on the media? Or only if the client (for PLAY) requests that from the SDP? ::: gst/rtsp-server/rtsp-media.c @@ +1519,3 @@ + i, &storage); + if (storage) + g_object_set (storage, "size-time", (media->priv->latency + 50) * GST_MSECOND, NULL); Should this 50ms be configurable? @@ +3132,3 @@ +new_storage_cb (GstElement * rtpbin, GObject * storage, guint sessid, GstRTSPMedia * media) +{ + g_object_set (storage, "size-time", (media->priv->latency + 50) * GST_MSECOND, NULL); And this 50ms? ::: gst/rtsp-server/rtsp-stream.c @@ +5019,3 @@ + + +/** Double new-line above ::: gst/rtsp-server/rtsp-stream.h @@ +310,3 @@ +/* ULP Forward Error Correction (RFC 5109) */ +GST_RTSP_SERVER_API +gboolean gst_rtsp_stream_get_ulpfec_enabled (GstRTSPStream *stream); I wonder if some more generic API could be added here, similar to what is in rtpbin. So that we won't have to add yet another set of functions for flexfec later.
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 370453 [details] [review] [review]: > > Are we always sending FEC now if requested on the media? Or only if the > client (for PLAY) requests that from the SDP? > So with this patch, only the RECORD case is implemented, and rtspclientsink doesn't use the media API if that's what you're asking. In that case FEC is only used on the client-side if it was requested through the new ulpfec-percentage property of rtspclientsink, and on the server side we only instantiate a decoder and tweak the storage properties if FEC was present in the ANNOUNCE request. I assume for PLAY, the client (rtspsrc) would not have a say on whether FEC is used or not, it would simply be informed about it in the answer to the DESCRIBE request. Does that answer yyour question? :) > ::: gst/rtsp-server/rtsp-media.c > @@ +1519,3 @@ > + i, &storage); > + if (storage) > + g_object_set (storage, "size-time", (media->priv->latency + 50) * > GST_MSECOND, NULL); > > Should this 50ms be configurable? > > @@ +3132,3 @@ > +new_storage_cb (GstElement * rtpbin, GObject * storage, guint sessid, > GstRTSPMedia * media) > +{ > + g_object_set (storage, "size-time", (media->priv->latency + 50) * > GST_MSECOND, NULL); > > And this 50ms? This is a tad ugly, but no, this should not be configurable, we just want the duration of the lookback window in the storage to be a tad higher than the latency, otherwise by the time we get the PacketLost events in the ulpfec decoder, the storage might have disposed of the relevant packets, 50 ms is usually a safe value, though it hasn't been determined in an extremely scientific manner :) > > ::: gst/rtsp-server/rtsp-stream.c > @@ +5019,3 @@ > + > + > +/** > > Double new-line above > > ::: gst/rtsp-server/rtsp-stream.h > @@ +310,3 @@ > +/* ULP Forward Error Correction (RFC 5109) */ > +GST_RTSP_SERVER_API > +gboolean gst_rtsp_stream_get_ulpfec_enabled (GstRTSPStream > *stream); > > I wonder if some more generic API could be added here, similar to what is in > rtpbin. So that we won't have to add yet another set of functions for > flexfec later. I wondered about that too, then opted for the less generic choice, because I'm not really sure what the API will have to look like for flex, specifically because of the fact multiple streams can be protected by the same flex protection stream.
(In reply to Mathieu Duponchelle from comment #4) > (In reply to Sebastian Dröge (slomo) from comment #3) > > Review of attachment 370453 [details] [review] [review] [review]: > > > > Are we always sending FEC now if requested on the media? Or only if the > > client (for PLAY) requests that from the SDP? > > > > So with this patch, only the RECORD case is implemented, and rtspclientsink > doesn't use the media API if that's what you're asking. In that case FEC is > only used on the client-side if it was requested through the new > ulpfec-percentage property of rtspclientsink, and on the server side we only > instantiate a decoder and tweak the storage properties if FEC was present in > the ANNOUNCE request. > > I assume for PLAY, the client (rtspsrc) would not have a say on whether FEC > is used or not, it would simply be informed about it in the answer to the > DESCRIBE request. > > Does that answer yyour question? :) Yeah, does that work for PLAY already then? Probably not as we don't add the relevant pieces to the SDP? > > ::: gst/rtsp-server/rtsp-media.c > > @@ +1519,3 @@ > > + i, &storage); > > + if (storage) > > + g_object_set (storage, "size-time", (media->priv->latency + 50) * > > GST_MSECOND, NULL); > > > > Should this 50ms be configurable? > > > > @@ +3132,3 @@ > > +new_storage_cb (GstElement * rtpbin, GObject * storage, guint sessid, > > GstRTSPMedia * media) > > +{ > > + g_object_set (storage, "size-time", (media->priv->latency + 50) * > > GST_MSECOND, NULL); > > > > And this 50ms? > > This is a tad ugly, but no, this should not be configurable, we just want > the duration of the lookback window in the storage to be a tad higher than > the latency, otherwise by the time we get the PacketLost events in the > ulpfec decoder, the storage might have disposed of the relevant packets, 50 > ms is usually a safe value, though it hasn't been determined in an extremely > scientific manner :) For RTX this is configurable though, it's the same thing there right? > > ::: gst/rtsp-server/rtsp-stream.c > > @@ +5019,3 @@ > > + > > + > > +/** > > > > Double new-line above > > > > ::: gst/rtsp-server/rtsp-stream.h > > @@ +310,3 @@ > > +/* ULP Forward Error Correction (RFC 5109) */ > > +GST_RTSP_SERVER_API > > +gboolean gst_rtsp_stream_get_ulpfec_enabled (GstRTSPStream > > *stream); > > > > I wonder if some more generic API could be added here, similar to what is in > > rtpbin. So that we won't have to add yet another set of functions for > > flexfec later. > > I wondered about that too, then opted for the less generic choice, because > I'm not really sure what the API will have to look like for flex, > specifically because of the fact multiple streams can be protected by the > same flex protection stream. Ok then :)
(In reply to Sebastian Dröge (slomo) from comment #5) > (In reply to Mathieu Duponchelle from comment #4) > Yeah, does that work for PLAY already then? Probably not as we don't add the > relevant pieces to the SDP? No, it doesn't do anything for PLAY yet. > For RTX this is configurable though, it's the same thing there right? Sort of, for rtxsend you're safe enough setting it to be strictly equal to the client-side latency, as any request beyond that will be too late, for the rtpstorage it's a bit different as we request packets from the storage just as we get packet lost events, which is why we must have some tolerance on top of that, hope I'm explaining this well enough :) > w> > > ::: gst/rtsp-server/rtsp-stream.c > > > @@ +5019,3 @@ > > > + > > > + > > > +/** > > > > > > Double new-line above > > > > > > ::: gst/rtsp-server/rtsp-stream.h > > > @@ +310,3 @@ > > > +/* ULP Forward Error Correction (RFC 5109) */ > > > +GST_RTSP_SERVER_API > > > +gboolean gst_rtsp_stream_get_ulpfec_enabled (GstRTSPStream > > > *stream); > > > > > > I wonder if some more generic API could be added here, similar to what is in > > > rtpbin. So that we won't have to add yet another set of functions for > > > flexfec later. > > > > I wondered about that too, then opted for the less generic choice, because > > I'm not really sure what the API will have to look like for flex, > > specifically because of the fact multiple streams can be protected by the > > same flex protection stream. > > Ok then :)
Comment on attachment 370453 [details] [review] Implement support for ULP Forward Error Correction Let's get it in then. For PLAY please open another bug unless you plan to implement it soonish (and won't forget :) )
Attachment 370453 [details] pushed as bfc35ae - Implement support for ULP Forward Error Correction