After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 750325 - rtcpbuffer: Update package validation to support reduced size rtcp packets
rtcpbuffer: Update package validation to support reduced size rtcp packets
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 750327 750332
 
 
Reported: 2015-06-03 10:13 UTC by Jose Antonio Santos Cadenas
Modified: 2015-06-05 08:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtcpbuffer: Update package validation to support reduce size rtcp packets (1.87 KB, patch)
2015-06-03 10:13 UTC, Jose Antonio Santos Cadenas
none Details | Review
rtcpbuffer: Update package validation to support reduce size rtcp packets (5.67 KB, patch)
2015-06-03 11:09 UTC, Jose Antonio Santos Cadenas
none Details | Review
rtcpbuffer: Update package validation to support reduced size rtcp packets (5.67 KB, patch)
2015-06-03 11:27 UTC, Jose Antonio Santos Cadenas
none Details | Review
rtcpbuffer: Update package validation to support reduced size rtcp packets (7.15 KB, patch)
2015-06-03 21:23 UTC, Jose Antonio Santos Cadenas
committed Details | Review

Description Jose Antonio Santos Cadenas 2015-06-03 10:13:32 UTC
Created attachment 304472 [details] [review]
rtcpbuffer: Update package validation to support reduce size rtcp packets

According to https://tools.ietf.org/html/rfc5506#section-3.4.2

Validation should be updated in order to support reduce size RTP packages.

Attached is a patch that changes mask to allow other types than RR and SR at the first header of the RTCP package.
Comment 1 Sebastian Dröge (slomo) 2015-06-03 10:21:45 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2015-06-03 10:25:20 UTC
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.
Comment 3 Jose Antonio Santos Cadenas 2015-06-03 10:30:20 UTC
Sames applies to ssrcdemux patch I sent to: https://bugzilla.gnome.org/show_bug.cgi?id=750327
Comment 4 Jose Antonio Santos Cadenas 2015-06-03 10:32:02 UTC
What do you think about adding a function to check if a rtcp package is a valid compound or reduce size package?
Comment 5 Sebastian Dröge (slomo) 2015-06-03 10:33:23 UTC
Something like this maybe?
> boolean validate_full(packet, require_sr_rr_at_start)
or
> boolean validate_full(packet, allow_reduced_size_rtcp)
Comment 6 Jose Antonio Santos Cadenas 2015-06-03 10:36:42 UTC
I was thinking about something like the second option.
Comment 7 Jose Antonio Santos Cadenas 2015-06-03 11:09:51 UTC
Created attachment 304480 [details] [review]
rtcpbuffer: Update package validation to support reduce size rtcp packets
Comment 8 Sebastian Dröge (slomo) 2015-06-03 11:22:26 UTC
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 :)
Comment 9 Jose Antonio Santos Cadenas 2015-06-03 11:27:06 UTC
Created attachment 304487 [details] [review]
rtcpbuffer: Update package validation to support reduced size rtcp packets
Comment 10 Olivier Crête 2015-06-03 16:19:47 UTC
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.
Comment 11 Jose Antonio Santos Cadenas 2015-06-03 20:41:00 UTC
(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.
Comment 12 Jose Antonio Santos Cadenas 2015-06-03 21:23:24 UTC
Created attachment 304557 [details] [review]
rtcpbuffer: Update package validation to support reduced size rtcp packets
Comment 13 Jose Antonio Santos Cadenas 2015-06-05 07:16:56 UTC
Hi, any more comments on this?
Comment 14 Sebastian Dröge (slomo) 2015-06-05 08:18:40 UTC
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