[Bug 611689] [NEW PLUGIN] crossfeed plugin

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=611689
  GStreamer | gst-plugins-bad | git

--- Comment #11 from Stefan Sauer (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> 2014-12-12 08:46:38 UTC ---
(In reply to comment #10)
> (In reply to comment #8)
> > Review of attachment 284336 [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.

> > @@ +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!

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.