FEC elements: document, remove irrelevant properties

The ulpfecenc "mux-seq" and "ssrc" properties were initially added
because the element did more than implement ULPFEC. As it was
decided that FLEXFEC would be implemented in a separate element,
both properties are now unneeded and confusing.

Change the default for the ulpfecenc multi-packet property,
as it is expected that most users of this element will be protecting video
streams.

Change the default property for the rtpredenc allow-no-red-blocks
property, as it should also be its default mode of operation.

https://bugzilla.gnome.org/show_bug.cgi?id=793843

6 files changed, 123 insertions, 55 deletions

diff --git a/gst/rtp/gstrtpreddec.c b/gst/rtp/gstrtpreddec.c
index 488a9350c..5f769ba3f 100644
--- a/gst/rtp/gstrtpreddec.c
+++ b/gst/rtp/gstrtpreddec.c
@@ -25,6 +25,18 @@
  *
  * Decode Redundant Audio Data (RED) as per RFC 2198.
  *
+ * This element is mostly provided for chrome webrtc compatibility:
+ * chrome will wrap ulpfec-protected streams in RED packets, and such
+ * streams need to be unwrapped by this element before being passed on
+ * to #GstRtpUlpFecDec.
+ *
+ * The #GstRtpRedDec:pt property should be set to the expected payload
+ * types of the RED packets.
+ *
+ * When using #GstRtpBin, this element should be inserted through the
+ * #GstRtpBin::request-fec-decoder signal.
+ *
+ * See also: #GstRtpRedEnc, #GstWebRTCBin, #GstRtpBin
  * Since: 1.14
  */
 
diff --git a/gst/rtp/gstrtpredenc.c b/gst/rtp/gstrtpredenc.c
index b3068bc8b..f1bf72c88 100644
--- a/gst/rtp/gstrtpredenc.c
+++ b/gst/rtp/gstrtpredenc.c
@@ -25,6 +25,20 @@
  *
  * Encode Redundant Audio Data (RED) as per RFC 2198.
  *
+ * This element is mostly provided for chrome webrtc compatibility:
+ * chrome expects protection packets generated by #GstRtpUlpFecEnc
+ * to be wrapped in RED packets for backward compatibility purposes,
+ * but does not actually make use of the redundant packets that could
+ * be encoded with this element.
+ *
+ * As such, when used for that purpose, only the #GstRtpRedEnc:pt property
+ * should be set to a payload type different from both the protected and
+ * protection packets' payload types.
+ *
+ * When using #GstRtpBin, this element should be inserted through the
+ * #GstRtpBin::request-fec-encoder signal.
+ *
+ * See also: #GstRtpRedDec, #GstWebRTCBin, #GstRtpBin
  * Since: 1.14
  */
 
