Comment on attachment 304472 [details] [review]
rtcpbuffer: Update package validation to support reduce size rtcp packets

You can't do that without breaking API. Code assumes that validated RTCP packets have SR and RR at the beginning.

It needs new API.

FWIW, you can easily"add reduced size RTCP support to rtpsession now by adding a property to enable it, and adding some more "if (data.is_early && sess->do_reduced_size_rtcp)" in the RTCP packet generation. After the patches I merged there yesterday.

Sames applies to ssrcdemux patch I sent to: https://bugzilla.gnome.org/show_bug.cgi?id=750327

What do you think about adding a function to check if a rtcp package is a valid compound or reduce size package?

Something like this maybe?
> boolean validate_full(packet, require_sr_rr_at_start)
or
> boolean validate_full(packet, allow_reduced_size_rtcp)

I was thinking about something like the second option.

Created attachment 304480 [details] [review]
rtcpbuffer: Update package validation to support reduce size rtcp packets

Review of attachment 304480 [details] [review]:

Looks correct to me, just some typos. I think there was also some code in rtpsession or rtpsource that assumes that RTCP packets start with SR/RR btw.

::: gst-libs/gst/rtp/gstrtcpbuffer.c
@@ +92,3 @@
  * @data: (array length=len): the data to validate
  * @len: the length of @data to validate
+ * @allow_reduced_size_rtcp: TRUE if reduce size RTCP package are considered valid

reduceD

@@ +99,3 @@
  * this module.
  *
+ * This function is updated to support reduce size rtcp packets according to

reduceD

::: gst-libs/gst/rtp/gstrtcpbuffer.h
@@ +176,3 @@
+ * packets, basically it accepts other types than RR and SR
+ */
+#define GST_RTCP_REDUCE_SIZE_VALID_MASK (0xc000 | 0x2000 | 0xf8)

REDUCED with D in the end :)

Created attachment 304487 [details] [review]
rtcpbuffer: Update package validation to support reduced size rtcp packets

Review of attachment 304487 [details] [review]:

::: gst-libs/gst/rtp/gstrtcpbuffer.h
@@ +229,3 @@
 
+gboolean        gst_rtcp_buffer_validate_data_full      (guint8 *data, guint len, gboolean allow_reduced_size_rctp);
+gboolean        gst_rtcp_buffer_validate_full           (GstBuffer *buffer, gboolean allow_reduced_size_rctp);

Can't you just make it into a
gst_rtsp_buffer_validate_reduced() or something like that. Or if you want ot make a _full(), make the argument be flags, although I'm not sure what other flags they could be. Guessing what gst_rtcp_buffer_validate_full (buf, TRUE) means is non easy without having devhelp opened. Something like gst_rtsp_buffer_validate_full(buf, GST_RTCP_VALIDATE_ACCEPT_REDUCED) is much more clear. Although my first option is to just make a function without arguments or flags.

(In reply to Olivier Crête from comment #10)
> Review of attachment 304487 [details] [review] [review]:
> 
> ::: gst-libs/gst/rtp/gstrtcpbuffer.h
> @@ +229,3 @@
>  
> +gboolean        gst_rtcp_buffer_validate_data_full      (guint8 *data,
> guint len, gboolean allow_reduced_size_rctp);
> +gboolean        gst_rtcp_buffer_validate_full           (GstBuffer *buffer,
> gboolean allow_reduced_size_rctp);
> 
> Can't you just make it into a
> gst_rtsp_buffer_validate_reduced() or something like that. Or if you want ot
> make a _full(), make the argument be flags, although I'm not sure what other
> flags they could be. Guessing what gst_rtcp_buffer_validate_full (buf, TRUE)
> means is non easy without having devhelp opened. Something like
> gst_rtsp_buffer_validate_full(buf, GST_RTCP_VALIDATE_ACCEPT_REDUCED) is much
> more clear. Although my first option is to just make a function without
> arguments or flags.

I agree, having a boolean parameter does not produce clean code. Moreover, having a full function with only one flag is a little bit useless if you want to add more options. I'll implement the _reduced () version and if we need more verify functions in the future we can add flags.

Created attachment 304557 [details] [review]
rtcpbuffer: Update package validation to support reduced size rtcp packets

Hi, any more comments on this?

commit bf5d3bf868337bc4af11bc428e2e33d007a4d2fb
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Jun 5 10:16:56 2015 +0200

    rtcpbuffer: Improve documentation of new functions a bit
    
    Also actually add them to the documentation.

commit 9931bef8ca9af404da2900f3944edc18b2921b68
Author: Jose Antonio Santos Cadenas <santoscadenas@gmail.com>
Date:   Wed Jun 3 11:20:35 2015 +0200

    rtcpbuffer: Update package validation to support reduced size rtcp packets
    
    According to this section of the rfc.
    https://tools.ietf.org/html/rfc5506#section-3.4.2
    The validation should be updated to accept more types of RTCP
    packages, with this mask change feedback packages will be also
    accepted.
    
    Change-Id: If5ead59e03c7c60bbe45a9b09f3ff680e7fa4868