Created attachment 367097 [details] [review]
rtpbuffer.h: Add new RTPBuffer flags

Comment on attachment 367097 [details] [review]
rtpbuffer.h: Add new RTPBuffer flags

+1 Looks good to me, apart from these nitpicks:

>+ * @GST_RTP_BUFFER_FLAG_REDUNDANT:      The packet represents redundant RTP packet.
>+ *           The flag is used in gstrtpstorage to be able to hold the packetback
>+ *           and use it only for recovery from packet loss.
>+ * @GST_RTP_BUFFER_FLAG_IMPORTANT:      The packet is essential for correct
>+ *           decoding of the stream, and should for example be protected as well
>+ *           as possible when performing Forward Error Correction (FEC)
>+ * @GST_RTP_BUFFER_FLAG_IGNORABLE:  The packet is not essential for correct
>+ *           decoding of the stream, and can for example be safely ignored by FEC
>+ *           mechanisms.

Please add a "(Since: 1.14)" at the end of each of the descriptions for the new enums. 

> typedef enum {
>   GST_RTP_BUFFER_FLAG_RETRANSMISSION = (GST_BUFFER_FLAG_LAST << 0),
>+  GST_RTP_BUFFER_FLAG_REDUNDANT      = (GST_BUFFER_FLAG_LAST << 1),
>+  GST_RTP_BUFFER_FLAG_IMPORTANT  = (GST_BUFFER_FLAG_LAST << 2),
>+  GST_RTP_BUFFER_FLAG_IGNORABLE  = (GST_BUFFER_FLAG_LAST << 3),
>   GST_RTP_BUFFER_FLAG_LAST           = (GST_BUFFER_FLAG_LAST << 8)
> } GstRTPBufferFlags;

Could we align the '=' here with the others? :)

Review of attachment 367097 [details] [review]:

The IGNORABLE & IMPORTANT flags don't seem specific to RTP at all. I wonder if we should instead put them in GstBufferFlags. This way, the encoder or parser can mark the buffers directly if possible. Are you implemented ulpfec? In that case, you may also want to only mark a part of a buffer as important and we'd probably need a new Meta for this.. So maybe this should be a Meta instead ?

::: gst-libs/gst/rtp/gstrtpbuffer.h
@@ +241,3 @@
+ * @GST_RTP_BUFFER_FLAG_IGNORABLE:  The packet is not essential for correct
+ *           decoding of the stream, and can for example be safely ignored by FEC
+ *           mechanisms.

Isn't this the same as GST_BUFFER_FLAG_DROPPABLE ? It seems like a duplicate

Good call re. core buffer flags, that could indeed go there instead.

Bug #685279 also needs addition of some kind of IMPORTANT/DO_NOT_DROP buffer flag to make sure still video frames in DVD menus aren't accidentally dropped because of QoS.

I remember the 'mark part of a buffer as important (or more important)' thing in ulpfec coming up in discussions. I believe it was said that no one was using that yet, so it seems premature to add anything for it now.

I think we can go a long way with the two proposed buffer flags, and I think we should only add a full-fledged meta once we have a clearer idea what exactly is needed and we have something using it. Premature featurisation being the root of all evil etc.

Also, Metas are much heavier than flags (1-N allocs/frees), so it would be really good to avoid them, especially for things like RTP buffers, unless absolutely needed.

Created attachment 367583 [details] [review]
rtpbuffer.h: Add new RTPBuffer flags

These flags will be used for Forward Error Correction purposes

Followed up with https://bugzilla.gnome.org/show_bug.cgi?id=793008

Question is if we still want RTP_FEC specific flag defines that map to the core ones.

I think it might be nice for readability/discoverability, but we can always add that later.

So let's close this as we have the core flags now and can work with those.

That shouldn't be closed, we still need the REDUNDANT flag, reopening :)

I thought REDUNDANT = DROPPABLE ?

No, that is different, this flag is for RED packets as potentially output by reddec

Ah, sorry :)

Attachment 367583 [details] pushed as d7397f0 - rtpbuffer.h: Add new RTPBuffer flags