Some problems I see with that code, ordered by relevance:

The dispose handler of GObjects must cope with being called multiple times.  Set the pointers you g_free () to NULL afterwards.

You need to implement basetransform's event () vfunc and watch out for EOS.  Perform what ever you do in stop () if you get one (see below).

The time interval property value is stored in gdouble instead of guint64, this makes no sense.  Even if it was needed, the value is handled totally wrong.

Implement basetransform's stop () vfunc and use gst_adapter_clear () there (instead of in start ()).  Also do this on EOS.

filter->width is set in _set_caps (), but never used (element supports 16 bit only, with 16 bit depth only, so there is indeed no need to inspect the configured width).

The code still uses GST_DEBUG in _transform_ip ().

In _init (), use "GST_MSECOND * 100" instead of "GST_SECOND / 10".

In _set_caps (), wanted = ..., use "sizeof (gint16)" instead of "2" for clarity.

In _init (), g_malloc () is used and memset (..., 0, ...) directly after that.  Just use g_malloc0 instead.

In _set_property (), case PROP_BANDS, just use g_realloc.

René, many thanks for the review!

I've changed this in CVS so far.
- use g_malloc0() instead of g_malloc() + memset()
- added the missing g_free() in dispose
- downgraded the GST_DEBUG to GST_LOG in transform_ip()
- use "sizeof (gint16)" instead of "2" in transform_ip()
- removed "width" variable and querying

Please note that I tried to make tha elemnt to be sort of similar to GstLevel (plugins-good).
Some comment about the other changes:
- event() is optional. see api docs. stop() gets called on EOS
- gst_adapter_clear() in stop() just saves the initial gst_adapter_clear() call
- "GST_SECOND / 10" is easier to understand and the result is the same (its a constant expression that gets resolved at compile time)
- g_realloc() copies the data, which we don't want

http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-bad/gst/spectrum/gstspectrum.c.diff?r1=1.24&r2=1.25
http://freedesktop.org/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins-bad/gst/spectrum/gstspectrum.h.diff?r1=1.8&r2=1.9

Also fixed intervall usage in both spectrum and level. Thanks for spotting it.

(In reply to comment #2)
[...]
> - downgraded the GST_DEBUG to GST_LOG in transform_ip()

Why not GST_DEBUG_OBJECT?

> Some comment about the other changes:
> - event() is optional. see api docs. stop() gets called on EOS

Wrong.  stop () only gets called on the state change from PAUSED to READY.  If your transformation implementation carries state between invocations, you are responsible to clear it on EOS youself.  In this case, the data left in the adapter is to be dropped since it belongs to the old stream.

> - gst_adapter_clear() in stop() just saves the initial gst_adapter_clear() call

This is not about saving anything, it is just more logical in stop because from PAUSED to READY, you are supposed to give up on stream specific resources.  Why keep it around all the time we linger in READY or NULL if we drop it when going back to PAUSED, PLAYING anyways? Just drop it at once.

> - "GST_SECOND / 10" is easier to understand and the result is the same (its a
> constant expression that gets resolved at compile time)

Multiplying integers is always cleaner than dividing.  This is also why there are all the GST_SECOND, GST_MSECOND, GST_USECOND, etc.  You can always pick the unit closest to your usage (to keep it easy understandable) and just multiply.

> - g_realloc() copies the data, which we don't want

Yes I noted that afterwards.  But your assertion that we don't want it is incorrect.  We do not _need_ it, but it won't break anything if it's moved over.

Why GST_EBUG_OBJECT() this imho ofen just bloats debugs logs. How many analyzers are you using at the same time. As transform_ip() will be called a multiple time LOG() seems to be fine.

Regarding overriding the event-func(). When EOS gets emitted, it will be caught be the app and then the state-change happens. So I don't yet agree that we have to handle EOS.

regarding GST_SECOND/10 vs. GST_MSECOND*100, maybe you should have a look at how GST_MSECOND is defined :)

Finally g_realloc(): we don't *want* to copy data we will overwrite :) Lets not nitpick too much, this is just fine as it is.

I'll do the stop() method for the adapter_clear(). As a side note, theres a dozen plugins that also clear() on start() (or PAUSED_TO_PLAYING).

(In reply to comment #5)
> Why GST_EBUG_OBJECT() this imho ofen just bloats debugs logs. How many
> analyzers are you using at the same time. As transform_ip() will be called a
> multiple time LOG() seems to be fine.

This tells us that the message is superfluous and can be omitted anyways.  The transform function should indeed produce at most 1 message IMHO.

> Regarding overriding the event-func(). When EOS gets emitted, it will be caught
> be the app and then the state-change happens. So I don't yet agree that we have
> to handle EOS.

You don't know that the app takes down the state of the whole pipeline at EOS.  Can you point out where this is stated as a hard requirement?  I just put months of work into an element that relies on that behaviour.

> 
> regarding GST_SECOND/10 vs. GST_MSECOND*100, maybe you should have a look at
> how GST_MSECOND is defined :)
> 

