Created attachment 109740 [details] [review]
Patch adding visualization data source

Thanks for the patch, Chris! Some things:

Minor style note: it is Banshee style not to use the this keyword unless it disambiguate a member from a local variable.

Why does PlayerEngine.DataAvailable provide it's own add/remove blocks? Do you intend in the future to perform more work in those blocks than you're currently doing? If not, they are unnecessary.

Also, the private mDataAvailable field is out of style. It should be called data_available. However, if you're getting rid of the custom event registration, this field should not be necessary.

In OnVizData, you should cache the value of the event at the top of the method and use that local variable for the null check and the invokation. In it's current form, you are vulnerable to a race condition. Suppose the event is non-null at the time of the null check, but another thread unregisters the only delegate to the event before you invoke the event at the end of the method. It will result in a NPE.

You should be able to get better memory performance if you use a two-dimensional array rather than a jagged array. Something like this:

float[,] cbd = new float[channels,samples];

int offset = 0;

for (int i = 0; i < channels; i++) {
    for (int j = 0; j < samples; j++) {
        cbd[i,j] = flat[offset + j * channels];
    }
    offset++;
}

And update the VisualizationDataCallback delegate signature accordingly.

Aside from those minor issues, it's a good looking patch. I look forward to seeing your OpenVP stuff in action!

I have fixed everything except for changing float[][] to float[,].  Yes, the memory performance appears to be worse but remember that we are dealing with a relatively low amount of data to begin with, so performance is negligible in this case.

While float[,] better expresses the nature of the data, it is extremely inconvenient.  That is why OpenVP uses float[][] too.  Say you have some routine that renders a scope.  You want it to only render one channel, so you declare it with one parameter, a float[].  There is really no way to express "float[0,*]" in CIL such that you can take the left, right, or center channels out of the array individually and throw them around without copying them.  You lose a rather large amount of flexibility and ease of coding to prematurely optimize.

In fact, if you have enough extensions consuming the visualization data event, and they all need to take one or more channels as a float[] then you are actually going to *lose* performance as each extension is making a duplicate of the channels it needs.

I'll try to upload the updated patch tomorrow.  Thanks for the tips.

Created attachment 109845 [details] [review]
Patch adding visualization data source

Looks good. The only (really minor) thing I would say, which is not really your fault since this an issue with the unpatched code, is that the interfaces PlayerEngine implements do not need to have namespace qualifiers. The base type, however, does, since it is ambiguous between Banshee.MediaEngine.PlayerEngine and Banshee.GStreamer.PlayerEngine. So the class definition should read:

public class PlayerEngine : Banshee.MediaEngine.PlayerEngine, IEqualizer, IVisualizationDataSource

A great looking patch.

Created attachment 110072 [details] [review]
Patch adding visualization data source

New patch fixes the last minor issue you noted, and also adds spectrum data to the event.

Chris, looks good.  Can you clean up the commented out lines and add a ChangeLog entry?  Also, should/can  the PlayerEngine stuff be generalized so other backends could provide the same functionality (theoretically)?

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

Chris, would this conflict with http://bugzilla.gnome.org/show_bug.cgi?id=524718 at some stage down the road? This is attempting to integrate visualizer engine as far as I can see (which is cool), but I want to know how that's going to act with controlling playbin's visualization property and setting it directly that way?

(In reply to comment #10)
> Chris, would this conflict with
> http://bugzilla.gnome.org/show_bug.cgi?id=524718 at some stage down the road?
> This is attempting to integrate visualizer engine as far as I can see (which is
> cool), but I want to know how that's going to act with controlling playbin's
> visualization property and setting it directly that way?

There should not be any conflict.  My patch uses the tee created by Banshee to push the PCM and spectrum data to managed land for managed visualizations.  I don't touch the vis-plugin property.

Created attachment 115765 [details] [review]
Patch adding visualization data source

Added ChangeLog entries and removed commented code.  Also disables the unmanaged->managed callback when there are no managed event handlers registered.

As a side note, I would prefer to actually unhook the viz pipeline from the tee when there are no managed handlers set up but I did not want to get into modification of running pipelines.  So instead I just short-circuited as much of the process as possible when there are no managed handlers registered.  The difference in CPU utilization between no viz pipeline and a viz pipeline that goes nowhere seems negligible.

Minor style note: if statements should always have braces. Please add them to the three bare if statements. Also, this code:

        public event VisualizationDataCallback DataAvailable {
            add {
                dataAvailable += value;
                
                if (dataAvailable != null)
                    bp_set_viz_data_callback (handle, viz_data_callback);
            }
            remove {
                dataAvailable -= value;
                
                if (dataAvailable == null)
                    bp_set_viz_data_callback (handle, null);
            }
        }

Should probably look like this:

        public event VisualizationDataCallback DataAvailable {
            add {
                if (dataAvailable == null && value != null) {
                    bp_set_viz_data_callback (handle, viz_data_callback);
                }

                dataAvailable += value;
            }
            remove {
                dataAvailable -= value;
                
                if (dataAvailable == null && value != null) {
                    bp_set_viz_data_callback (handle, null);
                }
            }
        }

This will prevent unnecessary calls to bp_set_viz_data_callback.

Lastly, there are still a few lines of commented code in banshee-player-pipeline.c. Looking pretty good.

Created attachment 115899 [details] [review]
Patch adding visualization data source

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

I took the latest patch and reorganized it to fit better with the layout of libbanshee, fixed a few areas to better use GStreamer/GLib facilities, and fit with the code guidelines defined in HACKING.

My revised version is now committed.

Thanks a lot Chris!

...

2008-08-20  Aaron Bockover  <abock@gnome.org>

    Committed a revised/reorganized version of Chris Howie's <cdhowie@gmail.com>
    visualization support patch (BGO #529479); this does not actually implement
    visualization, but adds support for extensions to do so by harvesting
    spectrum data that is now readily available.

    * src/Core/Banshee.Services/Banshee.MediaEngine/IVisualizationDataSource.cs:
    * src/Core/Banshee.Services/Banshee.MediaEngine/VisualizationDataCallback.cs:
    Add interface for PlayerEngines that can provide data for visualizations.

    * src/Backends/Banshee.GStreamer/Banshee.GStreamer/PlayerEngine.cs:
    Implement IVisualizationDataSource, binding the unmanaged visualization
    support from libbanshee's BansheePlayer

    * libbanshee/banshee-player-vis.c
    * libbanshee/banshee-player-vis.h: Unmanaged support for gathering
    visualization data on the pipeline using the spectrum element

    * libbanshee/banshee-player-pipeline.c:
    * libbanshee/banshee-player-private.h: Integrate new vis code into the
    pipeline and player object