After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 348085 - [PLUGIN-MOVE] move spectrum to good
[PLUGIN-MOVE] move spectrum to good
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.10.7
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2006-07-20 06:10 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2008-02-09 01:45 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Stefan Sauer (gstreamer, gtkdoc dev) 2006-07-20 06:10:33 UTC
spectrum has been ported according to the requirements. please review and sponsor the move.
Comment 1 René Stadler 2006-09-10 02:53:52 UTC
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.

Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2006-09-11 18:05:39 UTC
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
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2006-09-11 18:12:22 UTC
Also fixed intervall usage in both spectrum and level. Thanks for spotting it.
Comment 4 René Stadler 2006-09-11 18:37:25 UTC
(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.
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2006-09-12 18:17:33 UTC
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).
Comment 6 René Stadler 2006-09-12 19:12:49 UTC
(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.
Comment 7 René Stadler 2006-09-13 02:05:46 UTC
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 :-)
Comment 8 Tim-Philipp Müller 2006-10-05 17:40:28 UTC
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


Comment 9 René Stadler 2006-10-05 18:18:46 UTC
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.
Comment 10 René Stadler 2006-10-05 18:36:04 UTC
Tim: Where are events eaten here?
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2006-10-06 02:36:02 UTC
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?
Comment 12 René Stadler 2006-10-06 05:25:53 UTC
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?
Comment 13 Tim-Philipp Müller 2006-10-06 08:33:19 UTC
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)?
Comment 14 Tim-Philipp Müller 2006-10-06 11:38:26 UTC
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?

Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2006-10-06 21:23:26 UTC
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/
Comment 16 Tim-Philipp Müller 2006-10-08 15:26:10 UTC
> 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.

Comment 17 Christophe Dehais 2007-03-04 21:37:01 UTC
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.
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2007-03-05 11:10:19 UTC
yes the comment is misleading. Quite likely copy'n'pasted from somewhere. Fixed in CVS.
Comment 19 Wim Taymans 2007-03-06 13:58:03 UTC
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.
Comment 20 Thomas Vander Stichele 2007-05-20 16:16:58 UTC
I'm assuming this one is not yet ready for a move ?
Comment 21 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-21 11:21:37 UTC
There was one FIXME which now is fixed :)
Comment 22 Tim-Philipp Müller 2007-05-21 11:44:29 UTC
> 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?
Comment 23 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-22 11:02:45 UTC
tim, look at CVS, there is a lock - punto. besides feel free do to any other improvemnets you belive that vthey are needed.
Comment 24 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-22 11:14:45 UTC
okay. the defines make sense (done in cvs). Unit test will come a little later.
Comment 25 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-23 07:56:52 UTC
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.
Comment 26 David Schleef 2007-05-23 17:59:10 UTC
float please.  guchar makes no sense.
Comment 27 Sebastian Dröge (slomo) 2007-08-20 18:38:41 UTC
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.
Comment 28 Stefan Sauer (gstreamer, gtkdoc dev) 2007-08-23 06:44:58 UTC
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.
Comment 29 Sebastian Dröge (slomo) 2007-08-23 08:44:33 UTC
(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.
Comment 30 Sebastian Dröge (slomo) 2007-11-11 21:12:47 UTC
Should be ready for a move to gst-plugins-good too now... thus marking as a blocker bug for the next release.
Comment 31 Jan Schmidt 2008-02-07 01:30:20 UTC
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.
Comment 32 Stefan Sauer (gstreamer, gtkdoc dev) 2008-02-07 08:48:18 UTC
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?
Comment 33 Tim-Philipp Müller 2008-02-07 09:45:49 UTC
> 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>)
Comment 34 Jan Schmidt 2008-02-07 09:51:43 UTC
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.
Comment 35 Jan Schmidt 2008-02-08 03:28:57 UTC
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.

Comment 36 Jan Schmidt 2008-02-09 01:45:44 UTC
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