Review of attachment 320987 [details] [review]:

Maybe it makes sense to add more information from the RTP header here while we're at it?

::: gst-libs/gst/rtp/gstrtpbasedepayload.c
@@ +758,3 @@
+    return;
+
+  ssrc = g_new (guint32, 1);

Why allocate that here on the heap?

::: gst-libs/gst/rtp/gstrtpbasedepayload.h
@@ +59,3 @@
 
+  gboolean source_info;
+

Breaks ABI

::: gst-libs/gst/rtp/gstrtpbasepayload.h
@@ +99,3 @@
   guint    current_ssrc;
   guint    mtu;
+  gboolean source_info;

Breaks ABI

::: gst-libs/gst/rtp/gstrtpmeta.c
@@ +34,3 @@
+ * Returns: (transfer none): the #GstRTPSourceMeta on @buffer.
+ *
+ * Since: 1.8

Everything should be Since: 1.12 now

@@ +99,3 @@
+
+    /* FIXME: HACK: Buffer should be writable at this point. */
+    if (!gst_buffer_is_writable (dst))

So make it a g_warning() or g_return_val_if_fail()

::: gst-libs/gst/rtp/gstrtpmeta.h
@@ +44,3 @@
+  GstMeta meta;
+
+  guint32 *ssrc;

Why a pointer if it can only be one?

Not sure about the context, but maybe this is useful -- https://bugzilla.gnome.org/show_bug.cgi?id=762628

Let be elaborate a bit on the context that the proposed source-info feature solves and the difference in regards to 762628 mentioned above.

GStreamer is currently lacking a way to propagate SSRC information from incoming streams and add that as CSRC to an outgoing stream in order to attach the sources that has contributed to a combined stream. Let me just copy-paste our typical scenario which is also described in RFC 3550: "An example application is audio conferencing where a mixer indicates all the talkers whose speech was combined to produce the outgoing packet, allowing the receiver to indicate the current talker, even though all the audio packets contain the same SSRC identifier (that of the mixer)." The same is applicable to video of course.

In order to achieve this functionality it's necessary to extract and propagate the SSRC and CSRCs from the depayloader, forward this meta downstream and possibly aggregate multiple metas into one, and finally have the payloader add the contributing sources as CSRCs.

In addition to the case "depayloader ! mixer ! payloader" it's possible that the application wants to add a specific CSRC on a stream. With the proposed patch this can be achieved for example with a probe or element that adds meta to the buffer before payloading.

The proposed patch adds a property to the depayloader that makes it add the SSRC and CSRC as meta, and the payloader will add (if enabled) CSRCs to the packets from this meta. The other patch mentioned above targets a different use case. It can be used to pass SSRC and CSRC from the depayloader, but it makes less sense to aggregate and payload a full RTP header meta like that. Thus, I think a RTPSourceMeta is better suited to implement the missing CSRC functionality.

I'll update the patch according to the comments.

Regarding why store ssrc as a pointer instead of just a guint32: It is to allow the meta to have ssrc=NULL in case the meta is added by someone else than the depayloader. Not sure if it's strictly necessary, but it felt more correct at the time. I'll revisit and see if I still think so.

Created attachment 338605 [details] [review]
rtpbasepayload: rtpbasedepayload: Add source-info property

Add a source-info property that will read/write meta to the buffers
about RTP source information. The GstRTPSourceMeta can be used to
transport information about the origin of a buffer, e.g. the sources
that is included in a mixed audio buffer.

A new function gst_rtp_base_payload_allocate_output_buffer() is added
for payloaders to use to allocate the output RTP buffer with the correct
number of CSRCs according to the meta and fill it.

RTPSourceMeta does not make sense on RTP buffers since the information
is in the RTP header. So the payloader will strip the meta from the
output buffer.

I've updated the patch and fixed version, ABI compability and removed the writable check as gst_buffer_add_meta already does that.

