GNOME Bugzilla – Bug 743974
gl: rethink glfilter, glmixer, glupload, glcolorconvert, gldownload
Last modified: 2015-08-08 14:24:22 UTC
The problem I currently have with the current design of glfilter and glmixer is that they are performing the functionality of multiple elements. Along with that, the formats supported are tied closely to the implementation of glupload/glcolorconvert/gldownload. The current pipeline inside glfilter is as follows glupload ! glcolorconvert ! gl operation/s ! glcolorconvert ! gldownload with possible short cuts through each "element/object" to avoid unnecessary processing that's not feature complete. There's also the possibility of having multiple upload stages as witnessed by the dmabuf support requiring the step through EGLImage. The other concern is that we do not expose EGLImage caps on both src and sink pads and so elements like glcolorscale are not able to do EGLImage passthrough. I would like to split these different stages up into separate elements and put them all into a bin. In order to avoid losing the functionality of GstBaseTransform/GstAggregator I propose the following split of elements GstGLBaseFilter - current GstGLFilter without any format specifics. It only deals with the propagation of the GL context in the pipeline. This would also be the base class for glupload, glcolorconvert, gldownload, and any GL filter element. GstGLBase{Mixer,Aggregator,whatever} - current GstGLMixer without any format specifics. It only deals with the propagation of the GL context in the pipeline like GstGLBaseFilter. GstGLFilter - a bin like the above pipeline description that has a signal "new-element" or somesuch that the 'subclass' can connect to to create an element that performs their specific gl operation. Or even multiple operations in a row (like the sobel filters). GstGL{Mixer,Aggregator,whatever} - similar idea to the new GstGLFilter. Another advantage of the bin is the possibility of separating the upload/download into separate threads and possibly separate gl contexts which can improve throughput on certain platforms/configurations. One concern with the proposed approach is current availability of using glupload/glcolorconvert/gldownload from an independent/application perspective. I believe this use case to be a 'nice to have' rather than a hard requirement in the "new age" due to the availability of glcolorscale and appsink/appsrc.
Would this mean we get glupload and gldownload elements again? If we do (nothing wrong with that), I would like to keep the current behaviour of not requiring them and having the uploading/downloading (and conversion) be handled transparently inside the elements if needed. But having them additionally as elements allows for greater flexibility. Otherwise this all sounds good to me
(In reply to comment #1) > Would this mean we get glupload and gldownload elements again? If we do > (nothing wrong with that), I would like to keep the current behaviour of not > requiring them and having the uploading/downloading (and conversion) be handled > transparently inside the elements if needed. But having them additionally as > elements allows for greater flexibility. Yes, Yes as part of the new GstGLFilter/GstGLMixer bins and Yes :)
What about glimagesink? Is it going to become a bin too, or just stay as is?
We can also do a similar thing to glimagsink and make it a bin along with gltestsrc.
commit dbe8ae4d9851076a0322756b5e8178a2f75eeaa3 Author: Matthew Waters <matthew@centricular.com> Date: Sun Mar 8 18:16:04 2015 +1100 caopengllayersink: implement as a bin like glimagesink commit 66ccdab09a87b02d57d1fe908f522a8159712e55 Author: Matthew Waters <matthew@centricular.com> Date: Tue Mar 3 18:05:04 2015 +1100 gl/cocoa: avoid deadlock when creating context on the main thread. Make window/view creation async so that it is possible to gst_gl_context_create from the main thread. commit ef0bd30c8731ed04144ed3ae1b6c72925667570c Author: Matthew Waters <matthew@centricular.com> Date: Tue Mar 3 17:26:47 2015 +1100 gl: store the list of contexts within gldisplay Removes the reliance on the allocation query to propogate GL contexts. Allows thread safely getting a context for the a specific thread. commit 2f2470488b07f38925a55fdcc7351e139d5ac3e9 Author: Matthew Waters <matthew@centricular.com> Date: Tue Mar 3 16:48:24 2015 +1100 glimagesink: unset the current shader after rendering fixes gltestsrc ! glimagesink when gltestsrc doesn't use a shader commit 5495397c8148e1b7dea9f09e786818de23307bf6 Author: Matthew Waters <matthew@centricular.com> Date: Tue Mar 3 16:38:56 2015 +1100 gltestsrc: remove usage of gldownload library object commit cebdf84c81cb56cd7e2670e4b6564ff1e09c85ff Author: Matthew Waters <matthew@centricular.com> Date: Sat Feb 28 00:30:38 2015 +1100 glcontext: store the thread current context commit 776d190f59ea75e58bb9354d526a6c0fe5c4aee1 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 26 18:26:36 2015 +1100 gl: new glsrcbin element commit 0fb56738a14391f248aa0be8756adeaf978baa0c Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 26 13:45:56 2015 +1100 glvideomixer: implement with glmixerbin The relevant properties are forwarded to/from the containing bin and sink pads. commit d5a692bdb0f05cc789a62b2bbc365da4b2cd6520 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 26 00:20:37 2015 +1100 glmixer: remove usage of upload/download objects commit 9e605fca6c4d8f06279fbc173a0d460cb1387efd Author: Matthew Waters <matthew@centricular.com> Date: Wed Feb 25 23:48:56 2015 +1100 gl: new glmixerbin element commit 8a0017e21d5f9a8507f0593c6b24f723aa415258 Author: Matthew Waters <matthew@centricular.com> Date: Fri Feb 20 16:47:01 2015 +1100 glimagesink: implement as a bin glupload ! glcolorconvert ! sink Some properties are manually forwarded. The rest are available using GstChildProxy. The two signals are forwarded as well. commit b0600aca97e28a0595fc1cc814305c7681a347c2 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 19 18:23:37 2015 +1100 gl: new glsinkbin element similar to glfilterbin but for sinks commit 5a867ddc47b17ececbff6bf3dfc8e53c017b2196 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 19 14:19:59 2015 +1100 glfilter: don't use the library upload/convert objects commit 7fe908bc02673ec9cfe4efa81d7d770338ad9564 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 19 13:33:28 2015 +1100 gl: new element glfilterbin It encapsulates a confiurable GL processing element in the upload/colorconvert/download dance required to transparently process the majority of GstBuffer's. commit a7cbc04abae0af115dc02a075f002189865a4cfb Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 19 13:24:59 2015 +1100 gl: add new gldownloadelement Simply transforms caps to/from raw/glmemory capsfeatures commit 3514600bf30df3de319c9e572b0903cf4b847050 Author: Matthew Waters <matthew@centricular.com> Date: Thu Feb 12 17:59:27 2015 +1100 gl: add a new glcolorconvert element based on the glcolorconvert library object commit 0c800027ba844712e01de86f49cfb7c66a0ad240 Author: Matthew Waters <matthew@centricular.com> Date: Wed Mar 11 16:56:16 2015 +0000 glupload: implement propose_allocation pool handling for glmemory upload commit efaca13d11882ad2eb9ceb9cf8f0d8722d75cb88 Author: Matthew Waters <matthew@centricular.com> Date: Wed Feb 11 23:29:01 2015 +1100 gl: add a new glupload element based on the glupload library object commit 45d85a057083da227a448987bf482c7b74b38148 Author: Matthew Waters <matthew@centricular.com> Date: Wed Feb 11 14:48:45 2015 +1100 gl: add a new glbasemixer class below glmixer It deals with propagating the gl display/contexts throughout the application/pipeline commit ecdc5568c4fe5f50f7cf9060e5e65319cfe857c7 Author: Matthew Waters <matthew@centricular.com> Date: Wed Feb 11 01:48:11 2015 +1100 gl: add a new glbasefilter class below glfilter It deals with propagating the gl display/contexts throughout the application/pipeline commit 41e3b3286672bec4203b44fb8156d6e507828435 Author: Matthew Waters <matthew@centricular.com> Date: Wed Feb 11 01:27:28 2015 +1100 glutils: expose running a query on a set of src/sink pads
Why don't we make this a bit more convenient? :) We could have the glupload/download/colorconvert internal to all the elements if needed, but also have additional elements to do that. That way everything would continue to work as before, but if one wants more control over where uploads/downloads are happening the new additional elements could be inserted into the pipeline at strategic places.
Just a side note, "gst-inspect-1.0 glimagesink" now prints: Pad Templates: SINK template: 'sink' Availability: Always Capabilities: video/x-raw(ANY) It is not really friendly :)
Maybe we should revert this for 1.5/1.6 and then reconsider this for 1.7/1.8?
After thinking a bit more about this I think we should either: - revert this for 1.5 - keep it, but re-add the library API for upload/download/convert and use it in the elements on their pads again. If upstream/downstream is an glupload/download element, these wouldn't do anything and applications still have explicit control over where things are happening... but by default things will Just Work (as they did before) even without adding magic elements
(In reply to Sebastian Dröge (slomo) from comment #9) > - keep it, but re-add the library API for upload/download/convert I think it is still there or I am missing something ? Also I thought the point of glimagesink being a bin means that it adds gluploadelement and glconvertelement in its bin + glimagesink(notbinversion) ? About reverting I think it depends if Matthew can fix remaining problems (#746399, #746556, #746251, video/x-raw(ANY) in glimagesink inspect). Also I noticed a problem with vtdec, I had to put prefer_passthrough to FALSE here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/gl/gstgluploadelement.c#n113 but this is not the correct way to fix it. Generally we should check glimagesink compatibily with hw decoders, vtdec, v4l2videodec, vaapidecode ... (maybe have a try on IOS also) I would prefer that we can fix these remaining problems rather than reverting and caussing problems again later. But depends if Matthew has time to check all of it. I can my self just handle a few cases, maybe vtdec.
(In reply to Julien Isorce from comment #10) > (In reply to Sebastian Dröge (slomo) from comment #9) > > - keep it, but re-add the library API for upload/download/convert > I think it is still there or I am missing something ? Indeed, I thought it was made private API. Good then :) > Also I thought the point of glimagesink being a bin means that it adds > gluploadelement and glconvertelement in its bin + glimagesink(notbinversion) > ? Yes, but I don't see why we can't just have e.g. glimagesink directly do GstGLUpload on its sinkpad as before... and do nothing if it already receives GL memory. It seems to make things overly complicated to add these bins, and then have elements that only handle GL things.
Also these additional elements impose some overhead, sending things through more pads is not for free :)
I agree with that, in fast I thought it was supposed to make things less complicated :) In addition to the list of problems that need to be fixed (see comment #10), we also need to have glimagesink as not a bin by default (or not a bin in all cases). In order to avoid some overhead that Sebastian mentioned.
commit 87d8270f302b03f63ce04f986d824892a2c131fd Author: Matthew Waters <matthew@centricular.com> Date: Thu Apr 30 11:15:40 2015 +1000 gl: readd glupload/download onto element pads Allows insertion of gl elements into non-gl pipelines without converter (upload/download) elements. https://bugzilla.gnome.org/show_bug.cgi?id=743974 commit b4bd11f2f3a60224d188b27ab55b278077cb1217 Author: Matthew Waters <matthew@centricular.com> Date: Wed Apr 29 22:55:00 2015 +1000 Revert "glvideomixer: implement with glmixerbin" This reverts commit 0fb56738a14391f248aa0be8756adeaf978baa0c. commit be938f92d94e8acccf593128281f6e09213600a0 Author: Matthew Waters <matthew@centricular.com> Date: Wed Apr 29 22:38:00 2015 +1000 Revert "glimagesink: implement as a bin" This reverts commit 8a0017e21d5f9a8507f0593c6b24f723aa415258. commit 59fb0f830f08e3e59f87f83df8fa3c2d9f3d9741 Author: Matthew Waters <matthew@centricular.com> Date: Wed Apr 29 22:32:33 2015 +1000 Revert "glimagesink: forward ALL the properties on the bin" This reverts commit 4be45e5f30dc6121f2769323603447f591ca4a0a. commit d96e43b034a03fe54633907bc1bf2a26fe5f95fb Author: Matthew Waters <matthew@centricular.com> Date: Wed Apr 29 22:32:20 2015 +1000 Revert "glimagesink: add pixel-aspect-ratio property on the bin" This reverts commit 2ba6bb9b9325b63f58a9ff0b2c82fa28759dcabc.
It's not clear that this is actually an improvement, without also adding glcolorconvert back to the pads. Now what happens is that glimagesink only supports RGB, so when used with playbin it will do conversion from YUV to RGB with videoconvert in the CPU... and thus be a bit slow. I think we should change that back to the bins if putting glcolorconvert into the pads is no option. We just need to make sure that negotiation works properly between all the elements (especially with the caps features and allocation queries!). I don't know if there are any known remaining negotiation issues.
Arguably, playbin could learn to plug the right converter.
(In reply to Nicolas Dufresne (stormer) from comment #16) > Arguably, playbin could learn to plug the right converter. And every application using GL elements?
(In reply to Robert Swain from comment #17) > (In reply to Nicolas Dufresne (stormer) from comment #16) > > Arguably, playbin could learn to plug the right converter. > > And every application using GL elements? Depends how bad we want to screw up 1.4 user of GL elements. Having the uploader is already less bad ;-P
Arguably glupload makes things worse ;) Before it failed to link non-GL elements without adding glupload/glcolorconvert. Now it works, but is horribly slow unless you provide RGB or put in a glcolorconvert.
This is a good point.
I've reverted the reverts for now, people started to complain that things are slow on Android/iOS :/
I'm a bit confused now, what's the state, what the final consensus ? Can we help sort this out ?
The state now is that you need glupload/glcolorconvert/gldownload elements everywhere, or use the convenience bins. Which means that glimagesink supports more than RGBA again But there's no consensus or plan yet how to solve all this completely. The reverts were just to make things work again as good as we had them before ;) So question still is: a) glupload/download/convert on the pads, or b) not, but have separate elements for them only Currently we have b), in 1.4 we had a). Before the reverts today we had a) but without convert on the pads, which makes things worse than both options.
Comment 7 is fixed. Comment 10, nearly all bugs mention are fixed, remains a need info but I think the problem as with the caps which has been fixed too. Few comments speak about bin overhead, glimagesinkelement is now exposed. You can now create pipeline where you have complete control on where things are uploaded. It's handy when you want to make sure it's impossible to upload/download multiple time due to performance limitation. While there is arguably a good deal of cleanup that could be done, I wonder what is left here. In the end, looking back a bit, we now have more control, with good helpers.
(In reply to Nicolas Dufresne (stormer) from comment #24) > Comment 7 is fixed. > > Comment 10, nearly all bugs mention are fixed, remains a need info but I > think the problem as with the caps which has been fixed too. > > Few comments speak about bin overhead, glimagesinkelement is now exposed. > You can now create pipeline where you have complete control on where things > are uploaded. It's handy when you want to make sure it's impossible to > upload/download multiple time due to performance limitation. > > > While there is arguably a good deal of cleanup that could be done, I wonder > what is left here. In the end, looking back a bit, we now have more control, > with good helpers. What is the current state? Do applications need to do things manually? Or is the default behaviour usually OK? What should one look out for for situations where one would need to manually specify upload and download points?
(In reply to Robert Swain from comment #25) > What is the current state? Do applications need to do things manually? Or is > the default behaviour usually OK? Current state is the exact same as at comment #23. Nothing's changed conceptually since then, just some fixes/clean ups. For playback, nothing changed and glimagesink behaves just as before, it's just a bin now instead of a single element. Same with glvideomixer. It's when you need to use other GL elements that you need to upload/download/colorconvert either explicitly or through the helper bins (glfilterbin/glmixerbin/glsrcbin/glsinkbin). > What should one look out for for situations where one would need to manually > specify upload and download points? Other than mentioned above, currently nowhere else. There are other optimisations available when delving into multiple contexts and threads that will require explicit modelling of transfer points.
Closing after discussion with Sebastian and Matt.