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 572973 - [API] buffer size query or indication from audio encoders to upstream how many samples to produce per buffer
[API] buffer size query or indication from audio encoders to upstream how man...
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-02-24 12:39 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2018-11-03 12:13 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Stefan Sauer (gstreamer, gtkdoc dev) 2009-02-24 12:39:02 UTC
It would be useful if (audio) encoders could send a buffer-size-request event upstream. Sources can use that to configure the size of the buffers that they will send in push mode. Basesource can implement that and configure the "blocksize" property.

It would free encoders from using the adapter.
Comment 1 Sebastian Dröge (slomo) 2009-02-24 13:13:53 UTC
Sounds like a good idea to optimize things a bit but encoders would still need to use the adapter as it can't be guaranteed that it will always get a fixed size of samples per buffer (think of demuxers/decoders that output raw audio for example).
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2009-02-26 08:40:30 UTC
encoders would want to request a number of samples:
- mp3: a frame that consists of 384, 576, or 1152 samples
 (depends on MPEG version and layer)
- aac: a frame always consists of 1024 samples

dunno if ther eare difference in mono/stereo. problem is that sources work in bytes (at least if we implement it in basesrc via blocksize). It means the request needs to be converted when passing it along. there could be even the case of:
8bitaudiosrc ! audioconvert ! audioenc

if audioenc request 1024 samples a 16bit = 2048 bit from audioconvert, then audioconvert will request 1024 samples a 8bit from 8bitaudiosrc.

Regarding comment #1. I need to figure when extactly a buffer-size-request should be sent. In the _chain function the encoder could do:

