GNOME Bugzilla – Bug 746387
srtp: Add support for buffer list in srtpenc
Last modified: 2015-08-17 08:54:51 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
Created attachment 299692 [details] [review] srtpenc: Add missing locks
Created attachment 299693 [details] [review] srtpenc: Add support for buffer list
Created attachment 299694 [details] [review] srtpdec: Separate buffer encoding functionality into a different function
Created attachment 299695 [details] [review] srtpdec: Add support for buffer list
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
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?
Review of attachment 299693 [details] [review]: Looks good
Review of attachment 299694 [details] [review]: Looks good
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
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.
Created attachment 299715 [details] [review] 0001 srtpenc: Split chain functionality so it can be reused for buffer list
Created attachment 299716 [details] [review] 0002 srtpenc: Add missing locks
Created attachment 299717 [details] [review] 0003 srtpenc: Add support for buffer list
Created attachment 299718 [details] [review] 0004 srtpdec: Separate buffer encoding functionality into a different function
Created attachment 299720 [details] [review] 0005 srtpdec: Add support for buffer list
Created attachment 299721 [details] [review] 0003 srtpenc: Add support for buffer list
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?
Created attachment 299798 [details] [review] 0005 srtpdec: Add support for buffer list
Comment on attachment 299798 [details] [review] 0005 srtpdec: Add support for buffer list You attached the wrong patch
Created attachment 299828 [details] [review] 0005 srtpdec: Add support for buffer list
Created attachment 299829 [details] [review] 0006 srtpenc: Do not drop all buffers in buffer list if one fails
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
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
Created attachment 299840 [details] [review] 0005 srtpdec: Add support for buffer list
Created attachment 299841 [details] [review] 0006 srtpenc: Do not drop all buffers in buffer list if one fails
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
Created attachment 299842 [details] [review] 0005 srtpdec: Add support for buffer list
All pushed :)
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.
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.
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.
Olivier, let's revert then? What about the part in rtpsession you talked about?
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.
Ok, you want to fix that and revert the srtp commit?
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.
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
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().
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.