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 746387 - srtp: Add support for buffer list in srtpenc
srtp: Add support for buffer list in srtpenc
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-18 10:28 UTC by Jose Antonio Santos Cadenas
Modified: 2015-08-17 08:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
srtpenc: Split chain functionality so it can be reused for buffer list (6.22 KB, patch)
2015-03-18 10:28 UTC, Jose Antonio Santos Cadenas
none Details | Review
srtpenc: Add missing locks (1.96 KB, patch)
2015-03-18 10:29 UTC, Jose Antonio Santos Cadenas
none Details | Review
srtpenc: Add support for buffer list (4.91 KB, patch)
2015-03-18 10:29 UTC, Jose Antonio Santos Cadenas
none Details | Review
srtpdec: Separate buffer encoding functionality into a different function (3.43 KB, patch)
2015-03-18 10:29 UTC, Jose Antonio Santos Cadenas
none Details | Review
srtpdec: Add support for buffer list (4.55 KB, patch)
2015-03-18 10:30 UTC, Jose Antonio Santos Cadenas
none Details | Review
0001 srtpenc: Split chain functionality so it can be reused for buffer list (6.22 KB, patch)
2015-03-18 14:07 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0002 srtpenc: Add missing locks (3.34 KB, patch)
2015-03-18 14:08 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0003 srtpenc: Add support for buffer list (4.97 KB, patch)
2015-03-18 14:08 UTC, Jose Antonio Santos Cadenas
none Details | Review
0004 srtpdec: Separate buffer encoding functionality into a different function (3.56 KB, patch)
2015-03-18 14:09 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0005 srtpdec: Add support for buffer list (4.62 KB, patch)
2015-03-18 14:11 UTC, Jose Antonio Santos Cadenas
none Details | Review
0003 srtpenc: Add support for buffer list (4.97 KB, patch)
2015-03-18 14:13 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0005 srtpdec: Add support for buffer list (6.75 KB, patch)
2015-03-19 10:56 UTC, Jose Antonio Santos Cadenas
none Details | Review
0005 srtpdec: Add support for buffer list (5.94 KB, patch)
2015-03-19 14:50 UTC, Jose Antonio Santos Cadenas
none Details | Review
0006 srtpenc: Do not drop all buffers in buffer list if one fails (3.83 KB, patch)
2015-03-19 14:50 UTC, Jose Antonio Santos Cadenas
none Details | Review
0005 srtpdec: Add support for buffer list (6.21 KB, patch)
2015-03-19 15:20 UTC, Jose Antonio Santos Cadenas
none Details | Review
0006 srtpenc: Do not drop all buffers in buffer list if one fails (4.03 KB, patch)
2015-03-19 15:20 UTC, Jose Antonio Santos Cadenas
committed Details | Review
0005 srtpdec: Add support for buffer list (6.23 KB, patch)
2015-03-19 15:25 UTC, Jose Antonio Santos Cadenas
committed Details | Review

Description Jose Antonio Santos Cadenas 2015-03-18 10:28:47 UTC
Created attachment 299691 [details] [review]
srtpenc: Split chain functionality so it can be reused for buffer list

Attached patches add support for buffer list in srtpenc and srtpdec
Comment 1 Jose Antonio Santos Cadenas 2015-03-18 10:29:14 UTC
Created attachment 299692 [details] [review]
srtpenc: Add missing locks
Comment 2 Jose Antonio Santos Cadenas 2015-03-18 10:29:35 UTC
Created attachment 299693 [details] [review]
srtpenc: Add support for buffer list
Comment 3 Jose Antonio Santos Cadenas 2015-03-18 10:29:56 UTC
Created attachment 299694 [details] [review]
srtpdec: Separate buffer encoding functionality into a different function
Comment 4 Jose Antonio Santos Cadenas 2015-03-18 10:30:11 UTC
Created attachment 299695 [details] [review]
srtpdec: Add support for buffer list
Comment 5 Sebastian Dröge (slomo) 2015-03-18 11:49:30 UTC
Review of attachment 299691 [details] [review]:

::: ext/srtp/gstsrtpenc.c
@@ +985,1 @@
   do_setcaps = filter->key_changed;

Doesn't this need some locking?

