Created attachment 85844 [details] [review]
Proof of concept patch implementing equalizer.

This is simply a proof-of-concept patch to demonstrate how to hook together the equalizer in Banshee. It's rough around the edges, so there might be a few issues, however, once the code in the GStreamer glue code is cleaned up and working, it should be pretty close to be able to be commited (I hope). Preamp is yet to be implemented.

This patch adds an optional prequisite, gstreamer0.10-plugins-bad from CVS (since the last stable release does not include the required plugin). Preamp should use ladspa-PreampIII available from the same package, when eventually implemented.

GStreamer pipeline to demonstrate functionality: 'gst-launch-0.10 filesrc location='blah.ogg' ! decodebin ! audioconvert ! ladspa-PreampIII gain=2 ! audioconvert ! equalizer-nbands ! alsasink

Bring on the critism/comments..! :)

Created attachment 87026 [details] [review]
Ivan's Patch

* EqualizerSupported and EqualizerBandCount

When I was initially hacking on the equalization abock said he would
like to have as an optional interface that a MediaEngine can
implement, so IEqualizer was born (and if it is not implemented there
is no equalization support).

* nband-equalizer

> My GStreamer code is fairly messy, and your implementation is much,
> much cleaner. However, I would have liked to see equalizer-nbands,
> rather than equalizer-10bands, since it may be possible later to have
> equalizers with an adjustable number of frequency bands.
> g_object_set(G_OBJECT(engine->equalizer), "num-bands", 10, NULL); can
> be used to emulate the current implementation.
>

Initially I implemented the equalizer support for my Xine media engine
and for Helix. They both have different number of bands and different
ranges of gains. So it was decided to have a fixed 10 band equalizer
with fixed frequencies and all the media engines should adapt to that.

* Preference Loading

> My code also has the added functionality to load preset values from
> the last session, enable/disable the equalizer, as well as hiding the
> menu item depending on if the GStreamer element can be found.
>

My personal opinion is that EqualizerView and EqualizerPresetComboBox
(and everything in the Equalizer.GUI namespace) are purely UI
controls, so they should not have the preference loading logic or any
other logic. All the logic should be in EqualizerManager.

> Hopefully the best bits of both patches can be merged, and then
> possibly work on the preamp started.

If you are okay with it, you can merge the following with my patch and
we can continue discussing any further issues:

 * fill in the amplifier related methods in gst-playback with the amp code
 * schema modifications

Regards.

P.S: There is a bug GstChildProxy which prevents the equalizer from working, which was fixed yesterday in CVS. I
haven't yet tried if the equ works now (I don't have CVS of
gstreamer). I think i might have to change the code to pass gdouble
instead of a GValue as well.

P.S2: The attached is a slightly updated patch with minor changes in gst_playback_set_equalizer_gain

I've attempted to add support for the preamp, but no luck.
It seems to be adding the element to the playbin correctly, but isn't linking up right. I've also added audioconvert on either side of the element, which should let it link with the other elements in the playbin fine, but again to no avail.

Otherwise, the rest of this patch is pretty much done.

Any ideas?

Mind attaching the new patch as well? :-)

You should update the patch to the latest changes in GStreamer CVS, most notably the change of the gain property. It's now a dB value between -24.0 and +12.0.
Also, you should read the band frequencies instead of hardcoding them (or at least set frequency and bandwidth to your values instead). Yours are wrong anyway btw ;)

You can get the frequency of a band in C by:

gfloat freq;
GstObject *band = gst_child_proxy_get_child_by_index (GST_OBJECT (equalizer), i);
g_object_get (G_OBJECT (band), "freq", &freq, NULL);
g_object_unref (G_OBJECT (band));

For a larger example see:
http://webcvs.freedesktop.org/gstreamer/gst-plugins-bad/gst/equalizer/demo.c?view=markup

Setting frequency and bandwidth ("bandwidth" property) works similar but I would suggest to use the default frequencies.

And then I would suggest to use a dB scale in banshee too, it's more intuitive than some random -100 to 100 scale.



Why do you want to add a preamp btw? What's different from the normal volume?

If you need some help with the C glue just ask btw, what exactly is not working, what should be done?

The main reason you'd normally want a preamp is so that you can adjust the gain before putting it through the equaliser. You can then boost up the modified signal using the volume control.

