GNOME Bugzilla – Bug 742875
[API] new audiovisualizer base class
Last modified: 2015-10-12 16:35:49 UTC
The audiovisualizer code is duplicated (and slightly off sync): http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/ext/libvisual/gstaudiovisualizer.c http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst/audiovisualizers/gstaudiovisualizer.c These need to be synced. All audiovisualization elements need to use this. Then it needs to be merged into a public base class. I am currently working on this. This thread is to keep track of it and avoid any work duplication.
Stefan has adviced in IRC, "basically add another copy to goom and goom2k and if all 3 copies are the same, move the copy out as a public baseclass :)"
There is also GL audio visualizer (which has not been ported to 1.x yet) which shares code with files you pointed. http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/libvisual/visual-gl.c Il will benefit from your work.
Thanks for pointing this out Julien! :) Working on this now.
Files synced. in -base: 684e5e0c8751dca5cb45a28ddd4f9aa6db01d925 945afe6cc4c4f0a7762d34ed38f15083d2124b81 in -bad: 6431de32860cc8b0e24f9973081e488fd50bdbff 1b2d74728ccdcddf8d774c0e66f12f72a6094dda Now I can cleanly merge them.
Created attachment 295213 [details] [review] patch for plugins-base
Created attachment 295214 [details] [review] patch for plugins-bad These are the two patches for the two modules so far. They build cleanly and I have tested the audiovisualizers from -bad. spacescope, spectrascope, synaescope, and wavescope. Any comments?
Luis, can you first try to port the goom visualizers using yet another copy to verify that the api is good?
Stefan, So first having a third copy that goom uses and then merge them all into the above solution (patch)? I can do that! :) Will start on it after lunch today. Thanks for looking at this.
One other thing that should be mentioned is that the current baseaudiovisualizer already maps the output buffer readwrite before calling the subclass' render() function. For GL, this essentially means downloading random data and then (possibly) uploading it again to draw to. One fix common to all the other base classes is to make the subclass deal with mapping the buffer with it's (possible) special requirements.
Another thing we need to do is make sure GAP events negotiate the output format and get forwarded cleanly. Bug 735666 - adding new trickmode modes - makes audio decoders output GAP events instead of audio buffers, which will make things not preroll with current goom (at least).
True. But let's merge code first, and then we can optimize/fix. Working on it.
Jan, goom isn't using this baseclass yet. Can you try that with an element that does - e.g. synaescope.
Created attachment 295773 [details] [review] port goom2k1 to using the new baseclass
Created attachment 295774 [details] [review] port goom to using the new baseclass
With these two now patches, we have all related plugins using the new baseclass. I know there is some cleanup to be done in goom and goom2k1, but wanted to submit this before traveling to FOSDEM. Will clean very soon and push upstream (unless anybody points out issues). With this we keep current behaviour but with everything ported to use common code. Then, I can continue the work on audiovisualizers by doing the fixes people have mentioned here. One step at a time :)
Please don't push this yet, I think we should review the APIs a bit more carefully before we make this all public. The new public structs (instance+class) need padding.
Review of attachment 295773 [details] [review]: This looks great!
Thanks Stefan! :) Tim, no problem. I am happy to review the APIs.
Created attachment 296031 [details] [review] make private all the variable the subclasses don't need Cleaning the API
Ready for review again: https://github.com/luisbg/gst-plugins-base/blob/audiovisual/gst-libs/gst/pbutils/gstaudiovisualizer.h https://github.com/luisbg/gst-plugins-base/blob/audiovisual/gst-libs/gst/pbutils/gstaudiovisualizer.c Good example in goom from -good of how the new API is used: https://github.com/luisbg/gst-plugins-good/blob/audiovisual/gst/goom/gstgoom.c https://github.com/luisbg/gst-plugins-good/blob/audiovisual/gst/goom/gstgoom.h And current stage of audiovisualizers in -bad: https://github.com/luisbg/gst-plugins-bad/tree/audiovisual/gst/audiovisualizers Thanks! :)
Can you push the patches that make the variables private? I think that makes sense in any case. It might be useful to have a local variable for "p = self->priv" in methods that do a lot "self->priv->abc".
Also for now, please just add yet another copy of the baseclass into goom and goom2k1. This way we don't loose your work. FYI: I just pushed commits to update the license, this was always meant to be LGPL.
Review of attachment 296031 [details] [review]: As Stefan suggested, pushing this now to not lose it since it doesn't change API. commit a881085a75d20e7fd6d90bb818b14e17332be5d1
Stefan, Following your advice I moved a copy of the baseclass into goom and goom2k1 to not have it bitrot. Once 1.6 opens I can unify them all into gst-libs/pbutils. commit ffe75075125ffdc06406e360f631770a85508454 Author: Luis de Bethencourt <luis.bg@samsung.com> goom2k1: remove variables not needed anymore https://bugzilla.gnome.org/show_bug.cgi?id=742875 commit 8756b6a9d42de500319822665449615342adc314 Author: Luis de Bethencourt <luis.bg@samsung.com> goom2k1: rebase to use the audiovisualizer class Rebase to have goom2k1 using the common GstAudioVisualizer class https://bugzilla.gnome.org/show_bug.cgi?id=742875 commit 89903bf66ae0eec6f5613288123d263e1d80a5c9 Author: Luis de Bethencourt <luis.bg@samsung.com> goom: rebase to use the audiovisualizer class
Thanks for submitting, did you sync all versions? find . -name "gstaudiovisualizer.c" -printf "%s %p\n" | grep -v "www" 42070 ./gst-plugins-bad/gst/audiovisualizers/gstaudiovisualizer.c 42247 ./gst-plugins-good/gst/goom2k1/gstaudiovisualizer.c 42237 ./gst-plugins-good/gst/goom/gstaudiovisualizer.c 40933 ./gst-plugins-base/ext/libvisual/gstaudiovisualizer.c
Thanks to the changes Luis made, the public api surface is much less. A few things we might want to discuss: guint req_spf; /* min samples per frame wanted by the subclass */ This could also be a property. Then the baseclass could wrap this with code to do some sanity checks. Although I don't have a good example yet for e.g. what would be out-of-bounds here. The 3 vmethods are apparently sufficient for all our visualizers. Anything that should be changed there? The shader feature might need more work for some videoformats, e.g.: gst-launch-1.0 -v audiotestsrc ! goom shader=8 ! videoconvert ! autovideosink Also for the goom and goo2k1 ports gst-launch-1.0 -v audiotestsrc ! goom shader=0 ! videoconvert ! autovideosink gst-launch-1.0 -v audiotestsrc ! goom2k1 shader=0 ! videoconvert ! autovideosink causes a black screen.
Stefan, I've updated and synced all instances of gstaudiovisualizer.c. They will still show different in your find command because each instance has a different static type name to avoid clashes. Thanks for the review. Will make req_spf a property. It is a good idea. Will fix the shader property bug you mention. Cool!
Review of attachment 295213 [details] [review]: commit 8ae0fd3990ef922b6dc8bb531cb1caf91926fec2 Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Thu Oct 1 11:55:59 2015 +0100 visual: merge audiovisalizer base classes Move the audiovisualizer base class to pbutils, so it can be used by plugins from other modules https://bugzilla.gnome.org/show_bug.cgi?id=742875
Review of attachment 295214 [details] [review]: commit 9e97267ca01e81f1ea0696e2cb4cfab0e05714dc Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Thu Oct 1 15:35:37 2015 +0100 audiovisualizers: merge audiovisualizer base classes These plugins now use the audiovisualizer base class in pbutils https://bugzilla.gnome.org/show_bug.cgi?id=742875
Review of attachment 295774 [details] [review]: commit 17c17fc369d17a0236eb92bb3d341da6bf15fbf0 Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Thu Oct 1 16:16:08 2015 +0100 goom: use the new audiovisualizer base class Rebase to have goom using the GstAudioVisualizer base class in gst-plugins-base/gst-libs/gst/pbutils https://bugzilla.gnome.org/show_bug.cgi?id=742875
Review of attachment 295773 [details] [review]: commit 711b035137a34d946d01f623ef68af4787aea6ca Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Thu Oct 1 16:24:32 2015 +0100 goom2k1: use the new audiovisualizer base class Rebase to have goom using the GstAudioVisualizer base class in gst-plugins-base/gst-libs/gst/pbutils https://bugzilla.gnome.org/show_bug.cgi?id=742875
Sorry to be late to the party. Is pbutils the best place for the base class? I would have guessed libgstaudio.
> Sorry to be late to the party. Is pbutils the best place for the base class? > > I would have guessed libgstaudio. It's not the best place, more like the least worst place. Unless you think making libgstaudio depend on libgstvideo is a good idea, libgstaudio is out.
Having said that, so far pbutils only has a 'hidden' dependency on libgstaudio/video through some trivial discoverer usage that could easily be done internally without a dependency, and with this we are adding an explicit dependency in the public headers. So perhaps there's a case to be made for adding a new mini lib for this after all. Perhaps it could be the same lib into which we then move the camerabin wrapper src base class, for example. But what to call it? ;)
Created attachment 313109 [details] [review] goom/goom2k1: remove obsolete left over files Let me know if this is ok and then I can push it.
Created attachment 313110 [details] [review] goom/goom2k1: fix undefined reference to gst_audio_visualizer_get_type Same
Review of attachment 313110 [details] [review]: Can you squash both patches together? ::: gst/goom/Makefile.am @@ +40,2 @@ libgstgoom_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_BASE_CFLAGS) $(GST_CFLAGS) $(GOOM_FILTER_CFLAGS) $(ARCH_CFLAGS) $(ORC_CFLAGS) +libgstgoom_la_LIBADD = -lgstpbutils-$(GST_API_VERSION) $(GST_PLUGINS_BASE_LIBS) $(GST_BASE_LIBS) $(GST_LIBS) $(ORC_LIBS) First $(GS_TPLUGINS_BASE_LIBS), then the lib ::: gst/goom2k1/Makefile.am @@ +24,3 @@ -DzoomFilterNew=zoomFilterNew2k1 +libgstgoom2k1_la_LIBADD = $(GST_BASE_LIBS) $(GST_PLUGINS_BASE_LIBS) -lgstpbutils-$(GST_API_VERSION) $(GST_LIBS) $(LIBM) First GST_PLUGINS_BASE, then GST_BASE, then GST
And if you remove GST_BASE_LIBS please also remove GST_BASE_CFLAGS.
commit c9df481e279982c67b3c9aace062d141375d77c9 Author: Julien Isorce <j.isorce@samsung.com> Date: Sun Oct 11 22:27:47 2015 +0100 goom/goom2k1: remove obsolete left over files They now use the new GstAudioVisualizer base class from gst-plugins-base/gst-libs/gst/pbutils Also fixed undefined reference to gst_audio_visualizer_get_type Added GST_PLUGINS_BASE_LIBS to Makefile.am and re-order LIBADD. https://bugzilla.gnome.org/show_bug.cgi?id=742875