Created attachment 369933 [details] [review]
rtpbuffer: expose gst_rtp_buffer_initialize_header function

Review of attachment 369933 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbuffer.c
@@ +88,3 @@
+ * Write a RTP header in a buffer with pre-allocated data.
+ * The size of the buffer must be sufficient, the minimum size
+ * can be obtained with gst_rtp_buffer_calc_header_len()

Might be worth mentioning here that the caller is responsible for filling all the fields with useful values, and that this is just enough to gst_rtp_buffer_map()

@@ +93,3 @@
+ * otherwise.
+ *
+ * Since: 1.14

1.16

As we required to header to be in only one memory block, the header layout is fixed. Can't you just use gst_rtp_buffer_new_allocate() with payload_len=0 and then append the extra GstMemory with the actual data? You could even have a gst_rtp_buffer_new_take(.., GstBuffer *payload_buf) ?

(In reply to Olivier Crête from comment #3)
> As we required to header to be in only one memory block, the header layout
> is fixed.

I didn't get that, can you expand a bit?

> Can't you just use gst_rtp_buffer_new_allocate() with
> payload_len=0 and then append the extra GstMemory with the actual data? You
> could even have a gst_rtp_buffer_new_take(.., GstBuffer *payload_buf) ?

The use case for this is:

alawenc ! rtppcmapay , where the payloader provides a buffer pool, creating buffers with gst_buffer_new_allocate , with a single memory, then setting the memory offset to header_len, which means the encoder will write right after the header and no allocation of GstMemory will occur at gst_buffer_map time, gst_rtp_buffer_new_allocate doesn't really work for this.

Created attachment 370013 [details] [review]
rtpbuffer: expose gst_rtp_buffer_initialize_header function

An example use case is to avoid using gst_rtp_buffer_new_allocate
as it implies a particular memory layout.

(In reply to Olivier Crête from comment #3)
> As we required to header to be in only one memory block, the header layout
> is fixed.

I also thought this first but it's not true fortunately.

(In reply to Mathieu Duponchelle from comment #4)
> The use case for this is:
> 
> alawenc ! rtppcmapay , where the payloader provides a buffer pool, creating
> buffers with gst_buffer_new_allocate , with a single memory, then setting
> the memory offset to header_len, which means the encoder will write right
> after the header and no allocation of GstMemory will occur at gst_buffer_map
> time, gst_rtp_buffer_new_allocate doesn't really work for this.

I don't understand why it doesn't work? You can do gst_buffer_new_allocate(payload_len), then do gst_buffer_resize(offset=header_len), and then if the buffer comes back as expected, then you can just size it back?

Are you planning to do this in GstRTPBaseAudioPayload ?


(In reply to Sebastian Dröge (slomo) from comment #6)
> (In reply to Olivier Crête from comment #3)
> > As we required to header to be in only one memory block, the header layout
> > is fixed.
> 
> I also thought this first but it's not true fortunately.

In which case is it not true?

(In reply to Olivier Crête from comment #7)
> (In reply to Mathieu Duponchelle from comment #4)
> > The use case for this is:
> > 
> > alawenc ! rtppcmapay , where the payloader provides a buffer pool, creating
> > buffers with gst_buffer_new_allocate , with a single memory, then setting
> > the memory offset to header_len, which means the encoder will write right
> > after the header and no allocation of GstMemory will occur at gst_buffer_map
> > time, gst_rtp_buffer_new_allocate doesn't really work for this.
> 
> I don't understand why it doesn't work? You can do
> gst_buffer_new_allocate(payload_len), then do
> gst_buffer_resize(offset=header_len), and then if the buffer comes back as
> expected, then you can just size it back?

Just checked again, this might work, but this would make the implementation of the pool in pcmapay more complicated, as there is actually no memory layout requirement for "RTP buffers" I figure this API is valid and helpful :)

> 
> Are you planning to do this in GstRTPBaseAudioPayload ?
> 

I don't know, this is only really valid for formats with fixed payload sizes, such as PCMA, I figure we could start with PCMA then put that upper in the class hierarchy once we have a better idea of how to make something more generic.

> 
> (In reply to Sebastian Dröge (slomo) from comment #6)
> > (In reply to Olivier Crête from comment #3)
> > > As we required to header to be in only one memory block, the header layout
> > > is fixed.
> > 
> > I also thought this first but it's not true fortunately.
> 
> In which case is it not true?

Take a look at gst_rtp_buffer_map, it does not actually require a specific memory layout :)

(In reply to Mathieu Duponchelle from comment #8)
> (In reply to Olivier Crête from comment #7)
> > (In reply to Mathieu Duponchelle from comment #4)
> >
> > Are you planning to do this in GstRTPBaseAudioPayload ?
> > 
> 
> I don't know, this is only really valid for formats with fixed payload
> sizes, such as PCMA, I figure we could start with PCMA then put that upper
> in the class hierarchy once we have a better idea of how to make something
> more generic.

GstRTPBaseAudioPayload is for formats where the payload is a multiple of a fixed size, so it's exactly for this! Anything with variable bitrates can't use it anyway.


> > (In reply to Sebastian Dröge (slomo) from comment #6)
> > > (In reply to Olivier Crête from comment #3)
> > > > As we required to header to be in only one memory block, the header layout
> > > > is fixed.
> > > 
> > > I also thought this first but it's not true fortunately.
> > 
> > In which case is it not true?
> 
> Take a look at gst_rtp_buffer_map, it does not actually require a specific
> memory layout :)

from the comments in gst_rtp_buffer_map

/* the header must be completely in the first buffer */

But this comment is from 0.10, it shoudl actually be all in the first memory. The extensions should all be in the same block or in just one more block.

/* all extension bytes must be in this block */

The layout of the dats is indeed not fixed, but that doesn't really matter for the GstRtpBuffer* functionis as they only operate on the header.



"

(In reply to Olivier Crête from comment #9)
> GstRTPBaseAudioPayload is for formats where the payload is a multiple of a
> fixed size, so it's exactly for this! Anything with variable bitrates can't
> use it anyway.

Heh, I didn't know that, nice, then we can probably put the optimization there direcly, it will however require more testing on my end, I kind of don't want to break all the subclasses except pcma :)

> 
> > > (In reply to Sebastian Dröge (slomo) from comment #6)
> > > > (In reply to Olivier Crête from comment #3)
> > > > > As we required to header to be in only one memory block, the header layout
> > > > > is fixed.
> > > > 
> > > > I also thought this first but it's not true fortunately.
> > > 
> > > In which case is it not true?
> > 
> > Take a look at gst_rtp_buffer_map, it does not actually require a specific
> > memory layout :)
> 
> from the comments in gst_rtp_buffer_map
> 
> /* the header must be completely in the first buffer */
> 
> But this comment is from 0.10, it shoudl actually be all in the first
> memory. The extensions should all be in the same block or in just one more
> block.
> 
> /* all extension bytes must be in this block */
> 
> The layout of the dats is indeed not fixed, but that doesn't really matter
> for the GstRtpBuffer* functionis as they only operate on the header.

Yes, there are indeed *some* constraints, we could maybe enforce that in the initialize_header function by simply requiring the buffer to have a single memory, what do you think?

I'll attach the pcma pool WIP patch here for reference

Created attachment 370553 [details] [review]
pcmapay pool patch for reference, do not merge

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/427.