Created attachment 309485 [details] [review]
gstbasertphandler: useful base class for custom RTP packet management

Created attachment 309486 [details] [review]
srtpenc: add "add-rtp-handler" property

I'm not sure I understand what this is for? Any reason to not just emit a signal? Or use a pad probe? The buffer coming out of srtpenc will always be writable anyway. Or do you want to modify the buffer before encrypting it, in that case you can use a probe or just an element before srtpenc?

I think the point is that srtpenc is always doing a copy of the packets, so we operate directly on the copy here. With an element before (or pad probe) you would modify the original buffer, not the copy.

Not sure how that makes a difference though, you always copy anyway. And whether you copy the modified version or the original one should be exactly the same.

Hello @olivier and @sebastian,
the result is the same that having an element or a pad probe before, but the point is the efficiency.
Now I am using a pad probe to manage each RTP packet, but to do this stuff thread-safe I have to make each buffer writable and then map them as GST_MAP_WRITE.
With this, new copies are made and after a new one is done by srtpenc. Because of this I want to take advantage of the copy done by srtpenc.

The first idea was using a signal, but it is more efficient use a direct function call, so I have done this proposal.
I am also thinking about using this mechanism in payloaders for adding custom RTP extension headers.

Why are the buffers that go into srtpenc not writable?

For custom RTP extension headers, a custom payloader/depayloader (or should we call it formatter/parser?) that goes after/before the codec payloader/depayloader is definitely the better idea. It would also not cause any additional copies to happen unless for some reason something in your code pushes unwritable buffers.

Review of attachment 309486 [details] [review]:

::: ext/srtp/gstsrtpenc.c
@@ +408,3 @@
+      G_STRUCT_OFFSET (GstSrtpEncClass, add_rtp_handler), NULL, NULL,
+      gst_srtp_marshal_VOID__OBJECT_STRING, G_TYPE_NONE, 2,
+      GST_TYPE_BASE_RTP_HANDLER, G_TYPE_STRING);

What about removing handlers again? This doesn't look very nice API to me in general, signals would be preferred here as they solve exactly the same problem.

(In reply to Sebastian Dröge (slomo) from comment #6)
> Why are the buffers that go into srtpenc not writable?
> 

If for example you have an RtxQueue before.

> For custom RTP extension headers, a custom payloader/depayloader (or should
> we call it formatter/parser?) that goes after/before the codec
> payloader/depayloader is definitely the better idea. It would also not cause
> any additional copies to happen unless for some reason something in your
> code pushes unwritable buffers.

I agree with you, it is a better design, but again the point is the efficiency. Now I am using a pad probe (similar as using a element) and I see that adding extension headers is too cost. Because of this I want to study if it can be avoid adding it in the payloader.
I am attaching a callgrind graph that could help us to find a solution.

Created attachment 309525 [details]
gst_rtp_buffer_add_extension_onebyte_header Callgrind Graph

(In reply to Sebastian Dröge (slomo) from comment #7)
> Review of attachment 309486 [details] [review] [review]:
> 
> ::: ext/srtp/gstsrtpenc.c
> @@ +408,3 @@
> +      G_STRUCT_OFFSET (GstSrtpEncClass, add_rtp_handler), NULL, NULL,
> +      gst_srtp_marshal_VOID__OBJECT_STRING, G_TYPE_NONE, 2,
> +      GST_TYPE_BASE_RTP_HANDLER, G_TYPE_STRING);
> 
> What about removing handlers again? This doesn't look very nice API to me in
> general, signals would be preferred here as they solve exactly the same
> problem.

I also prefer signals as I said before, but it is significantly more inefficient, isn't it?

I still think the signal is a bad idea. What you can do instead is to remove the copy from srtpenc if the underlying buffer is writable and has enough space available afterwards. That way, your element can just allocate a bigger buffer than needed and srtpenc won't do a copy. It will do the same effect but without having strange APIs. This will completely eliminate the copy in other cases.
 I guess srtpenc could return this required "padding" through the allocation query in a GstAllocationParams.

I think that Olivier's proposal is a good idea.
In my case, the RTP buffer is created by a payloader.
I have never used GstAllocationParams queries. How should I use it?
My pipeline is like this:
  payloader -> rtxqueue -> rtpsession -> srtpenc

Ok, let's close this then as the idea is considered bad and nobody is implementing it anyway, and Miguel's use-case can be handled differently :)