GNOME Bugzilla – Bug 758877
glupload: Should not offer its allocator unless memory:GLMemory is negotiated
Last modified: 2015-12-04 22:13:40 UTC
Currently the glupload element always offer it's customer allocator. This allocator may endup causing the pipeline to fail inserted into generic buffer pool (like videopool) since it requires using a special alloaction m ethod.
This allocator should only be offered if memory:GLMemory was negotiated.
What was done elsewhere about this is that allocators that require custom allocation methods are only provided as a non-first allocator in the allocation query... and the sysmem allocator goes first.
Created attachment 316578 [details] [review]
WIP query: first allocator cannot be custom
WIP patch to materialize the policy that Sebastian mentioned about custom allocators.
Note that custom allocators should defines their ->alloc func to NULL like in gst_fd_allocator_class_init. Maybe we can check that too somewhere else.
There's GST_ALLOCATOR_FLAG_CUSTOM_ALLOC for that
Ah right, thx, I'll update the patch with:
+ g_return_if_fail (allocator &&
+ gst_query_get_n_allocation_params (query) == 0 &&
+ !GST_OBJECT_FLAG_IS_SET (allocator, GST_ALLOCATOR_FLAG_CUSTOM_ALLOC));
(In reply to Sebastian Dröge (slomo) from comment #1)
> What was done elsewhere about this is that allocators that require custom
> allocation methods are only provided as a non-first allocator in the
> allocation query... and the sysmem allocator goes first.
I don't think this is right if you have negotiated memory:GLMemory. I think we should never offer an allocator without an alloc method unless we have negotiated that memory type.
Created attachment 316630 [details] [review]
glupload: Only offer custom allocator with caps features
To use GLMemory and EGLImage allocators, one need to know the
libgstgl API. This is only expected if the associated caps features
have been negotiated. Generic element that otherwise receive those
allocators may fail, resulting in broken pieline. We don't want to
force all generic element to check if the allocator is a custom
allocator or a normal allocator (which implement the _alloc method).
Review of attachment 316578 [details] [review]:
If the caps feature is negotiated for a specific allocator, this make no sense anymore.
How will the GL buffer pool behave if we don't configure the GL allocator on it? Because we still want the GL buffer pool to be used, independent of the caps features.
The pool should ignore and/or update the allocator in the configuration.
The current code, saves the allocator and then ignores it completly. It calls gst_gl_memory_setup_buffer() (or gst_egl_image_memory_setup_buffer() for EGL). I think this is correct, we should remove the code that keeps the allocator as it's unused.
Review of attachment 316630 [details] [review]:
Looks ok to me.
There's a typo in one the comments.
@@ +439,1 @@
+ /* Only offer out custom allocator if that type of memory was negotiated. */
Attachment 316630 [details] pushed as d84d170 - glupload: Only offer custom allocator with caps features