[Bug 761947] rtpbasepayload: rtpbasedepayload: Add source-info property

[GStreamer (GNOME Bugzilla)]

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

--- Comment #13 from Stian Selnes (stianse) <stian.selnes at gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #12)
> Review of attachment 339241 [details] [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?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.