GNOME Bugzilla – Bug 704507
glimagesink: Replace pointer properties with signals
Last modified: 2014-06-12 02:55:03 UTC
These patches also update the tests, I didn't compile qt examples though. The last one removes client-reshape-callback in gstglfilterapp as well, as it was never called.
Created attachment 249574 [details] [review] gstglimagesink: replace vmethods with callbacks.
Created attachment 249575 [details] [review] examples: Port to using callbacks instead of vmethods. And remove the usage of reshape-callback on gstglfilterapp as it was never called.
Created attachment 249576 [details] [review] glfilterapp: remove the property client-reshape-callback. As it is never used. Also remove the typedef in filterutils
Comment on attachment 249574 [details] [review] gstglimagesink: replace vmethods with callbacks. Looks good but please describe in the docs of these signals what the return value means :)
Comment on attachment 249576 [details] [review] glfilterapp: remove the property client-reshape-callback. Please also get rid of the pointer property and replace it with a signal while you're at it :)
Also fix the commit message of the first commit please, you replace properties with signals, not vfuncs with callbacks ;)
Created attachment 249656 [details] [review] Patch to update the recordgraphic example. I'll do all this but I've got a problem: To remove the client-draw-callback as well in gstglfilterapp, I think it would make sense to refactor a little the way the callback is handled. Currently it's passed along with 15 other arguments to gst_gldisplay_use_fbo, which in turn passes it to gst_glframebuffer_use, and I'm not sure this is optimal. On that codepath ther's also an intermediary step which I don't understand the purpose of, namely stuff all these arguments in a structure just to unpack them and pass them as arguments in another function, I don't see how that makes sense. (Actually I think it doesn't :) So yeah I kind of don't want to go and messup with all this too much before someone else with more knowledge of the code base comes and does a little cleanup, and when done I'd like to rebase my history a little, no point in having two different commits for gstglfilterapp IMHO. I think I'll rename the commits etc.. during that rebase, except if we decide to switch to using a signal for now. the attachment I added allows me to run the only example using this client-draw-callback.
Except if we decide *not* to switch.
(In reply to comment #7) > Created an attachment (id=249656) [details] [review] > Patch to update the recordgraphic example. > > I'll do all this but I've got a problem: > > To remove the client-draw-callback as well in gstglfilterapp, I think it would > make sense to refactor a little the way the callback is handled. > > Currently it's passed along with 15 other arguments to gst_gldisplay_use_fbo, > which in turn passes it to gst_glframebuffer_use, and I'm not sure this is > optimal. On that codepath ther's also an intermediary step which I don't > understand the purpose of, namely stuff all these arguments in a structure just > to unpack them and pass them as arguments in another function, I don't see how > that makes sense. (Actually I think it doesn't :) The reason it is stuffed into a structure is to perform the gst_gl_framebuffer_use function in the GL thread which may be different from the calling thread. Ideally, that function should be split up into set matrix, set texture attachements, etc and be moved into gstglframebuffer.c. Eventually gstglutils.c should be incinerated, it was and still is a temporary solution for speeding along the process of using GstContext. > So yeah I kind of don't want to go and messup with all this too much before > someone else with more knowledge of the code base comes and does a little > cleanup, and when done I'd like to rebase my history a little, no point in > having two different commits for gstglfilterapp IMHO. I think I'll rename the > commits etc.. during that rebase, except if we decide to switch to using a > signal for now. That might take a while :) > the attachment I added allows me to run the only example using this > client-draw-callback.
> The reason it is stuffed into a structure is to perform the > gst_gl_framebuffer_use function in the GL thread which may be different from > the calling thread. That's tru sorry didn't read the code attentively enough my bad :) > Ideally, that function should be split up into set matrix, > set texture attachements, etc and be moved into gstglframebuffer.c. Eventually > gstglutils.c should be incinerated, it was and still is a temporary solution > for speeding along the process of using GstContext. > That might take a while :) Hm OK, I'll try to find an interim solution to remove the property, which will already remove arguments from that function call anyway :)
Are you going to update the patches, Mathieu?
Mathieu?
Is this something that should be done before the release ideally?
Yes, definitely
commit e36f5b13267b9e4a955af7c24a2ec6326c823a1b Author: Matthew Waters <ystreet00@gmail.com> Date: Thu Jun 12 12:09:56 2014 +1000 gl/examples: Port to using signals instead of properties Based on patch by Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> https://bugzilla.gnome.org/show_bug.cgi?id=704507 commit 2ed84ac40f78d8f57352c2b66cd4127e7261986d Author: Matthew Waters <ystreet00@gmail.com> Date: Sun Jun 1 15:02:52 2014 +1000 glimagesink: replace pointer properties with signals Based on patch by Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> https://bugzilla.gnome.org/show_bug.cgi?id=704507 commit d7f8cc9a78eff6e0e2cd6b5f050e4e630b913d5b Author: Matthew Waters <ystreet00@gmail.com> Date: Thu Jun 12 12:49:42 2014 +1000 glfilterapp: remove the reshape/draw properties The reshape property was never used. Replace the draw property with a signal. Based on patch by Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> https://bugzilla.gnome.org/show_bug.cgi?id=704507