Created attachment 370509 [details] [review]
rtxsend: fix wrong memory layout assumption

The code responsible for creating retransmitted buffers
assumed the stored buffer had been created with
rtp_buffer_new_allocate when copying the extension data,
which isn't necessarily the case, for example when
the rtp buffers come from a udpsink.

Review of attachment 370509 [details] [review]:

::: gst/rtpmanager/gstrtprtxsend.c
@@ -401,3 @@
   /* copy extension if any */
   if (rtp.size[1]) {
-    mem = gst_memory_copy (rtp.map[1].memory, 0, rtp.size[1]);

memory is NULL in that case or how does it work? But the data and size pointers are all correctly set?

But in the udpsrc case for example we should be able to copy the memory nonetheless and use a different offset instead of completely copying. Not sure if that makes much of a difference though and it would complicate the code quite a bit, and still not cover all potential cases.

So merge as-is unless you see a way of doing this nicer :)

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 370509 [details] [review] [review]:
> 
> ::: gst/rtpmanager/gstrtprtxsend.c
> @@ -401,3 @@
>    /* copy extension if any */
>    if (rtp.size[1]) {
> -    mem = gst_memory_copy (rtp.map[1].memory, 0, rtp.size[1]);
> 
> memory is NULL in that case or how does it work? But the data and size
> pointers are all correctly set?

No in that case, rtp.map[1].memory == rtp.map[0].memory, what this ended up doing was copying rtp.size[1] bytes starting from the beginning of the RTP header, instead of starting at the beginning of the extension data.

> But in the udpsrc case for example we should be able to copy the memory
> nonetheless and use a different offset instead of completely copying. Not
> sure if that makes much of a difference though and it would complicate the
> code quite a bit, and still not cover all potential cases.
> 
> So merge as-is unless you see a way of doing this nicer :)

Yes, I think we should just go for correctness, also s/udpsink/udpsrc/ in the commit message :)

Attachment 370509 [details] pushed as 8270cba - rtxsend: fix wrong memory layout assumption