Created attachment 104668 [details] [review]
gst-plugins-good-goom2k4.patch

for i in *.[ch] ; do if [ $i != gstgoom.c -a $i != gstgoom.h ] ; then echo cvs rm -f $i ; fi ; done

(Copy all the files over from goom2k4)

cvs add *.[ch]

Voila

What sucks:
- Too many files that probably don't need to be there
- We unset errors in the Makefile.am because yacc produces crap code, and in the rest of the code
- Optimisations still not used (see bug 475411)
- Depends on Yacc/Lex when it didn't use to (should we ship the generated .c file instead? Or completely remove the config file crap)

Is goom2k4 available as a standalone library?

It is, even installs a .pc file, but I was told I should do a copy so that we can be sure that goom is always available, as before. There's probably also quite a few files that can be trimmed compared to the original code.

I'd prefer to keep our own local copy of the code, since goom is pretty much our fallback visualisation - often there are no libvisual plugins installed.

I'm really tempted to try and push this into this release, but with only a couple of hours until the freeze I'm going to restrain myself - it's too big a rewrite. 

Punting for after the release, when we can finesse the code properly.

I'd also rather avoid the YACC/LEX dependency - we have some funky pre-generation of the gst_parse code in core to allows builds on Windows without BISON/FLEX for exactly this reason.

Created attachment 104757 [details] [review]
gst-plugins-good-goom2k4-2.patch

Biiig patch.

- Use liboil to detect CPU optimisations (fixes bug 475411)
- Use gst-arch for which optimisations to compile
- Use the same hand-made Lex/Bison generation as gstreamer/gst/parse

OK to commit now that -good reopened?

Created attachment 105747 [details] [review]
goom-makefile.diff

Well, it works good for me (with attached patch) but it's almost impossible to review this large patch IMHO.

Also the printf() in plugin_info.c should probably be GST_DEBUG or something... and the MMX detection doesn't work:

"Too bad ! No SIMD optimization available for your CPU." although my CPU supports MMX. maybe liboil needs to be initialized first or something?

btw, I'd prefer to depend on new enough flex/bison for the next -base and -good release as this hack we currently have there is scary and the flex/bison versions required are released since _ages_.

(and glib >= 2.12 or 2.10 would be nice too but that's another topic)

It occurs to me to wonder if we should keep the old goom too and put the new one in a new sub-directory, registered as goom2k4 or something.    

Why? Unless something broke compared to the old version (which should be fixed then) there would be no advantage of having the old goom. What do I miss?

Well, this one adds new dependencies, and seems in general heavier than the old one. That, plus there's some wacko out there who prefers the old look.

I think I'd like to keep it around. Maybe a better idea is to call the old one goom2k1 and this one 'goom' so people get the new one on an upgrade though ;)

(In reply to comment #12)
> Well, this one adds new dependencies, and seems in general heavier than the old
> one. That, plus there's some wacko out there who prefers the old look.

The "old look" is the same minus actually showing interesting effects, and there's no dependencies that aren't in other GStreamer packages (ie. the core gstreamer).

> I think I'd like to keep it around. Maybe a better idea is to call the old one
> goom2k1 and this one 'goom' so people get the new one on an upgrade though ;)

Fair enough, I'll do that, and fix up Sebastian's problem on x86.

2008-02-23  Bastien Nocera  <hadess@hadess.net>

        * configure.ac: Add checks for Flex/Yacc/Bison and other
        furry animals, for the new goom 2k4 based plugin

        * gst/goom/*: Update to use goom 2k4, uses liboil to detect
        CPU optimisations (not working yet), move the old plugin to...

        * gst/goom2k1/*: ... here, in case somebody is sick enough

        Fixes #515073

All fixed now.

2008-02-23  Bastien Nocera  <hadess@hadess.net>

	* gst/goom/Makefile.am: Remove the warnings being disabled,
	fix linkage on x86, spotted by Sebastian Dröge
	<slomo@circular-chaos.org>

	* gst/goom/convolve_fx.c (convolve_init),
	(create_output_with_brightness), (convolve_apply):
	* gst/goom/filters.c (zoomFilterVisualFXWrapper_create):
	* gst/goom/goomsl.c:
	* gst/goom/ifs.c (ifs_update), (ifs_visualfx_create):
	* gst/goom/plugin_info.c:
	* gst/goom/tentacle3d.c (tentacle_fx_create):
	Fix warnings, and disable the motifs in the convolve_fx
	plugin (they were causing warnings, and they were just
	"Goom" in funny letterring)

I just noticed this in ppc_drawings.s and ppc_zoom_ultimate.s: 

"This Source Code is released under the terms of the General Public License"

We need to get some clarification of that, or remove the PPC asm bits.

(In reply to comment #16)
> I just noticed this in ppc_drawings.s and ppc_zoom_ultimate.s: 
> 
> "This Source Code is released under the terms of the General Public License"
> 
> We need to get some clarification of that, or remove the PPC asm bits.

Mailed the authors about it.

Created attachment 105818 [details]
goom-lgpl.txt

Guillaume is the only copyright holder (from the headers), and he's agreed to relicense those files under the LGPL.

I just wanted to add that the old goom sucked big time. That code should be killed as it looks like garbage by comparison.
In bug 341355, I added two screenshots:
Old goom: http://bugzilla.gnome.org/attachment.cgi?id=88873&action=view
New goom: http://bugzilla.gnome.org/attachment.cgi?id=88874&action=view

And the static screenshots don't do the difference justice.

Using the standard goom2k4 codebase will also give you a better place to do any work. Libvisual is good, but they are an added dependency, don't support Goom, it doesn't know what totem is, it isn't installed out of the box, doesn't use liboil, and has its own set of issues including the fact that the website is down and no one is working on it.

I'm very glad to see this happen! I use totem because all the other music tools do too much.

> Guillaume is the only copyright holder (from the headers), and he's agreed to
> relicense those files under the LGPL.

Changed headers accordingly:

 2008-04-09  Tim-Philipp Müller  <tim at centricular dot net>

        * gst/goom/ppc_drawings.s:
        * gst/goom/ppc_zoom_ultimate.s:
          Change license of these files to LGPL, as permitted by the
          author, Guillaume Borios. See #515073.