Re: [Banshee-devel-list] Equalizer Plugin and Patches

[Aaron Bockover <abockover novell com>]

On Mon, 2006-07-17 at 17:22 +0300, Ivan N. Zlatev wrote:
> Hey,
> 
> I have hacked up an equalizer plugin for Banshee, but it requires a
> few patches in Banshee. A screenshot:
> 
> http://i-nz.net/files/gallery/software/banshee-equalizer.png

Very nice. I have been wanting to add equalizer support for a while, so
your effort is a nice push in that direction.

> 1) PlayerEngine.patch - Implements the EQ/Amplifier functionallity.
> Take a look at the patch it's self explanatory I think. Commiting this
> won't break anything existing, because implementation of the virtual
> members related to the equalizer/amplifier is optional.

I'd rather not add more members to the PlayerEngine class which are
optional for implementation. I have committed an IEqualizer interface
which player engines can implement. 

> 2) helix-dbus-engine.patch + HelixEngine.patch - Adds the equalizer
> functionallity to helix-dbus-engine and then in the HelixEngine
> itself. I haven't managed to compile hxclientkit even after reading
> the instructions.

Nice start, I have not tested yet however.

> 3) banshee-equalizer.tar.gz - The Equalizer Plugin itself consists of
> an Equalizer widget. The sliders auto-position in the middle when
> moved. The Plugin takes care of saving, checking and loading the
> equalizer preset at startup/exit.

Actually, I would not like to have equalization as a plugin. This needs
to be in the core. I also do not like having the equalizer widget show
below the library view. Instead it should probably be a separate
window. 

> 4) banshee-engine-xine.tar.gz - My LibXine backend for Banshee, which
> fully supports equalizer and amplifier. I attach this just in case you
> don't wonna bother compiling helix-dbus-engine for a quick tryout of
> the Equalizer plugin.To install and activate:

If you can re-work your implementation to use IEqualizer instead, that
would be great. You can check to see if a PlayerEngine supports
equalization through IEqualizer like this:

IEqualizer equalizer = PlayerEngineCore.ActiveEngine as IEqualizer;
if(equalizer != null) {
    // use equalizer
}

> * GStreamer - I had a quick look, but it seems that neither equalizer
> nor amplifier is supported in GStreamer 0.10.

Equalization can easily be supported in GStreamer. GStreamer is very
different from other media libraries though, so there is no
pre-existing, hard-coded API for equalization. It has to be done using
an equalizer element which can be inserted into the pipeline. We already
have one of these in the works, so I would not worry about the GStreamer
side of things.

> What do you think? Any comments, suggestion, reviews?

Again, having this as a plugin is really not the right way to go. This
should be a core feature.

That being said, here's my quick code review, just from glancing:

a) For consistency in the core, please don't use an inZ.Widgets
namespace. This widget should be in the Banshee.Gui namespace.

b) Your coding style is pretty inconsistent with the rest of the code
base. Because the code base is fairly large and growing, it is best to
have a good deal of consistency for easiest maintaining. The HACKING
file explains almost everything you need to know to adhere to our
standards. Please read it and make an effort to code in this style when
working on Banshee.

c) There are a few things I have issues with regarding your public API
design. One thing that stood out to me was that some of your property
set accessors do things like throwing exceptions. A property accessor
should never throw an exception and should always be quick operations.
If you need to perform an exhaustive operation or one that can throw
exceptions, rewrite the property logic as a method instead. 

All this being said, I'd like to work to get this in core if you're
willing to address some of the issues I've brought up. Let's keep this
discussion open :)

Cheers,
Aaron