After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 611689 - [NEW PLUGIN] crossfeed plugin
[NEW PLUGIN] crossfeed plugin
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-03-03 11:26 UTC by Christoph Reiter (lazka)
Modified: 2015-06-11 19:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for gst1.0 (18.38 KB, patch)
2014-08-24 12:06 UTC, Christoph Reiter (lazka)
reviewed Details | Review
patch for gst1.0 v2 (18.05 KB, patch)
2015-01-10 20:55 UTC, Christoph Reiter (lazka)
reviewed Details | Review
patch for gst1.0 v3 (17.92 KB, patch)
2015-01-10 23:01 UTC, Christoph Reiter (lazka)
committed Details | Review
implement the preset iface (8.02 KB, patch)
2015-01-12 20:29 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
implement the preset iface (8.03 KB, patch)
2015-01-14 10:26 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Christoph Reiter (lazka) 2010-03-03 11:26:55 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/)
Comment 1 Tim-Philipp Müller 2010-03-03 11:44:58 UTC
Cool! New plugins get added to -bad, so moving there.
Comment 2 Sebastian Dröge (slomo) 2013-08-23 10:45:00 UTC
Can you prepare a patch against gst-plugins-bad and attach it here? Also needs to be ported to 1.0.
Comment 3 Christoph Reiter (lazka) 2013-08-23 10:49:58 UTC
Will do, if I find some time.

btw, the code lives here now: https://github.com/lazka/gst-bs2b/blob/master/src/gstbs2b.c
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2014-01-09 16:48:26 UTC
Nice plugin, would you have some time to get this into -bad?
Comment 5 Christoph Reiter (lazka) 2014-08-24 12:06:08 UTC
Created attachment 284336 [details] [review]
patch for gst1.0

Time flies..

Any feedback welcome.
Comment 6 Tim-Philipp Müller 2014-08-24 12:24:34 UTC
Thanks.
Comment 7 Christoph Reiter (lazka) 2014-12-10 14:41:20 UTC
Anything I can do to get this in?
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-10 20:04:23 UTC
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.
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-11 13:13:12 UTC
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) :/
Comment 10 Christoph Reiter (lazka) 2014-12-11 15:05:00 UTC
(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!
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-12 08:46:38 UTC
(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!
Comment 12 Christoph Reiter (lazka) 2014-12-12 09:23:03 UTC
(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!
Comment 13 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-12 11:48:47 UTC
(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.
Comment 14 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-12 20:56:11 UTC
Christoph, FYI, I added a gst_preset_is_editable() to make it obvious for apps to check the preset impl.
Comment 15 Stefan Sauer (gstreamer, gtkdoc dev) 2014-12-21 13:31:52 UTC
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.
Comment 16 Christoph Reiter (lazka) 2015-01-10 20:55:35 UTC
Created attachment 294246 [details] [review]
patch for gst1.0 v2
Comment 17 Christoph Reiter (lazka) 2015-01-10 21:03:26 UTC
(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
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-10 21:59:53 UTC
(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.
Comment 19 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-10 22:18:12 UTC
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.
Comment 20 Christoph Reiter (lazka) 2015-01-10 23:01:29 UTC
Created attachment 294253 [details] [review]
patch for gst1.0 v3
Comment 21 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-12 20:29:55 UTC
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);
Comment 22 Stefan Sauer (gstreamer, gtkdoc dev) 2015-01-14 10:26:04 UTC
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.
Comment 23 Christoph Reiter (lazka) 2015-01-14 23:17:25 UTC
Sure. Thanks.