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 758877 - glupload: Should not offer its allocator unless memory:GLMemory is negotiated
glupload: Should not offer its allocator unless memory:GLMemory is negotiated
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 745372
 
 
Reported: 2015-11-30 23:47 UTC by Nicolas Dufresne (ndufresne)
Modified: 2015-12-04 22:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
WIP query: first allocator cannot be custom (935 bytes, patch)
2015-12-01 09:18 UTC, Julien Isorce
rejected Details | Review
glupload: Only offer custom allocator with caps features (3.44 KB, patch)
2015-12-01 23:22 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2015-11-30 23:47:47 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.
Comment 1 Sebastian Dröge (slomo) 2015-12-01 06:31:10 UTC
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.
Comment 2 Julien Isorce 2015-12-01 09:18:14 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2015-12-01 10:33:12 UTC
There's GST_ALLOCATOR_FLAG_CUSTOM_ALLOC for that
Comment 4 Julien Isorce 2015-12-01 12:03:18 UTC
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));
Comment 5 Nicolas Dufresne (ndufresne) 2015-12-01 14:28:23 UTC
(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.
Comment 6 Nicolas Dufresne (ndufresne) 2015-12-01 23:22:21 UTC
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).
Comment 7 Nicolas Dufresne (ndufresne) 2015-12-01 23:25:06 UTC
Review of attachment 316578 [details] [review]:

If the caps feature is negotiated for a specific allocator, this make no sense anymore.
Comment 8 Sebastian Dröge (slomo) 2015-12-02 08:26:33 UTC
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.
Comment 9 Nicolas Dufresne (ndufresne) 2015-12-02 20:04:49 UTC
The pool should ignore and/or update the allocator in the configuration.
Comment 10 Nicolas Dufresne (ndufresne) 2015-12-02 20:08:08 UTC
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.
Comment 11 Matthew Waters (ystreet00) 2015-12-02 23:46:55 UTC
Review of attachment 316630 [details] [review]:

Looks ok to me.

There's a typo in one the comments.

::: gst-libs/gst/gl/gstglupload.c
@@ +439,1 @@
+  /* Only offer out custom allocator if that type of memory was negotiated. */

s/out/our/
Comment 12 Nicolas Dufresne (ndufresne) 2015-12-04 22:09:36 UTC
Attachment 316630 [details] pushed as d84d170 - glupload: Only offer custom allocator with caps features