Here's why ssrc is a pointer and it makes sense to allow it to be NULL, and also why we shouldn't use a generic RTP header meta for this use case: As mentioned earlier the typical use case is to have a mixer that creates a combined stream and puts the contributing sources in the CSRC. In practice that means that GstAggregator or something equivalent will take the meta of the incoming buffers and create a new combined meta for the outgoing buffer. Generally it does not make sense to combine all RTP header information, but it DOES makes sense to aggregate the sources. The outgoing buffer should have meta with ssrc=NULL (as anything else doesn't make sense to me) and have the contributing sources added to the list of csrc.

Review of attachment 338605 [details] [review]:

Thanks for all the explanations, makes sense to me. Having yet another allocation for the ssrc is annoying though, I'd rather prefer another boolean in there if there is no "invalid ssrc" value.

::: gst-libs/gst/rtp/gstrtpmeta.c
@@ +1,2 @@
+/* GStreamer
+ * Copyright (C) <2015> Stian Selnes <stian@pexip.com>

We have 2016, and it's almost over already :)

@@ +153,3 @@
+  (void)meta;
+  (void)params;
+  (void)buffer;

You need to initialize all your fields here, to zeroes for example.

Created attachment 338987 [details] [review]
rtpbasepayload: rtpbasedepayload: Add source-info property

Updated patch with the following changes:

* ssrc and csrc is not allocated separately anymore. Changed struct,
  API and documentation accordingly. (It became neater, I have to
  admit.)

* Removed gst_rtp_source_meta_free since nothing needs to be freed
  anymore.

* Updated copyrights.

I wonder if there also should be a helper function in GstRTPBasePayload that returns how many csrcs it will add, because payloaders often need to use gst_rtp_buffer_calc_payload_len(mtu, pad, csrc_count) to find the maximum payload size. You can hold off on committing this patch until I have revisited (once again).

Ok, let's wait until you did that then :)

Created attachment 339241 [details] [review]
rtpbasepayload: rtpbasedepayload: Add source-info property

Updated patch with the following changes:

* Added function gst_rtp_base_payload_get_source_count(). Name is
  source_count, not csrc_count, since it counts all sources it finds
  in the meta. Useful for finding the csrc_count that is passed to
  various gst_rtp_buffer_calc functions.

* Update GstRTPBaseAudioPayload to use
  gst_rtp_base_payload_get_source_count() so that max payload size is
  calculated correctly. If not the resulting packets could be slightly
  larger than the MTU.

* Added a test suite for GstRTPSourceMeta

Review of attachment 339241 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbaseaudiopayload.c
@@ +892,3 @@
+  if (!gst_rtp_base_audio_payload_get_lengths (basepayload,
+          gst_rtp_base_payload_get_source_count (basepayload, buffer),
+          &min_payload_len, &max_payload_len, &align))

This probably needs similar changes in all other payloaders?

::: gst-libs/gst/rtp/gstrtpbasedepayload.h
@@ +122,3 @@
+gboolean        gst_rtp_base_depayload_is_source_info_enabled  (GstRTPBaseDepayload * depayload);
+void            gst_rtp_base_depayload_set_source_info_enabled (GstRTPBaseDepayload * depayload,
+                                                                gboolean enable);

getters and setters should be named after the property: is_source_info() or get_source_info() and set_source_info(). You probably want to rename the property name to something more meaningful :)

::: gst-libs/gst/rtp/gstrtpbasepayload.c
@@ +1453,3 @@
+
+  if (buffer == NULL)
+    buffer = gst_rtp_buffer_new_allocate (payload_len, pad_len, csrc_count);

If we do all this, it would make sense to do also the allocation query dance and use the provided allocator here

::: gst-libs/gst/rtp/gstrtpbasepayload.h
@@ +168,3 @@
+GstBuffer *     gst_rtp_base_payload_allocate_output_buffer (GstRTPBasePayload * payload,
+                                                             guint payload_len, guint8 pad_len,
+                                                             guint8 csrc_count);

I see us adding another allocation function later that has another parameter, and then yet another one later... :) Can you think of anything else now already?

::: gst-libs/gst/rtp/gstrtpmeta.h
@@ +29,3 @@
+typedef struct _GstRTPSourceMeta GstRTPSourceMeta;
+
+#define GST_RTP_SOURCE_META_MAX_CSRC_COUNT 15

Where does the 15 come from?

(In reply to Sebastian Dröge (slomo) from comment #12)
> Review of attachment 339241 [details] [review] [review]:
> 
> This probably needs similar changes in all other payloaders?

Yes, and I will take on updating more payloaders when the feature is committed. But it's important to note that payloaders will still work as before until they are updated, they just won't add RTP source information automatically.

> getters and setters should be named after the property: is_source_info() or
> get_source_info() and set_source_info(). You probably want to rename the
> property name to something more meaningful :)

The naming of the functions follows the convention of the "qos", "async", "last-sample" property of GstBaseSink and GstBaseTransform. But there are examples of what you suggest also, such as the "live" property. It doesn't seem very consistent.

Maybe the property name "source-info" is not super self-describing and obvious to what it enables. But I would really like the payloader and depayloader to have the same name so that it's clear that they are counterparts of each other. Naming is hard :-p

> If we do all this, it would make sense to do also the allocation query dance
> and use the provided allocator here

The gst_rtp_buffer-functions such as gst_rtp_buffer_new_allocate does not take an allocator, they use the default one. So even if we have a provided allocator, we cannot use it with the current gst_rtp_buffer-API. If we want to change that it's a separate patch that includes gst_rtp_buffer changes.

> I see us adding another allocation function later that has another
> parameter, and then yet another one later... :) Can you think of anything
> else now already?

The required arguments are for finding the size of the RTP packets, so they should define the length of the variable-length fields of an RTP packet. The only variable-length field missing here is the optional header extension. Currently the API for allocating extension header is designed to be used after the initial packet has been created. So today it wouldn't give us any benefit to add extension-length as an argument, and the current arguments aligns with the gst_rtp_buffer-API. So in order to get a benefit (in the future) when using an extension header, new gst_rtp_buffer-functions need to be added to the API. My two cents is that we keep this function aligned with gst_rtp_buffer-functions, and if necessary in the future we'll add new functions to both the base payloader and gst_rtp_buffer.

> +#define GST_RTP_SOURCE_META_MAX_CSRC_COUNT 15
> 
> Where does the 15 come from?

The max number of CSRCs allowed in a RTP packet.

Would you still like me to change anything with this patch?

Created attachment 370851 [details] [review]
updated patch

Rebased the patch on latest master and fixed up quite a few things.

This dealing with and using CSRCs as a concept, this has proved immensely useful, as both Google and Microsoft (and Pexip) uses this information to indicate who is in the mix at any given time.

ping?

It seems good, so I merged it. I also fixed a couple little things, added it to the doc, updated the since to 1.16, added rtpmeta.h to rtp.h, and updated the unit tests to use rtp.h