I'll look at cleaning up my patches (some awful code there ;). I'll also try and catch you on IRC within the next few days if I need a hand doing the pipeline stuff.

Cheers.

Great, you can find me in #banshee most of the time. If I'm not there just write me a mail and you should get a rather fast answer...

Created attachment 99264 [details] [review]
Updated equalizer patch

This is against SVN trunk.

Basically, a much cleaner patch containing elements of both original patches, and fixing the issues Sebastian raised. The code should be generally self explainatory.

The only thing I think that should be cleaned up a bit is the gst_equalizer_get_frequencies(...) function. At the moment, we return the pointer to each gdouble containing the frequency of each band; I'd like to return a gdouble array, but then we'd have to have custom marshalling, wouldn't we? Otherwise, I'm sure that could be trivially fixed.

Few minor changes to PlayerEngine API included in patch:
 * Abstract class PlayerEngine requires engines to provide bool SupportsEqualizer { get; }
 * New event for StateChanged - PlayerEngineState.Initalized
 * Engines that support the equalizer are required to implement IEqualizer (which has also been changed)

+    *min = (gdouble *)-12;
+    *max = (gdouble *)12;

these lines look wrong, you probably don't want the asterisk before the closing bracket ;)

Apart from that, what's so bad about the -24 to 12 dB range? Why is it bad that zero gain is not in the middle? :)
Otherwise it looks good from a short look... you could only simplify the frequency getter... IIRC you can do g_object_get (equalizer, "band0::freq", &freq0, "band1::freq", &freq1, ......)

Created attachment 99380 [details] [review]
Updated version of previous patch

Few minor changes to the C glue, specifically:
 * gst_equalizer_get_frequencies(..) returns an gdouble array instead.
 * gst_equalizer_get_bandrange(..) determines the current band range depending on the equalizer elemnt, which means we support both the current element in CVS, as well as the old(er) one.

*** Bug 506674 has been marked as a duplicate of this bug. ***

Created attachment 103387 [details] [review]
Final equalizer patch.

Changes from previous patch:

* Applies cleanly to trunk.
* Creates a new preset if none exist (instead of making the dialog look a bit funny and forcing a creation of the preset by the user manually).
* Few formatting cleanups in the GStreamer code.
* Fix up returning a null double array after marshalling.
* Return the correct range for the older equalizer element (and added few other fallback cases).
* Added ChangeLog entry.

Alexander,

Thanks for the patch.  I see you're targeting trunk, which is great.  I would prefer to use "svn mv" to move source files from src/Core/Banshee.Base into the new structure where possible.  What do you think?  If you're ok with that, can you tell me what files should be svn mv'd and then produce a patch after I've moved them?

Created attachment 103497 [details] [review]
[Part 1/2] Patch to update equalizer GUI and logic files after mv.

Sure!

I only just remembered that the code was still in src/Core/Banshee.Base, and that a move would probably be easier (and a less hefty diff, hehe).

The following files need to be moved first:

src/Core/Banshee.Base/Banshee.Equalizer.Gui/*.cs -> src/Core/Banshee.ThickClient/Banshee.Equalizer.Gui/

src/Core/Banshee.Base/Banshee.Equalizer/EqualizerManager.cs -> src/Core/Banshee.Services/Banshee.Equalizer/EqualizerManager.cs

Once that's done, apply the diff attached to patch the Banshee.Equalizer(.Gui) code, and svn add src/Core/Banshee.Services/Banshee.Equalizer/EqualizerSetting.cs and EqualizerSettingEvent.cs created by the patch.

Then, apply the second diff to hook up the engine with the equalizer logic parts.

You might have to fiddle with the ChangeLog, now. :)

Created attachment 103498 [details] [review]
[Part 2/2] Hook up Engine logic, patch Makefiles to build things, EqualizerSetting*

Committed, thanks so much Alex, everybody.

Is it included in Banshee 1 Alpha 3?

Yes but it's disabled currently because of gstreamer or alsa issues that nobody fixed yet. There's another bugreport for this which I can't find ATM :/

Quit funny... Old XMMS does this from years, while GStreamer can't implement it :> .

It is implemented just fine in gstreamer and other applications like sonance and songbird (iirc) are using it already without problems. And for rhythmbox at least a patch exists.

IMHO the problem that people had with banshee are caused by an already fixed bug in alsa-lib but nobody of the people who had this bug could confirm or decline that yet *shrug*