gst_xxx_chain(GstElement *self,GstBuffer *buf)
{
  GstBuffer *in;

  if((G_LIKELY(gst_adapter_available(self->adapter)==0) && (GST_BUFFER_SIZE(buf)==wanted_size)) {
    in=buf;
  } else {
    gst_adapter_push(self->adapter,buf);
    in=gst_adapter_take_buffer(self->adapter,wanted_size);
    if(in==NULL) ... /* need more */
  }

  ...

  gst_buffer_unref(in);
}

It mean the adapter would only be used if we ever get a frame that is no the wanted size.
Comment 3 Tim-Philipp Müller 2009-02-26 08:57:18 UTC
Why an upstream event rather than, say, a downstream query? Is this something dynamic that may change while the pipeline is playing?

As for the adapter-bypass-code in comment #2: isn't the main goal to avoid malloc()s+memcpy()s of the data when merging two GstBuffers in GstAdapter? If the buffers pushed into the adapter are all of the wanted size, the overhead of the adapter functions is surely negligible, no? 
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2009-02-26 09:28:16 UTC
Good idea. So the src will send a preffered-buffer-size query. The encoder will reply. The encoder will just keep using the adapter, which will hand out what you stick in as long as the sizes match.

When should the src query? Obviously before pushing the first buffer. READY->PAUSED?
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2009-03-10 12:24:26 UTC
Hmm several problems with the query:
* if the bitrate changes, the buffersize might change
* if one has the source running and start encoding dynamically, it is difficult to get the change sync'ed so that the frames of requested size appear.

A buffer-size request could be send just before a seek-event.

structure = gst_structure_empty_new ("GstBufferSize");
gst_structure_id_set (structure,
    GST_QUARK (FORMAT), GST_TYPE_FORMAT, format,
    GST_QUARK (SIZE), G_TYPE_INT64, size, NULL);
event = gst_event_new_custom (GST_EVENT_SEEK, structure);

The formats can be converted as they are for seeks.

Problem of both approaches is that elements need to convert that type of query/ event. At least audioconvert, audioresample come to my mind.
Of course we could send a gst_query_new_convert(GST_FORMAT_DEFAULT, samples, GST_FORMAT_BYTES) from the encoder if we would know what format the source uses.
Comment 6 Sebastian Dröge (slomo) 2011-05-20 05:51:26 UTC
Could this somehow be integrated with the allocation query and the reconfigure event in 0.11?
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2011-05-20 07:48:30 UTC
Yes!
Comment 8 Tim-Philipp Müller 2011-10-27 14:53:39 UTC
> Yes!

Really? How?
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2011-10-31 12:29:57 UTC
(In reply to comment #8)
> > Yes!
> 
> Really? How?

Dunno. My answer was more, yes please if possible. We do have the reconfigure event to ask elements to reconfigure. The first not inplace plugin would send a allocation query downstream, parse the answer, reconfigure itself (take the size). If things have changed if would send a new reconfigure event further upstream (unless is is a source).

Is there an initial reconfigure event in READY to do the setup when starting up?
Comment 10 Sebastian Dröge (slomo) 2011-10-31 14:00:46 UTC
Linking causes reconfigure events to be sent and when you didn't negotiate yet you would do the same as when receiving a reconfigure event anyway. It's not really required to get a reconfigure event in READY (also the pads would be flushing then and drop the events).

I don't think this is possible with the current allocation query. But the allocation query could/should be extended to support this or a new query could be added if it doesn't make sense to add it in the allocation query (currently I think it would make sense to extend it but need to look at it in detail)
Comment 11 Olivier Crête 2011-11-01 15:56:32 UTC
I have another use case for this, in RTP, the ptime (packet duration) can be negotiated. So we probably want pulsesrc to give us exactly the requested ptime so we can minimize latency. This will also allow us to smartly use the Opus super low latency mode, etc.
Comment 12 Wim Taymans 2011-12-01 18:31:49 UTC
You can currently return a size in the allocation query, either with or without a pool suggestion. basesrc doesn't use it yet to configure the blocksize, but I guess it should when the size is not 0...
Comment 13 Tim-Philipp Müller 2013-04-21 10:56:16 UTC
One might be able to use/abuse the param structure in

   gst_query_add_allocation_meta (GstQuery *query, GType api, const GstStructure *param)

for this.
Comment 14 Nicolas Dufresne (ndufresne) 2016-05-29 23:36:37 UTC
I might have missed something, but it's not clear to me why we want something more then the size in the allocation query ?
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2016-05-30 09:11:22 UTC
Sounds like we're mixing concepts. Using the buffer-size in the pool part of the allocation query sounds like misuse to me. The encoders don't necessarily need/want a pool.
Comment 16 Nicolas Dufresne (ndufresne) 2016-05-30 14:10:11 UTC
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #15)
> Sounds like we're mixing concepts. Using the buffer-size in the pool part of
> the allocation query sounds like misuse to me. The encoders don't
> necessarily need/want a pool.

The pool in the pool array is a optional field. You don't need to offer a pool to suggest a number of buffers and a buffer size. It's not an abuse, since this parameter means what you guys want.
Comment 17 Stefan Sauer (gstreamer, gtkdoc dev) 2016-05-31 08:06:19 UTC
Nicolas, can you please give an example how you would do it with the api we have?

The source creates and sends an allocation query:
query = gst_query_new_allocation(some_caps, FALSE);
gst_pad_peer_query (self->srcpad, query);

Now the decoder get those and wants to tell what buffer-size is preferred. The available API is:

gst_query_add_allocation_meta (GstQuery *query,
                               GType api,
                               const GstStructure *params);
gst_query_add_allocation_param (GstQuery *query,
                                GstAllocator *allocator,
                                const GstAllocationParams *params);
gst_query_add_allocation_pool (GstQuery *query,
                               GstBufferPool *pool,
                               guint size,
                               guint min_buffers,
                               guint max_buffers);

What I am saying is that using:
gst_query_add_allocation_pool (query, NULL, preferred_size, 0, 0);
sound like a hack to me (even though pool=NULL is allowed). I am mostly concerned that it won't be very discoverable, since the the api name suggest that we configure a pool.
Comment 18 Nicolas Dufresne (ndufresne) 2016-05-31 13:05:33 UTC
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #17)
> What I am saying is that using:
> gst_query_add_allocation_pool (query, NULL, preferred_size, 0, 0);
> sound like a hack to me (even though pool=NULL is allowed). I am mostly
> concerned that it won't be very discoverable, since the the api name suggest
> that we configure a pool.

We already do that to expose min/max buffer. It's not a hack. Btw, you should set at least 1, 0, otherwise it's not a valid config for a pool. Audio decoder base class should have an proper_allocation() virtual method to make it easy to implement.
Comment 19 Nicolas Dufresne (ndufresne) 2016-05-31 13:07:19 UTC
And as per size semantic, filters should only increase it. Reducing that will likely cause problems to those that required more. Obiously, for audio, this is best effort, as we don't want to copy just to add more padding, and we don't want to force upstream element to implement accumulation (which forces exposing more latency).
Comment 20 Stefan Sauer (gstreamer, gtkdoc dev) 2016-05-31 19:33:21 UTC
Re min,max=0, that is what gstvideoencoder,gstvideofilter does:
gst_query_add_allocation_pool (query, pool, size, 0, 0);

Anyway, we shoudl at least update the docs if API that has 'pool' in the name is supposed to be used to configure things that are not related to pools.
Comment 21 Nicolas Dufresne (ndufresne) 2016-05-31 20:18:51 UTC
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #20)
> Re min,max=0, that is what gstvideoencoder,gstvideofilter does:
> gst_query_add_allocation_pool (query, pool, size, 0, 0);
> 
> Anyway, we shoudl at least update the docs if API that has 'pool' in the
> name is supposed to be used to configure things that are not related to
> pools.

Why do you say it's not related to pools ? When using fixed size buffers, we should always use a pool to allocate. Those are parameters that could be passed to our local pool if downstream does not have a pool. But also, sometimes the pool won't be NULL here. It's just that for audio it should have the semantic that if possible, you use that extra space. If not, you'll be better allocating smaller chunk (otherwise it's a waste).
Comment 22 Olivier Crête 2016-05-31 20:40:10 UTC
I think the size here is not what you want. For audio encoder, you want an exact multiple of "size", forcing each buffer to be exactly "size" may explode the number of buffers and kill performance. Also, for my RTP ptime use-case, this use of the size in the allocation doesn't really work as payloader normally operate on encoded data which case be varying size in bytes. We may want to add an allocation meta with a duration in time instead of bytes.
Comment 23 Nicolas Dufresne (ndufresne) 2016-05-31 21:16:42 UTC
(In reply to Olivier Crête from comment #22)
> I think the size here is not what you want. For audio encoder, you want an
> exact multiple of "size", forcing each buffer to be exactly "size" may
> explode the number of buffers and kill performance. Also, for my RTP ptime
> use-case, this use of the size in the allocation doesn't really work as
> payloader normally operate on encoded data which case be varying size in
> bytes. We may want to add an allocation meta with a duration in time instead
> of bytes.

That's why I said it's best effort, a  recommendation. Element won't start buffering just because the size is bigger. Obviously, as size which is not a valid multiple of frame is not valid. It should be rounded up in general.

It's not going to explode the number of buffers, because no one will break GstBuffer in smaller one if downstream suggest a smaller size.

Can you better describe your ptime use-case please ?
Comment 24 Olivier Crête 2016-05-31 22:39:21 UTC
The ptime use case is if you have a pipeline like "pulsesrc ! enc ! rtppay ! application/x-rtp, ptime=20 ! udpsink ... then I'd want to have pulsesrc set a "latency-time" of 20ms.. So basically negotiate the src's latency from the caps. The goal is to have a small enough latency, but not too small so as to save battery.
Comment 25 Olivier Crête 2016-05-31 22:54:40 UTC
(In reply to Nicolas Dufresne (stormer) from comment #23)
> That's why I said it's best effort, a  recommendation. Element won't start
> buffering just because the size is bigger. Obviously, as size which is not a
> valid multiple of frame is not valid. It should be rounded up in general.
> 
> It's not going to explode the number of buffers, because no one will break
> GstBuffer in smaller one if downstream suggest a smaller size.

If they use a buffer pool and the buffer pool returns buffer of "size", then they will probably just copy "size" from the ringbuffer.. If they don't, then what's the point of this?
Comment 26 Nicolas Dufresne (ndufresne) 2016-06-01 17:49:14 UTC
(In reply to Olivier Crête from comment #25)
> If they use a buffer pool and the buffer pool returns buffer of "size", then
> they will probably just copy "size" from the ringbuffer.. If they don't,
> then what's the point of this?

Pool are configured to provide buffer of requested size. It's something an element negotiate with the pool at configuring time. If there is no agreement that make sense, the element throw away the pool. This is completely unrelated to this bug though.
Comment 27 Stefan Sauer (gstreamer, gtkdoc dev) 2016-06-02 06:57:37 UTC
(In reply to Nicolas Dufresne (stormer) from comment #21)
> (In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #20)
> > Re min,max=0, that is what gstvideoencoder,gstvideofilter does:
> > gst_query_add_allocation_pool (query, pool, size, 0, 0);
> > 
> > Anyway, we shoudl at least update the docs if API that has 'pool' in the
> > name is supposed to be used to configure things that are not related to
> > pools.
> 
> Why do you say it's not related to pools ? When using fixed size buffers, we
> should always use a pool to allocate. Those are parameters that could be
> passed to our local pool if downstream does not have a pool. But also,
> sometimes the pool won't be NULL here. It's just that for audio it should
> have the semantic that if possible, you use that extra space. If not, you'll
> be better allocating smaller chunk (otherwise it's a waste).

If the source implements a pool this is of course fine. I though you were proposing to also using for the no-pool case.
Comment 28 GStreamer system administrator 2018-11-03 12:13:11 UTC
-- 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/gstreamer/issues/10.