GNOME Bugzilla – Bug 641748
Equalizer crash
Last modified: 2011-05-07 17:41:54 UTC
View->Equalizer causes a crash: System.InvalidCastException occurred Message="Unable to cast object of type 'Banshee.GStreamerSharp.PlayerEngine' to type 'Banshee.MediaEngine.IEqualizer'." Source="Banshee.ThickClient" StackTrace: at Banshee.Equalizer.Gui.EqualizerView.BuildWidget() in C:\Projects\_private\banshee\src\Core\Banshee.ThickClient\Banshee.Equalizer.Gui\EqualizerView.cs:line 66
Confirm (latest git)
The problem here obviously is that GStreamerSharp's PlayerEngine does not implement IEqualizer. Does anybody have a clue where to look for AmplifierLevel, BandRange and EqualizerFrequencies in GStream#?
*** Bug 643223 has been marked as a duplicate of this bug. ***
I've been working on this and have gotten the EqualizerView to show. Now I'm trying to actually get the equalizer to work with all the settings. However, it seems that gstreamer-sharp's Element class has a protected AddPad method which complicates things (it should be public). I'll see about submitting a patch to gstreamer-sharp to make it public after I can verify it's working.
Created attachment 184808 [details] [review] Patch to fix equalizer crash on windows Here is a patch to fix the equalizer crash on Windows. This will allow the equalizer window to show without crashing banshee. NOTE: At this time the equalizer doesn't do anything on Windows because the GStreamerSharp backend is using a simple playbin2. Until the GStreamerSharp backend is rewritten, this equalizer window will have no effect.
Review of attachment 184808 [details] [review]: Looks pretty good, please read HACKING for some style points though ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +52,3 @@ + public enum EqualizerStatus + { + UNCHECKED, See HACKING for style on enums; in short, use CamelCase @@ +159,3 @@ + Log.Debug("Buggy system equalizer found. gst-plugins-good 0.10.9 or better required, or build Banshee with the built-in equalizer."); + } + else see HACKING; in short, should be `} else {` @@ +161,3 @@ + else + { + Log.Debug("No system equalizer found"); see HACKING; in short, put space between method name and (arg list)
Created attachment 184816 [details] [review] Patch to fix equalizer crash on windows with fixed style guidelines per HACKING Fixed style guidelines per HACKING.
Review of attachment 184816 [details] [review]: Please format the commit msg like others you see in git log. Something like this: GStreamerSharp: Fix crash on View > Equalizer (bgo#641748) ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +66,3 @@ + uint iterate_timeout_id = 0; + + public PlayerEngine() Most of the changed lines in this patch aren't really changed, except adding Windows-style line endings. Please run dos2unix on the file; we commit Unix line endings to our git repo. When you installed Git on Windows, did you chose to "Checkout Windows-style, commit Unix-style line endings"?
Oh, my bad. I use git through cygwin. I'll run it through dos2unix and resubmit with a better commit message.
Created attachment 184829 [details] [review] Fixes crash on View Equalizer in GStreamerSharp on Windows Let's try it again :P I ran it through dos2unix so it should have unix line endings and be styled correctly.
Review of attachment 184829 [details] [review]: Still lots of whitespace wrong ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +66,3 @@ uint iterate_timeout_id = 0; + public PlayerEngine() space between PlayerEngine and ( @@ +74,3 @@ + if (PlatformDetection.IsWindows) + { + var gst_paths = new string[] { Hyena.Paths.Combine(Hyena.Paths.InstalledApplicationPrefix, "gst-plugins") }; space between string and [] @@ +455,3 @@ + + if (pspec.Blurb != null) + { put { up on same line as if; same for for, while, foreach, etc
Ah sorry, I see what's happening, my VS is screwing up the formatting when I save. I'll go through and fix it and double check before submitting again. Sorry about that.
Created attachment 184834 [details] [review] Fixes crash on View Equalizer in GStreamerSharp on Windows Alright, hopefully I got everything this time.
Review of attachment 184834 [details] [review]: Getting closer! Add a copyright line and an author line for yourself, btw. ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +58,3 @@ + } + + public class PlayerEngine : Banshee.MediaEngine.PlayerEngine, IEqualizer, IVisualizationDataSource, ISupportClutter Remove ISupportClutter iface and implementation until we actually do. Unless there's a reason to have this I'm missing? @@ +102,3 @@ + if (equalizer != null) { + eq_audioconvert = ElementFactory.Make ("audioconvert", "audioconvert"); + eq_audioconvert2 = ElementFactory.Make ("audioconvert", "audioconvert2"); Does this whole bit of code do anything? are eq_audioconvertN actually used? @@ +125,3 @@ + if (this.equalizer_status == EqualizerStatus.Unchecked) { + this.equalizer_status = EqualizerStatus.UseBuiltIn; + Log.Debug ("Using built-in equalizer element"); Can you check what version the builtin equalizer is needed for? I wouldn't be surprised if it's < the version needed to even use gst# and therefore you could ignore the whole builtin eq bit @@ +415,3 @@ + public void SetEqualizerGain (uint band, double value) + { Does this not need to be implemented? Should we at least have a // FIXME or log msg or something? @@ +420,2 @@ public override bool SupportsEqualizer { get { return false; } Hrm, why is this false? Because the support isn't complete/ready yet? @@ +424,3 @@ + public double AmplifierLevel { + set { + double scale = Math.Pow (10.0, value / 20.0); no need for this local variable, just set preamp[volume] directly to this @@ +459,3 @@ + public uint [] EqualizerFrequencies { + get { + uint count = this.GetNBands (); remove unnecessary 'this.' here and everywhere -- see HACKING @@ +463,3 @@ + + if (equalizer != null) { + remove blank line @@ +465,3 @@ + + for (uint i = 0; i < count; i++) { + Gst.Object band = ChildProxyAdapter.GetObject (equalizer).GetChildByIndex (i); not sure how expensive ChildProxyAdapter.GetObject is - could do it outside this loop @@ +470,3 @@ + } + + uint [] ret = new uint [count]; why have both the double[] and uint[]? why not just use the uint[] array only?
(In reply to comment #15) > Review of attachment 184834 [details] [review]: > > @@ +125,3 @@ > + if (this.equalizer_status == EqualizerStatus.Unchecked) { > + this.equalizer_status = EqualizerStatus.UseBuiltIn; > + Log.Debug ("Using built-in equalizer element"); > > Can you check what version the builtin equalizer is needed for? I wouldn't be > surprised if it's < the version needed to even use gst# and therefore you could > ignore the whole builtin eq bit > I took that equalizer checking from lines 46 - 96 banshee-player-equalizer.c in libbanshee. When, I debug banshee through visual studio, it always falls back to the built-in eq (probably because the banshee-equalizer isnt installed on my system). Do you think we should still remove the builtin eq bit? > @@ +420,2 @@ > public override bool SupportsEqualizer { > get { return false; } > > Hrm, why is this false? Because the support isn't complete/ready yet? > This is false because the support isn't complete or ready yet (that's next :P)
Created attachment 184915 [details] [review] Fixes crash on View Equalizer in GStreamerSharp on Windows Made some improvements as suggested.
Review of attachment 184915 [details] [review]: "it always falls back to the built-in eq (probably because the banshee-equalizer isnt installed on my system)" I think you have it backwards; I'm pretty sure builtin (in libbanshee and your patch) is referring to the Banshee-bundled element (aka "banshee-equalizer") -- which yes, isn't built or used on Windows. Running `git log gstreamer/equalizer` it looks like it was added to work around a bug in the eq in < 0.10.9. I have 0.10.27 on my system, and I'm about 100% sure that Gst# requires a higher version than 0.10.9 -- so let's not use/mention the builtin eq in this patch/gst# backend. ::: src/Backends/Banshee.GStreamerSharp/Banshee.GStreamerSharp/PlayerEngine.cs @@ +103,3 @@ + preamp = ElementFactory.Make ("volume", "preamp"); + } + two newlines here -- remove one @@ +113,3 @@ + private Element MakeEqualizer () + { + Element the_equalizer; don't declare locals at the top of a method - declare when needed @@ +114,3 @@ + { + Element the_equalizer; + if (equalizer_status == EqualizerStatus.Disabled) { How would this ever be the case? It's called from the ctor, and equalizer_status will default to Unchecked @@ +118,3 @@ + } + + if (equalizer_status == EqualizerStatus.Unchecked || equalizer_status == EqualizerStatus.UseBuiltIn) { remove this whole if {} @@ +430,3 @@ + set { + if (equalizer != null && preamp != null) { + preamp ["volume"] = Math.Pow(10.0, value / 20.0); space after method name @@ +453,3 @@ + } + + two newlines @@ +460,3 @@ + private uint GetNBands () + { + if (equalizer == null) put {} around if @@ +469,3 @@ + uint count = GetNBands (); + uint[] ret = new uint[count]; + ChildProxy equalizer_child_proxy = ChildProxyAdapter.GetObject (equalizer); move this to inside the if null check @@ +473,3 @@ + if (equalizer != null) { + for (uint i = 0; i < count; i++) { + Gst.Object band = equalizer_child_proxy.GetChildByIndex(i); space after method name @@ +474,3 @@ + for (uint i = 0; i < count; i++) { + Gst.Object band = equalizer_child_proxy.GetChildByIndex(i); + ret [i] = (uint)((double)band ["freq"]); remove unneeded ()
Created attachment 185133 [details] [review] Fixes crash on View Equalizer in GStreamerSharp on Windows Fixed the issues mentioned above. You were right and the built-in eq is the banshee-equalizer from libbanshee, so I removed all that code.
hmmm, just read the code and not test it but where are you link the pream and equalizer and set playbin.AudioSink to it ? It fix the bug but it do not add equalizer support currently...
Right, the bug is fixed, but to keep the patch size down, I decided against adding in all the extra gst# stuff needed to actually get the equalizer working. I figured that's big enough to warrant its own ticket.
Has anyonoe had a chance to review the latest patch I submitted?
Scott, please file a new bug about adding eq support to the gst# backend. This bug was about it crashing, which I've fixed in master by making View -> Equalizer insensitive if the player engine doesn't support eq. Please make sure your formatting is per HACKING, that you don't have Windows line endings (latest patch does), and remove the enum - just use ElementFactory.Make to try to make the 10 band eq, and if that returns null then disable it.
Ah, sorry about all that. I'll triple check my patches next time. I'll file a new bug about adding eq support to the gst# backend.