GNOME Bugzilla – Bug 733265
srtpenc: remove get-rollover-counter signal and add stats property
Last modified: 2016-06-13 13:49:34 UTC
Created attachment 280868 [details] [review] remove get-rollover-counter and add stats We've been talking with Wim today about the ROC and SRTP caps properties (see bug 730539). This is a first patch towards an improved SRTP caps. From the commit log: We remove get-rollover-counter signal in favor of the "stats" property. The "stats" property is a GstStructure with caps application/x-multiplexed that contains an array of structures with caps application/x-srtp. Each structure will contain the SRTP caps, currently it only contains "ssrc" and "roc" fields. srtpenc only handles one SSRC right now, so currently the array mentioned above will only contain one element.
Created attachment 280946 [details] [review] remove get-rollover-counter and add stats fixup In the previous patch I was forcing the SSRC in the SRTP policy. I now leave it to what it was before: policy.ssrc.value = 0; policy.ssrc.type = ssrc_any_outbound; Because srtp_protect can handle multiple SSRC in a single stream.
Created attachment 289703 [details] [review] remove get-rollover-counter and add stats 2nd fixup + leak fixes This patch uses proper boxed types with GArray.
Ping? Wtay - was there something you didn't like about this patch, or it just dropped out of sight?
As additional information. We've been using this patch (and the other srtp related ones) since last year without any issues.
Review of attachment 289703 [details] [review]: ::: ext/srtp/gstsrtpenc.c @@ +611,3 @@ + + if (filter->session) { + stream = srtp_get_stream (filter->session, htonl (filter->current_ssrc)); Why only the current SSRC? Why not keep an array of seen SSRCs since the key was last changed and just return the ROC for all of them? @@ +639,3 @@ + + s = gst_structure_new ("application/x-multiplexed", + "streams", G_TYPE_ARRAY, streams, NULL); a GstStructure containing a GArray of GstStructures?? Why such a monstruosity? Also, you can'T use a GArray in element properties as they're sadly not introspectable. You may want to us a GValueArray of GStructures instead. @@ +807,2 @@ if (HAS_CRYPTO (filter)) + gst_structure_set (ps, "roc", G_TYPE_UINT, 0, NULL); Why put roc=0 in the caps? What's the point? @@ +1023,3 @@ if (filter->first_session) { + GstCaps *caps = gst_pad_get_current_caps (pad); + err_status_t status = gst_srtp_enc_create_session (filter, caps, is_rtcp); You should take the SSRC from inside the packets, not from the caps. The SSRC in the caps is kind of meaningless because of RTX, FEC, etc. There could be more than one SSRC in the stream.
Aleix, are you going to update the patch?
(In reply to Sebastian Dröge (slomo) from comment #6) > Aleix, are you going to update the patch? Yes, I will but it might take some time. If it's really urgent for someone I could try to speed up things.
Created attachment 326870 [details] [review] remove get-rollover-counter and add stats (1.8.0) This one still needs work per the above reviews. It just applies to 1.8.0.
Created attachment 329597 [details] [review] remove get-rollover-counter and add stats (1.8.0, revised) Thanks to Josep Torra on this one for making it more GStreamer-like.
Review of attachment 329597 [details] [review]: Makes sense, yes ::: ext/srtp/gstsrtpenc.c @@ +617,3 @@ + while (stream) { + GstStructure *ss; + guint32 ssrc = ntohl (stream->ssrc); You probably want to use GUINT32_FROM_BE() or something here, or g_ntohl() at least. @@ +630,3 @@ + } + + gst_structure_set_value (s, "streams", &va); gst_structure_take_value()?
Created attachment 329660 [details] [review] remove get-rollover-counter and add stats (1.8.0, revised 2)
Comment on attachment 329660 [details] [review] remove get-rollover-counter and add stats (1.8.0, revised 2) commit 15a3b0f6cee86b9c7cb7a697c7059bba42dba4a9 Author: Aleix Conchillo Flaqué <aleix@oblong.com> Date: Mon May 30 14:10:23 2016 +0200 srtpenc: remove get-rollover-counter signal and add stats property We remove get-rollover-counter signal in favor of the "stats" property. The "stats" property is a GstStructure with caps application/x-srtp-encoder-stats that contains an array of structures with caps application/x-srtp-stream. Each stream structure contains "ssrc" and "roc" fields.