@@ -54,7 +68,7 @@ static GstStaticPadTemplate src_template = GST_STATIC_PAD_TEMPLATE ("src",
 
 #define DEFAULT_PT                  (0)
 #define DEFAULT_DISTANCE            (0)
-#define DEFAULT_ALLOW_NO_RED_BLOCKS (FALSE)
+#define DEFAULT_ALLOW_NO_RED_BLOCKS (TRUE)
 
 GST_DEBUG_CATEGORY_STATIC (gst_rtp_red_enc_debug);
 #define GST_CAT_DEFAULT (gst_rtp_red_enc_debug)
diff --git a/gst/rtp/gstrtpstorage.c b/gst/rtp/gstrtpstorage.c
index 907380840..9efce4ef7 100644
--- a/gst/rtp/gstrtpstorage.c
+++ b/gst/rtp/gstrtpstorage.c
@@ -26,9 +26,30 @@
  * Helper element for storing packets to aid later packet recovery from packet
  * loss using RED/FEC (Forward Error Correction).
  *
- * This element is used internally by rtpbin and is usually created
- * automatically.
+ * The purpose of this element is to store a moving window of packets which
+ * downstream elements such as #GstRtpUlpFecDec can request in order to perform
+ * recovery of lost packets upon receiving custom GstRtpPacketLost events,
+ * usually from #GstRtpJitterBuffer.
  *
+ * As such, when building a pipeline manually, it should have the form:
+ *
+ * ```
+ * rtpstorage ! rtpjitterbuffer ! rtpulpfecdec
+ * ```
+ *
+ * where rtpulpfecdec get passed a reference to the object pointed to by
+ * the #GstRtpStorage:internal-storage property.
+ *
+ * The #GstRtpStorage:size-time property should be configured with a value
+ * equal to the #GstRtpJitterBuffer latency, plus some tolerance, in the order
+ * of milliseconds, for example in the example found at
+ * <https://github.com/sdroege/gstreamer-rs/blob/master/examples/src/bin/rtpfecclient.rs>,
+ * `size-time` is configured as 200 + 50 milliseconds (latency + tolerance).
+ *
+ * When using #GstRtpBin, a storage element is created automatically, and
+ * can be configured upon receiving the #GstRtpBin::new-storage signal.
+ *
+ * See also: #GstRtpBin, #GstRtpUlpFecDec
  * Since: 1.14
  */
 
diff --git a/gst/rtp/gstrtpulpfecdec.c b/gst/rtp/gstrtpulpfecdec.c
index eba34369c..aec7538ca 100644
--- a/gst/rtp/gstrtpulpfecdec.c
+++ b/gst/rtp/gstrtpulpfecdec.c
@@ -26,6 +26,25 @@
  * Generic Forward Error Correction (FEC) decoder for Uneven Level
  * Protection (ULP) as described in RFC 5109.
  *
+ * This element will work in combination with an upstream #GstRtpStorage
+ * element and attempt to recover packets declared lost through custom
+ * 'GstRTPPacketLost' events, usually emitted by #GstRtpJitterBuffer.
+ *
+ * As such, this element cannot be usefully used from the command line,
+ * because a reference to the upstream storage object needs to be
+ * provided to it through its #GstRtpUlpFecDec:storage property, example
+ * programs are available at
+ * <https://github.com/sdroege/gstreamer-rs/blob/master/examples/src/bin/rtpfecserver.rs>
+ * and
+ * <https://github.com/sdroege/gstreamer-rs/blob/master/examples/src/bin/rtpfecclient.rs>.
+ *
+ * Additionally, the payload types of the protection packets *must* be
+ * provided to this element via its #GstRtpUlpFecDec:pt property.
+ *
+ * When using #GstRtpBin, this element should be inserted through the
+ * #GstRtpBin::request-fec-decoder signal.
+ *
+ * See also: #GstRtpUlpFecEnc, #GstRtpBin, #GstRtpStorage
  * Since: 1.14
  */
 
diff --git a/gst/rtp/gstrtpulpfecenc.c b/gst/rtp/gstrtpulpfecenc.c
index aa8ac7b42..c9e68509a 100644
--- a/gst/rtp/gstrtpulpfecenc.c
+++ b/gst/rtp/gstrtpulpfecenc.c
@@ -26,6 +26,46 @@
  * Generic Forward Error Correction (FEC) encoder using Uneven Level
  * Protection (ULP) as described in RFC 5109.
  *
+ * This element will insert protection packets in any RTP stream, which
+ * can then be used on the receiving side to recover lost packets.
+ *
+ * This element rewrites packets' seqnums, which means that when combined
+ * with retransmission elements such as #GstRtpRtxSend, it *must* be
+ * placed upstream of those, otherwise retransmission requests will request
+ * incorrect seqnums.
+ *
+ * A payload type for the protection packets *must* be specified, different
+ * from the payload type of the protected packets, with the GstRtpUlpFecEnc:pt
+ * property.
+ *
+ * The marker bit of RTP packets is used to determine sets of packets to
+ * protect as a unit, in order to modulate the level of protection, this
+ * behaviour can be disabled with GstRtpUlpFecEnc:multipacket, but should
+ * be left enabled for video streams.
+ *
+ * The level of protection can be configured with two properties,
+ * #GstRtpUlpFecEnc:percentage and #GstRtpUlpFecEnc:percentage-important,
+ * the element will determine which percentage to use for a given set of
+ * packets based on the presence of the #GST_BUFFER_FLAG_NON_DROPPABLE
+ * flag, upstream payloaders are expected to set this flag on "important"
+ * packets such as those making up a keyframe.
+ *
+ * The percentage is expressed not in terms of bytes, but in terms of
+ * packets, this for implementation convenience. The drawback with this
+ * approach is that when using a percentage different from 100 %, and a
+ * low bitrate, entire frames may be contained in a single packet, leading
+ * to some packets not being protected, thus lowering the overall recovery
+ * rate on the receiving side.
+ *
+ * When using #GstRtpBin, this element should be inserted through the
+ * #GstRtpBin::request-fec-encoder signal.
+ *
+ * Example programs using this element can be found at
+ * <https://github.com/sdroege/gstreamer-rs/blob/master/examples/src/bin/rtpfecserver.rs>
+ * and
+ * <https://github.com/sdroege/gstreamer-rs/blob/master/examples/src/bin/rtpfecclient.rs>.
+ *
+ * See also: #GstRtpUlpFecDec, #GstRtpBin
  * Since: 1.14
  */
 
@@ -47,14 +87,11 @@ static GstStaticPadTemplate srctemplate = GST_STATIC_PAD_TEMPLATE ("src",
     GST_STATIC_CAPS ("application/x-rtp"));
 
 #define UNDEF_PT                255
-#define UNDEF_SSRC              0
 
 #define DEFAULT_PT              UNDEF_PT
-#define DEFAULT_SSRC            UNDEF_SSRC
 #define DEFAULT_PCT             0
 #define DEFAULT_PCT_IMPORTANT   0
-#define DEFAULT_MULTIPACKET     FALSE
-#define DEFAULT_MUX_SEQ         FALSE
+#define DEFAULT_MULTIPACKET     TRUE
 
 #define PACKETS_BUF_MAX_LENGTH  (RTP_ULPFEC_PROTECTED_PACKETS_MAX(TRUE))
 
@@ -66,10 +103,8 @@ G_DEFINE_TYPE (GstRtpUlpFecEnc, gst_rtp_ulpfec_enc, GST_TYPE_ELEMENT);
 enum
 {
   PROP_0,
-  PROP_SSRC,
   PROP_PT,
   PROP_MULTIPACKET,
-  PROP_MUX_SEQ,
   PROP_PROTECTED,
   PROP_PERCENTAGE,
   PROP_PERCENTAGE_IMPORTANT,
@@ -346,16 +381,13 @@ gst_rtp_ulpfec_enc_stream_ctx_cache_packet (GstRtpUlpFecEncStreamCtx * ctx,
 
 static void
 gst_rtp_ulpfec_enc_stream_ctx_configure (GstRtpUlpFecEncStreamCtx * ctx,
-    guint pt, guint32 fec_ssrc,
-    guint percentage, guint percentage_important, gboolean multipacket,
-    gboolean mux_seq)
+    guint pt, guint percentage, guint percentage_important,
+    gboolean multipacket)
 {
   ctx->pt = pt;
-  ctx->fec_ssrc = fec_ssrc;
   ctx->percentage = percentage;
   ctx->percentage_important = percentage_important;
   ctx->multipacket = multipacket;
-  ctx->mux_seq = mux_seq;
 
   ctx->fec_nth = percentage ? 100 / percentage : 0;
   if (percentage) {
@@ -375,9 +407,8 @@ gst_rtp_ulpfec_enc_stream_ctx_configure (GstRtpUlpFecEncStreamCtx * ctx,
 static GstRtpUlpFecEncStreamCtx *
 gst_rtp_ulpfec_enc_stream_ctx_new (guint ssrc,
     GstElement * parent, GstPad * srcpad,
-    guint pt, guint32 fec_ssrc,
-    guint percentage, guint percentage_important, gboolean multipacket,
-    gboolean mux_seq)
+    guint pt, guint percentage, guint percentage_important,
+    gboolean multipacket)
 {
   GstRtpUlpFecEncStreamCtx *ctx = g_new0 (GstRtpUlpFecEncStreamCtx, 1);
 
@@ -392,8 +423,8 @@ gst_rtp_ulpfec_enc_stream_ctx_new (guint ssrc,
       (GDestroyNotify) rtp_ulpfec_map_info_unmap);
   ctx->parent = parent;
   ctx->scratch_buf = g_array_new (FALSE, TRUE, sizeof (guint8));
-  gst_rtp_ulpfec_enc_stream_ctx_configure (ctx, pt, fec_ssrc,
-      percentage, percentage_important, multipacket, mux_seq);
+  gst_rtp_ulpfec_enc_stream_ctx_configure (ctx, pt,
+      percentage, percentage_important, multipacket);
 
   return ctx;
 }
@@ -425,7 +456,7 @@ gst_rtp_ulpfec_enc_stream_ctx_process (GstRtpUlpFecEncStreamCtx * ctx,
 
   ctx->num_packets_received++;
 
-  if (ctx->mux_seq && ctx->seqnum_offset > 0) {
+  if (ctx->seqnum_offset > 0) {
     buffer = gst_buffer_make_writable (buffer);
     if (!gst_rtp_buffer_map (buffer,
             GST_MAP_READWRITE | GST_RTP_BUFFER_MAP_FLAG_SKIP_PADDING, &rtp))
@@ -443,11 +474,8 @@ gst_rtp_ulpfec_enc_stream_ctx_process (GstRtpUlpFecEncStreamCtx * ctx,
 
   if (push_fec) {
     guint32 fec_timestamp = gst_rtp_buffer_get_timestamp (&rtp);
-    guint32 fec_ssrc =
-        ctx->fec_ssrc ==
-        UNDEF_SSRC ? gst_rtp_buffer_get_ssrc (&rtp) : ctx->fec_ssrc;
-    guint16 fec_seq =
-        ctx->mux_seq ? gst_rtp_buffer_get_seq (&rtp) + 1 : ctx->seqnum;
+    guint32 fec_ssrc = gst_rtp_buffer_get_ssrc (&rtp);
+    guint16 fec_seq = gst_rtp_buffer_get_seq (&rtp) + 1;
 
     gst_rtp_buffer_unmap (&rtp);
 
@@ -477,8 +505,8 @@ gst_rtp_ulpfec_enc_aquire_ctx (GstRtpUlpFecEnc * fec, guint ssrc)
   if (ctx == NULL) {
     ctx =
         gst_rtp_ulpfec_enc_stream_ctx_new (ssrc, GST_ELEMENT_CAST (fec),
-        fec->srcpad, fec->pt, fec->ssrc, fec->percentage,
-        fec->percentage_important, fec->multipacket, fec->mux_seq);
+        fec->srcpad, fec->pt, fec->percentage,
+        fec->percentage_important, fec->multipacket);
     g_hash_table_insert (fec->ssrc_to_ctx, GUINT_TO_POINTER (ssrc), ctx);
   }
   GST_OBJECT_UNLOCK (fec);
@@ -524,9 +552,8 @@ gst_rtp_ulpfec_enc_configure_ctx (gpointer key, gpointer value,
   GstRtpUlpFecEnc *fec = user_data;
   GstRtpUlpFecEncStreamCtx *ctx = value;
 
-  gst_rtp_ulpfec_enc_stream_ctx_configure (ctx, fec->pt, fec->ssrc,
-      fec->percentage, fec->percentage_important,
-      fec->multipacket, fec->mux_seq);
+  gst_rtp_ulpfec_enc_stream_ctx_configure (ctx, fec->pt,
+      fec->percentage, fec->percentage_important, fec->multipacket);
 }
 
 static void
@@ -539,9 +566,6 @@ gst_rtp_ulpfec_enc_set_property (GObject * object, guint prop_id,
     case PROP_PT:
       fec->pt = g_value_get_uint (value);
       break;
-    case PROP_SSRC:
-      fec->ssrc = g_value_get_uint (value);
-      break;
     case PROP_MULTIPACKET:
       fec->multipacket = g_value_get_boolean (value);
       break;
@@ -551,9 +575,6 @@ gst_rtp_ulpfec_enc_set_property (GObject * object, guint prop_id,
     case PROP_PERCENTAGE_IMPORTANT:
       fec->percentage_important = g_value_get_uint (value);
       break;
-    case PROP_MUX_SEQ:
-      fec->mux_seq = g_value_get_boolean (value);
-      break;
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
       break;
@@ -574,9 +595,6 @@ gst_rtp_ulpfec_enc_get_property (GObject * object, guint prop_id,
     case PROP_PT:
       g_value_set_uint (value, fec->pt);
       break;
-    case PROP_SSRC:
-      g_value_set_uint (value, fec->ssrc);
-      break;
     case PROP_PROTECTED:
       g_value_set_uint (value, fec->num_packets_protected);
       break;
@@ -589,9 +607,6 @@ gst_rtp_ulpfec_enc_get_property (GObject * object, guint prop_id,
     case PROP_MULTIPACKET:
       g_value_set_boolean (value, fec->multipacket);
       break;
-    case PROP_MUX_SEQ:
-      g_value_set_boolean (value, fec->mux_seq);
-      break;
     default:
       G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
       break;
@@ -650,11 +665,6 @@ gst_rtp_ulpfec_enc_class_init (GstRtpUlpFecEncClass * klass)
       GST_DEBUG_FUNCPTR (gst_rtp_ulpfec_enc_get_property);
   gobject_class->dispose = GST_DEBUG_FUNCPTR (gst_rtp_ulpfec_enc_dispose);
 
-  g_object_class_install_property (gobject_class, PROP_SSRC,
-      g_param_spec_uint ("ssrc", "SSRC",
-          "The SSRC to use on FEC'd packets", 0, G_MAXUINT32, DEFAULT_SSRC,
-          G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
-
   g_object_class_install_property (gobject_class, PROP_PT,
       g_param_spec_uint ("pt", "payload type",
           "The payload type of FEC packets", 0, 255, DEFAULT_PT,
@@ -665,12 +675,6 @@ gst_rtp_ulpfec_enc_class_init (GstRtpUlpFecEncClass * klass)
           "Apply FEC on multiple packets", DEFAULT_MULTIPACKET,
           G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
 
-  g_object_class_install_property (gobject_class, PROP_MUX_SEQ,
-      g_param_spec_boolean ("mux-seq", "Mux seq",
-          "Mux seqnum for media and fec packets in same seqnum space",
-          DEFAULT_MUX_SEQ,
-          G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
-
   g_object_class_install_property (gobject_class, PROP_PERCENTAGE,
       g_param_spec_uint ("percentage", "Percentage",
           "FEC overhead percentage for the whole stream", 0, 100, DEFAULT_PCT,
diff --git a/gst/rtp/gstrtpulpfecenc.h b/gst/rtp/gstrtpulpfecenc.h
index 82ff52291..885c6ad61 100644
--- a/gst/rtp/gstrtpulpfecenc.h
+++ b/gst/rtp/gstrtpulpfecenc.h
@@ -56,7 +56,6 @@ struct _GstRtpUlpFecEnc {
   guint percentage;
   guint percentage_important;
   gboolean multipacket;
-  gboolean mux_seq;
   guint num_packets_protected;
 };
 
@@ -68,7 +67,6 @@ typedef struct {
 
   /* settings */
   guint pt;
-  guint32 fec_ssrc;
   guint percentage;
   guint percentage_important;
   gboolean multipacket;