GNOME Bugzilla – Bug 764361
gdpdepay: query for buffer allocator
Last modified: 2016-04-05 08:41:43 UTC
Created attachment 325004 [details] [review] Patch where gdpdepay query for buffer allocator before using default If an other element downstreams has it own allocator, the buffer created with gst_dp_buffer_from_header appears to be allocated twice. One time using the default allocator in gdpdepay, and one in the downstream element with its allocator. Perhaps this is the intended behavior, but if not, would like to propose a change to the gdpdepay element where it tries to query for an potential allocator different from the default, and use that one if it exists. Otherwise use the default allocator. Attached a patch with a suggestion on how this can be done, but don't know if that is the correct way of doing it.
Review of attachment 325004 [details] [review]: Generally makes sense, just some changes needed here :) ::: gst/gdp/gstgdpdepay.c @@ +159,3 @@ g_object_unref (this->adapter); + if (this->allocator) + gst_object_unref (this->allocator); You also should get rid of the allocator in PAUSED->READY @@ +521,3 @@ + switch (transition) { + case GST_STATE_CHANGE_READY_TO_PAUSED: + gst_gdp_depay_decide_allocation (this); This should happen after the caps are set on the srcpad... and whenever the reconfigure flag is set on the srcpad @@ +550,3 @@ + GstCaps *caps; + + caps = gst_pad_peer_query_caps (gdpdepay->srcpad, NULL); You put your own srcpad caps into the query @@ +577,3 @@ + gst_caps_unref (caps); + if (query) + gst_query_unref (query); You always have the query here
Ok! Will fix it! Would it be enough to check for an GST_EVENT_RECONFIGURE on the src pad, or would you need to do check for the reconfigure flag with for example gst_pad_check_reconfigure in the gdpdepay chain function?
See gst_pad_check_reconfigure(). You could do that at the top of the chain function
Created attachment 325067 [details] [review] Updated gdpdepay buffer allocation patch New updated patch with regards to previous patch review comments.
Comment on attachment 325067 [details] [review] Updated gdpdepay buffer allocation patch What's your use case for this btw? And in theory the payloader wouldn't have to copy anything at all, it could just forward the parts of the input data without the GDP header.
Review of attachment 325067 [details] [review]: I've pushed this with some minor changes: ::: gst/gdp/dataprotocol.c @@ +494,3 @@ GstBuffer * +gst_dp_buffer_from_header (guint header_length, const guint8 * header, + GstAllocator * allocator, GstAllocationParams allocation_params) The allocation_params should be passed by reference, not value. Copying them here for every function call is not ideal and not needed ::: gst/gdp/gstgdpdepay.c @@ +534,3 @@ gst_adapter_clear (this->adapter); + if (this->allocator) + gst_object_unref (this->allocator); You have to set the allocator to NULL after getting rid of the reference, and also the allocation parameters should be reset.
commit de3b2cc171fea6f00b33a034dfbc9d47d61f06ee Author: Christoffer Stengren <christsn.gs@gmail.com> Date: Thu Mar 31 11:21:35 2016 +0200 gdpdepay: Query for buffer allocator before using default https://bugzilla.gnome.org/show_bug.cgi?id=764361
I should first state that I Just noticed that the email address used does not reflect that I work at Axis Communications, which I do. The use case is quite simple, we want to use an other allocator than the default to allocate the buffers, but noticed when using the gdpdepay element that it were using the default allocator. To avoid having to reallocate the same buffers with the allocator we need, we though it would be nice if the gddepay would do a query to see if there was an other allocator than the default it could use. Sorry about the confusion, a bit new here to this, and thank you very much for all the help you have given!