It doesn't really matter how it is defined.  Definitions can change.  Never said the current practice breakes anything and will break anyhow.  Multiplying is really more clean for obvious reasons, but granted.

> Finally g_realloc(): we don't *want* to copy data we will overwrite :) Lets not
> nitpick too much, this is just fine as it is.
> 

Granted.

> I'll do the stop() method for the adapter_clear(). As a side note, theres a
> dozen plugins that also clear() on start() (or PAUSED_TO_PLAYING).
> 

Have to grant that my argument works both ways.  It's "why bother cleaning up if we might die anyways" vs. "why keep something around we don't need later anyways"  or maybe rather "why delay the startup for something we could have dropped later".  Still makes a bit more sense in stop.

Dismiss my arguments about integer division, had some example in my head that really doesn't apply here.  Division is actually harder to break, 64bit ints are a joy :-)

Since I've just by chance come across the spectrum code, some random comments:

 - the code looks messy in places, with lots of commented
   out stuff / left-over cruft

 - there's no locking being done anywhere whatsoever, that
   can't be right

 - weird GST_LOG stuff: what's with GST_LOG ("               hello")?

 - GST_LOG etc. used instead of GST_LOG_OBJECT

 - don't know if this has been discussed before, but since spectrum is
   now effectively a sink, would it make sense to derive it from
   GstBaseSink instead of GstBaseTransform?

 - I suppose the (IMHO somewhat awkward) thing about having a
   GST_TYPE_LIST of G_TYPE_UCHAR has been done with language bindings
   in mind? Any reason not to just use a GST_TYPE_BUFFER instead, which
   bindings should be able to wrap fine as well (if I'm not mistaken)?

 - if you eat all events you probably need to unref them if you want
   to avoid leaks

About the sink vs. passthrough transform elements: This is a general question, and it should not make any difference in theory.  That is, you can easily turn a passthrough filter into a sink by linking a fakesink to the srcpad.  The other way around: You can easily turn any sink into a passthrough filter by linking a tee element before, although I think this can have side-effects due to limitations in tee (which may partly be unsolvalbe? don't know).  Because buffer data is immutable in gstreamer, this is equally efficient at least since the tee element does not copy any data (you know that of course, I just want to mention it generally).

I was under the assumption that analyzers generally make the most sense to be implemented as passthrough filters (like level).  The only exception I see is goom, which is rather a sink for audio and a source for video.  Thinking more about it, people should usually link analyzers into their pipelines by using "tee ! audioconvert ! audioresample ! analyzer ! fakesink".

About the data format of the message: I was actually about to propose to change to a more format agnostic layout, that is, it should output the dB result values rather in gdouble.  Stefan mentioned on IRC that the element might optionally get a more accurate analysis capability by using a library in the future.  Will be far more easy to not have any applications using this have to change the message parsing.  The fact that spectrum currently just outputs integer dB values should not matter, it's detail that can change in the future.

Tim: Where are events eaten here?

2006-10-06  Stefan Kost  <ensonic@users.sf.net>

	* gst/spectrum/gstspectrum.c: (gst_spectrum_init),
	(gst_spectrum_transform_ip):
	  Removed cruft code that was just commented out. Removed some obsolete
	  debug logs statements.

tim, thanks for the review. I did some more cleanups. I could not find the mentioned GST_LOG ("               hello") statement.

Regarding the sink. IMHO its easier to just link a bunch of different analysers than to use a tee and have them in parallel (more memcopies). The chain gets terminated using a fakesink.

Rene also commented on the current way of returning results. Returning a GstBuffer sounds quite freeform to me (like a db blob), but has the advantage of having less overhead. need to think about this.

Finally, I am quite sure the event handling (gst_spectrum_event()) is right. If not it done wrong all over the place in gst. Are we talking about the same thing?

Stefan: Show me exactly where there are additional copies made if you tee them in parallel.  Copies do not happen if the analyzers are really in passthrough mode (otherwise they are broken).  That's the whole point of having the whole framework based around immutable data structures, no?

Yes, sorry, ignore my comments about events and deriving from GstBaseSink, I was confused ;) Having a pass-through analyser makes sense.

Stefan: I wasn't talking about a particular log statement, I was wondering why there were a bunch of log statements with lots of spaces at the beginning. You seem to have removed those entirely now though, so that's not an issue any longer :)

I can see how the current GValue list stuff allows for more flexibility in the future. Btw, would a GST_TYPE_ARRAY (orderer set of values that make sense together) fit better than a GST_TYPE_LIST (unordered set of values)?

Two more things :)

 - with the following pipelines:
     $ gst-launch-0.10 audiotestsrc ! audioconvert ! spectrum message=true interval=5000000000 ! fakesink -m 
   or
     $ gst-launch-0.10 filesrc location=foo.og ! decodebin ! audioconvert ! spectrum message=true interval=5000000000 ! fakesink -m

   the spectrum data in the message seems to be always 0.
   Is that how it's supposed to be?

 - is there a unit test or test case/demo somewhere that I've missed?

