Just had a very quick look, so just some random notes:

 - has no docs

 - has no tests

 - Makefile.am lacks a bunch of CFLAGS and LIBS stuff for
   libgstbase and libgstaudio/video

 - this crashes for me:
   gst-launch-0.10 audiotestsrc ! audioconvert ! synaesthesia ! ximagesink
   (seems to work with ffmpegcolorspace ! xvimagesink though)

 - the license in the header says LGPL but in DEFINE_PLUGIN
   macro "GPL" (was probably there before, but still)

 - the _CLASS macro casts to an instance structure, not to
   the class structure

 - gst_set_foo_func() should be used with GST_DEBUG_FUNCPTR

 - the _setcaps() function returns GST_PAD_LINK_* values instead
   of TRUE/FALSE as it should

 - in _setcaps() you leak object references when you return early

 - _init() should use pad_new_from_static_template() (don't remember
   when that was added, should be changed to use that though)

 - in _chain(), if pad_alloc_buffer() fails, you leak an object
   reference

Looks like it needs some more work to me.

tim, thanks for the review.

* added docs (not yet added to gst-plugins-ugl/docs/plugins as plugin is not built by default)
* fixed Makefile.am
* fixed license headers (its GPL as it is derived from GPL code)
* fixed GST_SYNAESTHESIA_CLASS macro
* added GST_DEBUG_FUNCPTR
* reflowed _setcaps
* updated pad setup in _init
* fix leak in _chain

* gst-launch-0.10 audiotestsrc ! audioconvert ! synaesthesia ! ximagesink
  works here, I am not sure why this runs, but not shows any window
  gst-launch-0.10 audiotestsrc ! audioconvert ! synaesthesia ! xvimagesink

also fixed gst-launch-0.10 audiotestsrc ! audioconvert ! synaesthesia ! xvimagesink in cvs

@tim, are you okay if I enable builing it in ugly by default (and add the docs at the same time)?

for the tests I have no idea yet, but I could do a skelleton test that feeds one buffer through, to get the valgrind coverage at least.

> @tim, are you okay if I enable builing it in ugly by default (and add the docs
> at the same time)?

I'd prefer to stick to the procedure outlined in the moving-plugins document.


The code still doesn't really look ready, and you haven't even fixed all of the things I mentioned above (setcaps still returns GST_PAD_LINK_REFUSED, no GST_DEBUG_FUNCPTR for gst_pad_set_xyz_func, still leaks references in chain function). Mostly minor things, but still.

A few more things I noticed just now:

 - in _setcaps(): "fraction" is a GstFraction, not a double

 - if output height and width are really fixed, the caps should just
   reflect that instead of claiming to support any height/width and
   then refusing everything that's not 320x240 (see _setcaps()). It's
   worth noting though that a visualisation plugin that only supports
   a fixed height/width won't work properly with totem in its current
   state. IMHO the plugin should be rewritten to be able to support
   any height/width. If it needs to close/re-init the engine when
   height/width change in order to achieve that, so be it.

 - doesn't use GST_BOILERPLATE macros

 - doesn't use GST_(DEBUG/*)_OBJECT

 - caps nego in the chain function looks dodgy, to say the least
   (lines 373ff); I think it's mostly luck and implicit assumptions
   that make it work here.

 - the entire loop bit in the chain function looks a bit messy. I
   don't really understand why there's a MAX in

    while (gst_adapter_available (synaesthesia->adapter) >
        MAX (bytesperread,
            samples_per_frame * synaesthesia->channels * sizeof (gint16))) {

   but then you gst_adapter_peek(adapter, bytesperread) and then you
    gst_adapter_flush (adapter, samples_per_frame *
        synaesthesia->channels * sizeof (gint16));

   Usually, available/peek/flush should match up.



> for the tests I have no idea yet, but I could do a skeleton test that feeds
> one buffer through, to get the valgrind coverage at least.

Yeah, something simple should be fine, although it would be good if the test
demonstrated on-the-fly output size renegotiation as well (since that's what e.g. totem does).

Another thing :)

 - in the chain function, you should check for
   samplerate == 0 and return NOT_NEGOTIATED if
   it hasn't been set to make sure you don't end
   up dividing by 0 if upstream sends buffers
   without caps and your setcaps() function
   never got called ...
   (samplerate should be reset to 0 in state
   change function too then)

Thanks. I did some more fixes:
* have separate fps_n, fps_d
* don't recalc unneccesary values
* handle sample_rate and channels better

The size negotiation still sucks as the engine has hardcoded size that are not even synced with what gets set by synaesthesia_init(). Will rewrite the engine to accept size changes.

I need to investigate the adapter size issue. Simmilar issue goom (bug #407006).

Fixing the size should be easier after I commit these changes I have here. I've moved all the global instance data into a state struct, leaving only a few precomputed globals and allowing multiple separate synaesthesia instances (in case that's ever useful ;))

Committed my changes, btw.

Status update: the biggest issue is still dynamic size renegotiation as far as I can see.

Erm, its has dynamic resize now. Please retest.

Tim, is it okay to move it in this cycle?

> Tim, is it okay to move it in this cycle?

If there was actually a move involved, I'd say "no", but since it's just removing experimental status: maybe. Can't guarantee I'll have time to look at it. Could you walk us through the checklist?

Created attachment 167501 [details] [review]
use GST_BOILERPLATE macros

(one more patch follows)

- The plug-in's code:
  (all okay)

- The plug-in's build:
  (all okay)

- The compiled plug-in:
  (ok)

- The plug-in should be put in the correct location inside the module:
  (ok)

- The plug-in is documented:
  (ok)

- The plug-in should come with tests:
  (no tests, except the generic ones - any suggestions? would a pipeline based one be okay?

- The element must not rely on a running GLib main loop
  (ok)

- The plugins need to be marked correctly for translations.
  (no translatable strings)

Created attachment 167502 [details] [review]
code cleanups

Remove unused boilerplate for signals. Use _OBJECT variants of logging macros more.

This shows considerable artifacts when resizing the window:

  gst-launch-0.10 -v uridecodebin uri=file:///home/tpm/music/foo.mp3 ! audioconvert ! synaesthesia ! ximagesink 

Doesn't crash for me, but doesn't exactly look confidence-inspiring either ;-)

Created attachment 167515 [details]
synaesthesia screenshot


After discussion on IRC we decided to keep it experimental for now:

The resize artifacts need fixing (if you can't reproduce, check window manager settings to resize while resizing without delay)

Also attached a screenshot from totem. For me this is pretty typical, no matter what the input. I don't know if it's supposed to look like this, but I suspect not. At the top it's almost always just vertical bars, while the sparkles are only in the bottom 50-80%. Also, is the colour scheme supposed to me so minimal? (basically black plus gray with a bit of blue sparkle and red sparkle thrown in)

Lastly, it would be nice to just do a clean from-scratch implementation that can be put into -good under an LGPL license. It's not much code really.

Regarding a clean from-scratch implementation, please have a look at bug #651536.

I have pushed the new plugins to bad. If people think that syaescope looks sufficiently simillar, we can just remove this plugin.

Looks fine to me (more colourful even)!

commit feb316e60421b52c51e15d3b7cfee2102d534def
Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
Date:   Sat Jun 16 18:29:54 2012 +0100

    synaesthesia: remove experimental GPL-ed synaesthesia plugin
    
    It never made it out of experimental, and there's a new
    synaescope element in gst-plugins-bad that's hopefully
    sufficiently similar.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=356882