GNOME Bugzilla – Bug 611689
[NEW PLUGIN] crossfeed plugin
Last modified: 2015-06-11 19:06:08 UTC
"<Uraeus> lazka: nice, you should file a bug report pointing to it so we can get it moved into the core plugins modules" Code and information: http://bitbucket.org/lazka/gst-bs2b/ Main source: http://bitbucket.org/lazka/gst-bs2b/src/tip/src/gstbs2b.c Short summary: Crossfeed plugin using the bs2b library (http://bs2b.sourceforge.net/)
Cool! New plugins get added to -bad, so moving there.
Can you prepare a patch against gst-plugins-bad and attach it here? Also needs to be ported to 1.0.
Will do, if I find some time. btw, the code lives here now: https://github.com/lazka/gst-bs2b/blob/master/src/gstbs2b.c
Nice plugin, would you have some time to get this into -bad?
Created attachment 284336 [details] [review] patch for gst1.0 Time flies.. Any feedback welcome.
Thanks.
Anything I can do to get this in?
Review of attachment 284336 [details] [review]: ::: ext/bs2b/gstbs2b.c @@ +22,3 @@ + * SECTION:element-bs2b + * + * Improve headphone listening of stereo audio records using the bs2b library. Could you expand this a little and tell how it improves the audio? @@ +68,3 @@ +{ + PROP_0, + PROP_FCUT, PROP_FCUT = 1, and remove the PROP_0 @@ +267,3 @@ + case GST_EVENT_SEGMENT: + GST_BS2B_DP_LOCK (element); + bs2b_clear (element->bs2bdp); what is this doing? Maybe do this when the input-buffer has the DISCONT flag set? @@ +343,3 @@ + bs2b_clear (element->bs2bdp); + GST_BS2B_DP_UNLOCK (element); + g_object_notify_by_pspec (object, properties[PROP_PRESET]); you don't need to do this. @@ +356,3 @@ + case PRESET_DEFAULT: + GST_BS2B_DP_LOCK (element); + bs2b_set_level (element->bs2bdp, BS2B_DEFAULT_CLEVEL); Could you use the values in the enum to avoid the switch case here? Like drop PRESET_{DEFAULT, CMOY, ...} and use BS2B_{DEFAULT,CMOY,..}_CLEVEL in the enum above.
It seems that the presets are simply (fcut, feedback) pairs. http://sourceforge.net/p/bs2b/code/HEAD/tree/trunk/libbs2b/src/bs2b.h#l92 Would be nice to use the GstPresetInterface for these (although we still don't have a property to set a preset then and hence cannot use gst-launch to activate a preset) :/
(In reply to comment #8) > Review of attachment 284336 [details] [review]: > > ::: ext/bs2b/gstbs2b.c > @@ +22,3 @@ > + * SECTION:element-bs2b > + * > + * Improve headphone listening of stereo audio records using the bs2b library. > > Could you expand this a little and tell how it improves the audio? Good idea, thanks. > @@ +68,3 @@ > +{ > + PROP_0, > + PROP_FCUT, > > PROP_FCUT = 1, > and remove the PROP_0 OK. > @@ +267,3 @@ > + case GST_EVENT_SEGMENT: > + GST_BS2B_DP_LOCK (element); > + bs2b_clear (element->bs2bdp); > > what is this doing? Maybe do this when the input-buffer has the DISCONT flag > set? I wanted to reset the filter on seek events. DISCONT looks like what I wanted, thanks. > @@ +343,3 @@ > + bs2b_clear (element->bs2bdp); > + GST_BS2B_DP_UNLOCK (element); > + g_object_notify_by_pspec (object, properties[PROP_PRESET]); > > you don't need to do this. bs2b_set_level_fcut() will change the internal value of preset, changing the result of get_property(PRESET), so shouldn't this notify that change as well? The same happens for the other way around where setting the preset will change the filter properties (FCUT, FEED). > @@ +356,3 @@ > + case PRESET_DEFAULT: > + GST_BS2B_DP_LOCK (element); > + bs2b_set_level (element->bs2bdp, BS2B_DEFAULT_CLEVEL); > > Could you use the values in the enum to avoid the switch case here? Like drop > PRESET_{DEFAULT, CMOY, ...} and use BS2B_{DEFAULT,CMOY,..}_CLEVEL in the enum > above. BS2B_{DEFAULT,CMOY,..}_CLEVEL is a int containing the configuration state (e.g. default=2949820). While its format is defined in a header comment I'd prefer if this value wouldn't be exposed in the element interface. Also using gst-launch syntax: "bs2b preset=0" seems nicer than "bs2b preset=2949820". Thanks for the review!
(In reply to comment #10) > (In reply to comment #8) > > Review of attachment 284336 [details] [review] [details]: > > @@ +343,3 @@ > > + bs2b_clear (element->bs2bdp); > > + GST_BS2B_DP_UNLOCK (element); > > + g_object_notify_by_pspec (object, properties[PROP_PRESET]); > > > > you don't need to do this. > > bs2b_set_level_fcut() will change the internal value of preset, changing the > result of get_property(PRESET), so shouldn't this notify that change as well? > > The same happens for the other way around where setting the preset will change > the filter properties (FCUT, FEED). > You are right, I misread the code. > > @@ +356,3 @@ > > + case PRESET_DEFAULT: > > + GST_BS2B_DP_LOCK (element); > > + bs2b_set_level (element->bs2bdp, BS2B_DEFAULT_CLEVEL); > > > > Could you use the values in the enum to avoid the switch case here? Like drop > > PRESET_{DEFAULT, CMOY, ...} and use BS2B_{DEFAULT,CMOY,..}_CLEVEL in the enum > > above. > > BS2B_{DEFAULT,CMOY,..}_CLEVEL is a int containing the configuration state (e.g. > default=2949820). While its format is defined in a header comment I'd prefer if > this value wouldn't be exposed in the element interface. Also using gst-launch > syntax: "bs2b preset=0" seems nicer than "bs2b preset=2949820". You can use the enum names on gst-launch. But what do you think about actually implementing the preset interface. See https://bugzilla.gnome.org/show_bug.cgi?id=741427 for making presets usable from the command-line. > > Thanks for the review!
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #8) > > > Review of attachment 284336 [details] [review] [details] [details]: > > > @@ +343,3 @@ > > > + bs2b_clear (element->bs2bdp); > > > + GST_BS2B_DP_UNLOCK (element); > > > + g_object_notify_by_pspec (object, properties[PROP_PRESET]); > > > > > > you don't need to do this. > > > > bs2b_set_level_fcut() will change the internal value of preset, changing the > > result of get_property(PRESET), so shouldn't this notify that change as well? > > > > The same happens for the other way around where setting the preset will change > > the filter properties (FCUT, FEED). > > > You are right, I misread the code. OK. > > > @@ +356,3 @@ > > > + case PRESET_DEFAULT: > > > + GST_BS2B_DP_LOCK (element); > > > + bs2b_set_level (element->bs2bdp, BS2B_DEFAULT_CLEVEL); > > > > > > Could you use the values in the enum to avoid the switch case here? Like drop > > > PRESET_{DEFAULT, CMOY, ...} and use BS2B_{DEFAULT,CMOY,..}_CLEVEL in the enum > > > above. > > > > BS2B_{DEFAULT,CMOY,..}_CLEVEL is a int containing the configuration state (e.g. > > default=2949820). While its format is defined in a header comment I'd prefer if > > this value wouldn't be exposed in the element interface. Also using gst-launch > > syntax: "bs2b preset=0" seems nicer than "bs2b preset=2949820". > > You can use the enum names on gst-launch. But what do you think about actually > implementing the preset interface. See > https://bugzilla.gnome.org/show_bug.cgi?id=741427 for making presets usable > from the command-line. * Would that mean I'd lose the default presets? Or should I override the default implementation of get_preset_names(), load_preset() and get_meta() to provide them in any case (I can't seem to find a plugin in git which implements the interface besides using the default impl)? Should they be deletable? * Besides gst-launch support, would gst-inspect show the existing presets and meta data (comments/descriptions) as is currently done for enums? * Just a minor issue.. Afaics I would lose the property notification for the preset if I change the other properties. I currently have the following UI: https://i.imgur.com/skrL8cU.png where the combo box changes depending on the sliders and vice versa. Maybe use the default GstPreset impl (+ adjusted get_property_names ()) in addition to the current preset property? > > > > Thanks for the review!
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #8) > > > > Review of attachment 284336 [details] [review] [details] [details] [details]: > > > > @@ +356,3 @@ > > > > + case PRESET_DEFAULT: > > > > + GST_BS2B_DP_LOCK (element); > > > > + bs2b_set_level (element->bs2bdp, BS2B_DEFAULT_CLEVEL); > > > > > > > > Could you use the values in the enum to avoid the switch case here? Like drop > > > > PRESET_{DEFAULT, CMOY, ...} and use BS2B_{DEFAULT,CMOY,..}_CLEVEL in the enum > > > > above. > > > > > > BS2B_{DEFAULT,CMOY,..}_CLEVEL is a int containing the configuration state (e.g. > > > default=2949820). While its format is defined in a header comment I'd prefer if > > > this value wouldn't be exposed in the element interface. Also using gst-launch > > > syntax: "bs2b preset=0" seems nicer than "bs2b preset=2949820". > > > > You can use the enum names on gst-launch. But what do you think about actually > > implementing the preset interface. See > > https://bugzilla.gnome.org/show_bug.cgi?id=741427 for making presets usable > > from the command-line. > > * Would that mean I'd lose the default presets? Or should I override the > default implementation of get_preset_names(), load_preset() and get_meta() to > provide them in any case (I can't seem to find a plugin in git which implements > the interface besides using the default impl)? Should they be deletable? The preset iface is a little special in the sense that if you don't override any methods you inherit a gkeyfile based impl. You would override the load_preset and get_preset_names methods and map it to the library. _save, _rename, _delete would be overridden to NULL. Maybe lets do this as round two. Seems that it is not easy to discover for an application that in this case the presets are static (not editable). I'll think of something. > > * Besides gst-launch support, would gst-inspect show the existing presets and > meta data (comments/descriptions) as is currently done for enums? See the bug I just filed: https://bugzilla.gnome.org/show_bug.cgi?id=741427 > > * Just a minor issue.. Afaics I would lose the property notification for the > preset if I change the other properties. I currently have the following UI: > https://i.imgur.com/skrL8cU.png where the combo box changes depending on the > sliders and vice versa. Nope, when you load a preset, the implementation would fire notifies for the properties it sets. > > Maybe use the default GstPreset impl (+ adjusted get_property_names ()) in > addition to the current preset property? > I'd say make the other changes. Consider killing the enum (try gst-launch with the preset names). And we do a round two for the preset iface.
Christoph, FYI, I added a gst_preset_is_editable() to make it obvious for apps to check the preset impl.
Christoph, are you using the gst equalizer in quod libet? That comes with a few presets already. I've just submitted a few presets for freeverb (in -bad) as well. Using the preset iface could simplify your UI code.
Created attachment 294246 [details] [review] patch for gst1.0 v2
(In reply to comment #14) > Christoph, FYI, I added a gst_preset_is_editable() to make it obvious for apps > to check the preset impl. Ok, I can try implementing the interface. I don't really need it tbh, but why not. (In reply to comment #15) > Christoph, are you using the gst equalizer in quod libet? That comes with a few > presets already. I've just submitted a few presets for freeverb (in -bad) as > well. Using the preset iface could simplify your UI code. Our UI code [0] is quite simple already, but we also don't support saving more than one config. And I haven't had any feature requests until now to extend that. Also the UI code works against our backend interface for which we have two implementations (GStreamer and xine). Do you know of any application which uses this interface and exposes an UI (buzztrax maybe?)? [0] https://code.google.com/p/quodlibet/source/browse/quodlibet/quodlibet/ext/events/equalizer.py
(In reply to comment #17) > (In reply to comment #14) > > Christoph, FYI, I added a gst_preset_is_editable() to make it obvious for apps > > to check the preset impl. > > Ok, I can try implementing the interface. I don't really need it tbh, but why > not. > > (In reply to comment #15) > > Christoph, are you using the gst equalizer in quod libet? That comes with a few > > presets already. I've just submitted a few presets for freeverb (in -bad) as > > well. Using the preset iface could simplify your UI code. > > Our UI code [0] is quite simple already, but we also don't support saving more > than one config. And I haven't had any feature requests until now > to extend that. Also the UI code works against our backend interface for which > we have two implementations (GStreamer and xine). Do you know of any > application which uses this interface and exposes an UI (buzztrax maybe?)? > > [0] > https://code.google.com/p/quodlibet/source/browse/quodlibet/quodlibet/ext/events/equalizer.py Here is a screenshot of the buzztrax ui for the presets http://wiki.buzztrax.org/index.php/File:Bt-edit-0.4.0-01.png users can modify the presets, yours seems to be built-in. But given that you apply them to two different backends, I can understand your design. If you feel that adding the preset iface is too much for you, but you won't not mind using it in this case, I can make the change.
Review of attachment 294246 [details] [review]: Looks good now. ::: ext/bs2b/gstbs2b.c @@ +157,3 @@ + GST_BS2B_DP_LOCK (element); + element->bs2bdp = bs2b_open (); + GST_BS2B_DP_UNLOCK (element); You should not need the lock/unlock here. @@ +255,3 @@ + bs2b_close (element->bs2bdp); + element->bs2bdp = NULL; + GST_BS2B_DP_UNLOCK (element); You should not need the lock in finalize.
Created attachment 294253 [details] [review] patch for gst1.0 v3
Created attachment 294375 [details] [review] implement the preset iface Christoph, this will affect your UI. You will use gst_preset_get_preset_names(element) to get a list of presets. And then gst_preset_get_meta(element, preset_name, "comment", &comment) to get the description for each preset. To activate one you do: gst_preset_load_preset(element, preset_name);
Created attachment 294501 [details] [review] implement the preset iface Christoph, let me know if that api works for you. Otherwise I'll submit all of that in a few days.
Sure. Thanks.