There is no serialisation for G_TYPE_UCHAR and just adding
CREATE_USERIALIZATION (uchar, UCHAR); to gstvalue.c does not work, as there is no
G_MINUCHAR and G_MAXUCHAR defined in glib.

There are two demos in the source dir that should later go to tests/examples/

> There is no serialisation for G_TYPE_UCHAR ...

It's not a serialisation problem, it's due to the default threshold. If you use a different threshold (like the demos do) it's all fine.

Reading through the demo-audiotest.c file, I think there is a confusing comment in the message handler:

  /* we handled the message we want, and ignored the ones we didn't want.
   * so the core can unref the message for us */
  return TRUE;


If I understand the docs for gst_bus_add_watch() and GstBusFunc correctly, the returned boolean indicates whether the handler should be removed from the main loop or not. The message is always unref-ed.

yes the comment is misleading. Quite likely copy'n'pasted from somewhere. Fixed in CVS.

did the following in CVS:

        * gst/spectrum/gstspectrum.c: (gst_spectrum_class_init),
        (gst_spectrum_init), (gst_spectrum_set_property),
        (gst_spectrum_transform_ip):
        Fix and cleanup default property values.
        Add FIXMEs for stuff that looks rather wrong.

I'm assuming this one is not yet ready for a move ?

There was one FIXME which now is fixed :)

> There was one FIXME which now is fixed :)

Really? :)

  tpm@flap:~/gst-plugins-bad/gst/spectrum$ grep -iHn FIXME *c
  fix_fft.c:274:  fixed *pa = hpa;              /* FIXME */
  gstspectrum.c:240:      /* FIXME, this will segfault when the transform function  is running */
  gstspectrum.c:400:  /* FIXME: 4.0 was 2.0 before, but that include the mirrored spectrum */

At least the second one looks like it needs fixing given that there's not a single LOCK being taken anywhere in the file.


Also, there should be a simple unit test in tests/check/elements/ IMHO, so that at least the basic code paths get run through valgrind on a regular basis and are checked for leaks etc.

Small nitpick: should ->len and ->base maybe be turned into #defines given that they aren't configurable or changed anywhere?

tim, look at CVS, there is a lock - punto. besides feel free do to any other improvemnets you belive that vthey are needed.

okay. the defines make sense (done in cvs). Unit test will come a little later.

I have a unit-test + a small fix and an itch. Right now the spectrum data is send a guchar-values in the message. If someone ever plugs libfftw or something similar in, than this might be no sufficient. Maybe its better to have floats with db values here.

float please.  guchar makes no sense.

See bug #468619, it has a patch doing the guchar->float change and many other improvements. I guess after this is all done and the element is cleaned up a bit more it is perfect.

The event handling seems to need some work:

(bt-edit:9250): GStreamer-WARNING **: pad spectrum_0x84e7818:src sending tag event in wrong direction

I also noticed that running spectrum in a pipeline that loops (using segmented-seeks) breaks.

(In reply to comment #28)
> The event handling seems to need some work:
> 
> (bt-edit:9250): GStreamer-WARNING **: pad spectrum_0x84e7818:src sending tag
> event in wrong direction

This doesn't make sense. spectrum uses basetransform for all event stuff, the only custom event handling is saving of the newsegment event's segment.

Can you attach a GST_DEBUG=5 log with the relevant part here?

> I also noticed that running spectrum in a pipeline that loops (using
> segmented-seeks) breaks.

What exactly breaks with segmented seeks? spectrum only uses the newsegment event's segment for gst_segment_to_running_time() to get the current timestamp, other than that it's all basetransform stuff.

Should be ready for a move to gst-plugins-good too now... thus marking as a blocker bug for the next release.

The element documentation says:
"The message's structure contains two fields", and then confusingly lists 3 items.  It also doesn't mention how the 'message-magnitude' and 'message-phase' properties affect the message(s) sent.

It should include sample code, since the docs say "It isn't possible to use this element in gst-launch lines in a sensible way"

The tests and code otherwise look good. +1 for move.

We'll fix the docs. Have you seen the included demo apps (demo-audiotest/demo-osssrc)? they should help to understand the usage. or would you like to have a code-snippet in the docs?

> Have you seen the included demo apps (demo-audiotest/demo-osssrc)?

I wonder whether those should be moved to tests/examples/ or so when the move happens. (</bikeshed>)

The plugin-moving checklist calls for either gst-launch lines or a code snippet in the docs themselves to show usage, which I think sounds good.

I'm not going to let it stop me doing the move though.

Plugin is now moved. I'm leaving the bug open as a blocker against -good until we get some kind of simple sample code into the docs, and the fixed descriptions of how the messages and properties work.

docs fixed:

2008-02-09  Jan Schmidt  <jan.schmidt@sun.com>

        * docs/plugins/Makefile.am:
        * gst/spectrum/gstspectrum.c:
        * tests/examples/spectrum/.cvsignore:
        * tests/examples/spectrum/Makefile.am:
        * tests/examples/spectrum/spectrum-example.c:

        Add a simple example application for the spectrum element, include it
        in the docs, and fix some documentation ambiguities.

        Fixes: #348085