@@ +1118,2 @@
   if (gst_srtp_get_soft_limit_reached ()) {
     g_signal_emit (filter, gst_srtp_enc_signals[SIGNAL_SOFT_LIMIT], 0);

Holding the lock while emitting a signal is not a great idea
Comment 6 Sebastian Dröge (slomo) 2015-03-18 11:50:58 UTC
Review of attachment 299692 [details] [review]:

::: ext/srtp/gstsrtpenc.c
@@ +979,3 @@
   gboolean do_setcaps = FALSE;
 
+  if (filter->key_changed) {

filter->key_changed probably also needs a lock

Maybe we can just hold the lock over all of the chain function and only lock/unlock it once?
Comment 7 Sebastian Dröge (slomo) 2015-03-18 11:52:17 UTC
Review of attachment 299693 [details] [review]:

Looks good
Comment 8 Sebastian Dröge (slomo) 2015-03-18 11:54:11 UTC
Review of attachment 299694 [details] [review]:

Looks good
Comment 9 Sebastian Dröge (slomo) 2015-03-18 11:56:53 UTC
Review of attachment 299695 [details] [review]:

::: ext/srtp/gstsrtpdec.c
@@ +1214,3 @@
+  /* If all is well, we may have reached soft limit */
+  if (gst_srtp_get_soft_limit_reached ())
+    request_key_with_signal (filter, ssrc, SIGNAL_SOFT_LIMIT);

Emitting a signal with a mutex locked is not great

@@ +1217,3 @@
+
+push_out:
+  /* Push buffer list to source pad */

And all here must be done without holding the mutex, you also forget to unlock it before returning
Comment 10 Jose Antonio Santos Cadenas 2015-03-18 13:12:38 UTC
Review of attachment 299695 [details] [review]:

::: ext/srtp/gstsrtpdec.c
@@ +1214,3 @@
+  /* If all is well, we may have reached soft limit */
+  if (gst_srtp_get_soft_limit_reached ())
+      GST_OBJECT_UNLOCK (filter);

In fact the mutex is not locked, it is unlockd inside gst_srtp_dec_decode_buffer. It's a side effect of the function that I'm going to solve.
Comment 11 Jose Antonio Santos Cadenas 2015-03-18 14:07:51 UTC
Created attachment 299715 [details] [review]
0001 srtpenc: Split chain functionality so it can be reused for buffer list
Comment 12 Jose Antonio Santos Cadenas 2015-03-18 14:08:27 UTC
Created attachment 299716 [details] [review]
0002 srtpenc: Add missing locks
Comment 13 Jose Antonio Santos Cadenas 2015-03-18 14:08:59 UTC
Created attachment 299717 [details] [review]
0003 srtpenc: Add support for buffer list
Comment 14 Jose Antonio Santos Cadenas 2015-03-18 14:09:35 UTC
Created attachment 299718 [details] [review]
0004 srtpdec: Separate buffer encoding functionality into a different function
Comment 15 Jose Antonio Santos Cadenas 2015-03-18 14:11:17 UTC
Created attachment 299720 [details] [review]
0005 srtpdec: Add support for buffer list
Comment 16 Jose Antonio Santos Cadenas 2015-03-18 14:13:50 UTC
Created attachment 299721 [details] [review]
0003 srtpenc: Add support for buffer list
Comment 17 Sebastian Dröge (slomo) 2015-03-18 15:28:11 UTC
Review of attachment 299720 [details] [review]:

::: ext/srtp/gstsrtpdec.c
@@ +1201,3 @@
+      GST_OBJECT_UNLOCK (filter);
+      GST_WARNING_OBJECT (filter, "Invalid buffer, dropping");
+      goto drop_buffers;

Maybe if there are invalid buffers, just drop the invalid ones but continue processing the others. (Same in the encoder btw)

@@ +1217,3 @@
+      goto drop_buffers;
+    }
+    GST_OBJECT_UNLOCK (filter);

You're unlocking here in every iteration, that's not going to work.

@@ +1222,3 @@
+  /* If all is well, we may have reached soft limit */
+  if (gst_srtp_get_soft_limit_reached ())
+    request_key_with_signal (filter, ssrc, SIGNAL_SOFT_LIMIT);

And holding the lock while emitting the signal here. I assume the unlock should've been below the loop?
Comment 18 Jose Antonio Santos Cadenas 2015-03-19 10:56:36 UTC
Created attachment 299798 [details] [review]
0005 srtpdec: Add support for buffer list
Comment 19 Sebastian Dröge (slomo) 2015-03-19 11:38:00 UTC
Comment on attachment 299798 [details] [review]
0005 srtpdec: Add support for buffer list

You attached the wrong patch
Comment 20 Jose Antonio Santos Cadenas 2015-03-19 14:50:22 UTC
Created attachment 299828 [details] [review]
0005 srtpdec: Add support for buffer list
Comment 21 Jose Antonio Santos Cadenas 2015-03-19 14:50:55 UTC
Created attachment 299829 [details] [review]
0006 srtpenc: Do not drop all buffers in buffer list if one fails
Comment 22 Sebastian Dröge (slomo) 2015-03-19 15:08:43 UTC
Review of attachment 299829 [details] [review]:

::: ext/srtp/gstsrtpenc.c
@@ +1172,3 @@
+  if (!gst_srtp_enc_check_buffer (data->filter, *buffer, data->is_rtcp)) {
+    GST_WARNING_OBJECT (data->filter, "Invalid buffer, dropping");
+    buffer = NULL;

Either "gst_buffer_replace (buffer, NULL);" or "*gst_buffer_unref (*buffer); buffer = NULL;"

@@ +1214,1 @@
+  gst_buffer_list_foreach (buf_list, validate_buffer_it, &validate_data);

Maybe check here if any buffer is left at all
Comment 23 Sebastian Dröge (slomo) 2015-03-19 15:10:17 UTC
Review of attachment 299828 [details] [review]:

::: ext/srtp/gstsrtpdec.c
@@ +1202,3 @@
+          validate_buffer (data->filter, *buffer, data->ssrc, data->is_rtcp))) {
+    GST_WARNING_OBJECT (data->filter, "Invalid buffer, dropping");
+    buffer = NULL;

Same here

@@ +1216,3 @@
+          data->is_rtcp, data->ssrc)) {
+    GST_WARNING_OBJECT (data->filter, "Error decoding buffer, dropping");
+    buffer = NULL;

Same here

@@ +1245,3 @@
+
+  /* Check if this stream exists, if not create a new stream */
+  gst_buffer_list_foreach (buf_list, validate_buffer_it, &validate_data);

Maybe check if any is left at all

@@ +1255,3 @@
+  decode_data.is_rtcp = is_rtcp;
+
+  gst_buffer_list_foreach (buf_list, decode_buffer_it, &decode_data);

Maybe check if any is left at all
Comment 24 Jose Antonio Santos Cadenas 2015-03-19 15:20:09 UTC
Created attachment 299840 [details] [review]
0005 srtpdec: Add support for buffer list
Comment 25 Jose Antonio Santos Cadenas 2015-03-19 15:20:32 UTC
Created attachment 299841 [details] [review]
0006 srtpenc: Do not drop all buffers in buffer list if one fails
Comment 26 Jose Antonio Santos Cadenas 2015-03-19 15:23:00 UTC
Review of attachment 299840 [details] [review]:

::: ext/srtp/gstsrtpdec.c
@@ +1216,3 @@
+          data->is_rtcp, data->ssrc)) {
+    GST_WARNING_OBJECT (data->filter, "Error decoding buffer, dropping");
+

I miss to change this
Comment 27 Jose Antonio Santos Cadenas 2015-03-19 15:25:40 UTC
Created attachment 299842 [details] [review]
0005 srtpdec: Add support for buffer list
Comment 28 Sebastian Dröge (slomo) 2015-03-19 15:30:04 UTC
All pushed :)
Comment 29 Olivier Crête 2015-07-28 21:40:12 UTC
The srtpdec patch is all bad and should be reverted or re-written.

You can't assume that all buffers in a buffer list are RTP or RTCP and you can't assume they all have ther same SSRC. You must instead basically do all the processing that the single buffer function does on each buffer independently. And then create two buffer lists, one for RTP buffers ane one for RTCP buffers and then push them out on the separate pads.
Comment 30 Sebastian Dröge (slomo) 2015-07-30 19:44:57 UTC
Let's revert if nobody wants to come up with a patch. Buffer lists for the receiving side are mostly useless currently anyway.


As you said on IRC, the same bug is also in the RTP/RTCP demuxing in rtpsession.
Comment 31 Jose Antonio Santos Cadenas 2015-07-31 06:20:25 UTC
I don't have time right now to implement it correctly. It's OK for me to revert buffer list support for decoder, as Sebastian says, is not that important at the reception side. When I have some time I'll rework it.
Comment 32 Sebastian Dröge (slomo) 2015-08-07 11:18:25 UTC
Olivier, let's revert then? What about the part in rtpsession you talked about?
Comment 33 Olivier Crête 2015-08-07 15:27:29 UTC
I was sure rtpsession did the same thing, I must have been drunk or something. 

That said, the sender-side buffer list code in rtpsession code only processes the first buffer of a buffer list, which doesn't seem right to me, it will produce incorrect RTCP statistics.
Comment 34 Sebastian Dröge (slomo) 2015-08-08 09:28:48 UTC
Ok, you want to fix that and revert the srtp commit?
Comment 35 Sebastian Dröge (slomo) 2015-08-13 10:40:33 UTC
commit 88f85f6595689d147036ae6127c7a2ff4f73dcbd
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Aug 13 12:38:41 2015 +0200

    Revert "srtpdec: Add support for buffer list"
    
    This reverts commit ff11a1a8a0c685d2edd0e06c0071cbb94f2cb663.
    
    It can't be assumed that all buffers in a buffer list have the same SSRC or
    are RTP or RTCP only. It has to be checked for every single buffer, and one
    basically has to do the processing that is done by the default chain_list
    implementation.
Comment 36 Sebastian Dröge (slomo) 2015-08-13 10:43:28 UTC
Olivier, which rtpsession code do you mean? It only seems to get the timestamp for one buffer of the list, but they are supposed to be all the same in the list. Apart from that it seems to handle all buffers separately.

Let's track this issue in a new bug if you still think something is wrong there
Comment 37 Olivier Crête 2015-08-13 14:52:55 UTC
In rtpsession, why do you assume that all buffers must have the same ssrc? It also gets the rtptime only from the first one, and I'm not sure we can assume they all ahve the same rtptime either. And they definitely won't have the same seqnum, in update_packet().
Comment 38 Sebastian Dröge (slomo) 2015-08-13 14:56:41 UTC
But update_packet() is called for each buffer in the list. The only case where only a single buffer of a list is looked at seems to be the timestamp one.