By "presets" you mean predefined settings for a number of GObject properties of an element, right? If that is so, why do we need an interface for this at all - wouldn't a bunch of utility functions that take a GstElement or GstObject do just as well?

I see some interest with this with, for example, encoders. You could have some presets, like ffenc_mpeg4 can be fine-tuned for animation, black&white movies, etc....

The important part is that it's only the plugin who knows what those settings are, so you need a way for the plugin for *export* that information. Whether an interface is the best way... I don't care :)

I see some interest with this with, for example, encoders. You could have some presets, like ffenc_mpeg4 can be fine-tuned for animation, black&white movies, etc....

The important part is that it's only the plugin who knows what those settings are, so you need a way for the plugin for *export* that information. Whether an interface is the best way... I don't care :)

@tim, imho it needs to be an iface in order that wrapped plugins can override the implementation. E.g. the ladspa plugin might bring the presets as part of the rdf, buzzmachines have an own plugin format too.
Thats also why I want to provide a default implementation, so that gst elemnts just add the iface, but don't override anything.

What about using GValue's/GParams or GST value types instead of gchar* value? - That would help UI for example, to display the correct field editor...

g_value_transform should be able to cast to string if necessary.

it would be nice to get some feedback on the preset format that the default
implementation will use:
http://www.buzztard.org/index.php/Preset_handling_interface#formats

I also would like to receive comments about the interface (anything missing
here?)
http://www.buzztard.org/index.php/Preset_handling_interface#iface_design
and the merging (how to handle systemwide and user presets)
    http://www.buzztard.org/index.php/Preset_handling_interface#system_wide_.26_use_local

the code in buzztard cvs has been updated.

@marc-andre: where do you see a gchar* value? The in memory datastructure will definitely keep GValues, but on disk I prefer to serialize to an ascii format.

Definetely, serializing values should be in text format (if it's a .ini or a .xml). But what if the implementation decide to store it differently (using trackerdb ;)

Seriously, I can read gchar *value in http://buzztard.cvs.sourceforge.net/buzztard/gst-buzztard/src/preset/preset.h?revision=1.7&view=markup

What is that about? Why not have a GValue * or something better?

Alternatively, we could also think of GstPreset object, taking Gobject elements
and serializing properties in an internal database object,

gst_preset_save_properties (preset, tag, element)
gst_preset_load_properties (preset, tag, element)

Then each preset implementation might have a different backend:
GstPresetXML
GstPresetINI...

This sounds great.  Very useful for a lot of applications.

I kind of prefer an XML format.  It's not such a big dependency any more.  JSON or YAML could be options too but they probably add a dependency that is less attractive.

Note, I think the XML format on your wiki should include a reference to the element (factory) for the preset.

@brendan: I updated the wiki page. I am still not sure about the xml. I want it to be very quick to load the presets and for e.g. embeded devices ts good to have a xml-less gstreamer. That means this iface needs to be optional.

From gst-editor perspective, do you see any missing method?

Your example looks great now.  What about keeping track of the plugin/element version?  Updates of plugins can change the properties or enums, right?  If we have a version in there, you can avoid any nasty errors trying to load an incompatible preset.

The only other thing I might add would be some kind of meta tags so that an application could optionally implement things like groups or keyword tagging.  

As far as format, I guess I can understand if XML is too much overhead.  Can we do something in YAML, JSON or Sexp that would be up to the application to parse?  That way, an embedded app could have a custom parser and a bigger system could use a library method.  I hate using ugly binary or unreadable text formats if I can avoid it.

I also have a question about managing presets.  How should package maintainers handle merging presets?  They might need some kind of merge utility, unless the GStreamer devs want to manage some sort of preset repository.

I have to agree with tim here.  GObject already has a pretty decent properties system that already has much of what has been discussed here.  One thing it doesn't have is differentiation between configuration properties (i.e., audiotestsrc freq=440) and state properties (audiotestsrc name="blah").  But this and everything else I've read here could be put into a subclass of GParamSpec.

@David, how can a sub clas of GParamSpec help here (you certainly don't want to subclass all of the GParamSpec variants). Secondly as I already pointed out in the reply to tims comment, the interface is nessecary for wrapper plugins to be able to override the default implementation.

@Brendan,

the versions are in the file examples. I will add code tonight that uses the plugin version if it can. Meta data is there too. One example can be a comment, another example could be the use of keywords (e.g. Codecs: LQFast, HQSlow; Sounds: Pad, Effect, Bass).

The format won't we a binary one as this would not be platform independent.

I agree that we need to think about how to receive preset contrbutions from users and how to merge them.

Just want to bring to your attention the Rhythmbox equalizer plugin I'm writing: #76522. It may be an interesting use case here.

Presets handling is completely independent of gstreamer. It has the following features:
* presets are stored in a GKeyFile file, e.g.
   [Preset name]
   name[fr]=french localized name
   frequencies=100;200;500;1000;2000;5000;10000;
   gains=0.3;-5;-10;-2;0.0;0.0;2
   ...
* presets name are localized
* there is no direct mapping between the band setting as described in the file and the actual setting of the gstreamer element in the backend, because someone could have created a preset with a 10-bands equalizer and then wanted to use it with a 3 or 15-bands one. It does make sense for presets shipped with the element: they are designed with a particular configuration and the user may want to use them in another. So what I do is I interpolate gains value so that they fit the target number of bands. I think this sponsors the 'iface' design, so that a gstreamer equalizer element could implement this kind of interpolation if desired.

Something that concerns me too is the ability to 'port' presets from one element to another when it makes sense, for example between the various equalizer element we have: ladpa-Eq, ladspa-tap-equalizer and equalizer.

(Bug #76522)

Christophe; I also thought about converting presets from one element to another. Unfortunately I don't see a way to support this in a generic way and also not part of the preset support. I have a similar problem in buzztard. There I have pattern with music events. If one receives a song, wants to load it but misses an effect, the user might want to use something similar and this nees mapping the parameters. Also the musican might not be happy with a sounds and replace one effect with a similar one. For this me need a mechanism to install a->b transformers for specific elements.

One thing I could do in the equalizer is to remap the gain-values when the bands change. Right now bands is construct-only and I want to change that anyway. So you can load any preset and just reset the number of bands to 3 or 10 afterwards.

@gst core developers: where should this go to?
gstreamer/gst
gst-plugins-base/gst-libs/gst/interfaces

In the absence of comments,  will prepare a patch against core. E.g. queue could have preset support {minimal, audio (small), video (large)}.

Created attachment 97925 [details] [review]
add preset interface

Created attachment 98144 [details] [review]
add preset interface

* do backup when saving presets
* add header to gst.h

2007-11-17  Stefan Kost  <ensonic@users.sf.net>

	* docs/gst/gstreamer-docs.sgml:
	* docs/gst/gstreamer-sections.txt:
	* docs/gst/gstreamer.types.in:
	* gst/Makefile.am:
	* gst/gst.h:
	* gst/gstpreset.c:
	* gst/gstpreset.h:
	  Add the preset interface (Fixes #396779). Do some doc cleanups along.

This should not have gone in CVS in the current state, Stefan.

You are creating a new API (interface) for setting object properties? This should not be done. What you want is to make sure that all things you want to configure on the GObjects is done with the properties. If this is not possible, GParamSpec should be expanded.

What you then want is a way to configure those properties based on an external configuration file (which can come in many formats, hence specialized readers). In no way I can see how this would need any addition to existing elements.

It seems the feature is badly understood. I'll try to explain further. There are elements with lots of settings (gobject properties). Its useful to be able to have  named parameter settings (multiple parameters, one name). Can we agree on this?

Now applications need a defined interface to scan available presets, activate one of those, or store the current as a new preset. It would definitely be possible to handle this inside the app too. The downsides would be that the code would be duplicated in various applications and elements can not override the implementation. All the wrapper-plugins (ladspa, buzz, libvisual, ...) would want to support the native preset format (if there is one).

The idea of the current implementation is to provide something that can be used by  most elements as it is. The more special case an element is the more likely it will need to override things.

The implementation was available for almost a year and I annouced it on the mailing list and irc. Noone objected. There are some todos which need discussion, but a new core lifecycle just started, so lets discuss and I will make the changes. I've sent another mail to gst-devel regarding that.

> It seems the feature is badly understood. I'll try to explain further.

This may be so, but it should still have been reviewed before your commit in any case.  It is your responsibility to poke people to review it and understand it and see the usefulness of it before you commit it.


> The implementation was available for almost a year and I announced it on the
> mailing list and irc. Noone objected.

"No one objected" does not mean "ok to commit", as I (and others) have pointed out to you on IRC in the past (in relation to this bug, even!).

And while you say 'has been available over a year', an actual patch against core has been available for less than a month (and incidentally most people were busy releasing core/base, which is a fair bit of work, so your patch received even less attention than it would have received otherwise).

Again, it is your responsibility to poke people to review things.  I know that it can be frustrating and discouraging from own experience, but that's not an excuse to just commit stuff IMHO.


> There are some todos which need discussion, but a new core lifecycle
> just started, so lets discuss and I will make the changes. 

Does this have a name yet? If not, I shall call it the "guerilla-I-committed-it-deal-with-it-now-or-suck-it-up tactic" ;)


More on topic: while I don't have time to look into the entire thing right now, just looking at the API I think I'd prefer gst_preset_get_preset_names() to return a NULL-terminated gchar ** - the ownership and type and free-ing of this is much easier than with a GList (e.g. what's in the GList? Strings? Does the GList contain strdup'ed strings or not?) (I know other interfaces return a GList, but I don't think that's a good reason to keep doing it - and bindings have to special-case this anyway).

Tim, it was available online. Its a new interface. Everyone can comment on this without it beeing a patch. In above comments you see also, that pepople agree that it is useful.

Regadring the review. I did that and I am getting tired that everyone is busy with the videoplayback use-case. Other people commit changes to core/base directly too (playbin2). I would love to do it differently and ultimately I will also backout the patch if we can't get it ready until next relese.

Regarding gst_preset_get_preset_names. The docs say:
"Get a copy of the preset list names. Free list when done."
Preset-names are strings. I agree it could be racy if one removes a preset elsewhere. I'll rework that.

Created attachment 99354 [details]
two presets for 10band eq

Created attachment 99355 [details]
two presets for 3 band eq

Sounds great. I still miss a way for cross element preset sharing, but I guess it's not really feasible in any easy way.
Some remarks:
- I like the simple format, why an xml extension for non xml content ?
- Won't the ':' in the 'meta:value' construction be a problem for childproxy properties ?
- Do you have any idea about localization of user visible meta/values ? (using meta for example)

(In reply to comment #28)
> Sounds great. I still miss a way for cross element preset sharing, but I guess
> it's not really feasible in any easy way.
I will soon add the presets attached to this bugreport (eq) to cvs. Next I'll improve the preset iface to first load presets form $prefix/share/gstreamer-0.10/presets and then overlay it with the presets found in $HOME/.gstreamer-0.10/presets. All changed presets will go to $HOME. Because of the overlay the user just sees his version. The only issue I see is that when deleting the user version, you see the system wide one again (confusing) and systemwide ones cannot be deleted. I probably need to expose this in the API so that one can test if a preset is system or user. Then edit/delete buttons can be disabled.

I've also been thinking about a preset merge tool. This way users can submit presets and we can merge those for future releases.

> Some remarks:
> - I like the simple format, why an xml extension for non xml content ?
It was a mistake. Its '.prs'

> - Won't the ':' in the 'meta:value' construction be a problem for childproxy
> properties ?
I still need to do those. Will probably use [] for the child.

> - Do you have any idea about localization of user visible meta/values ? (using
> meta for example)
I am a bit unsure abot meta. It could contain tags (quality, performance). Then one can ask gstreamer to optimize a pipiline for speed. It could conatin comments for the user (like now). The meta could store the comment together with the current local as
meta[en]:big booming bass
meta[de]:grosser dröhnender Bass

CVS Version has now the _get_preset_names API change to return a copied strv. Also the API has new _get_property_names vfunc that allows implementation to influence which properties to serialize and in which order.

Christophe, regarding systemwide and user local presets. Right now the UI in buzztard checks if a preset of a given name already exists. So one would have to dele a preset before beein able to recreate it under the same name. That means system wide ones will always be there and cannot be shadowed. But this is because of the UI. I am not sure what is the best beaviour yet. I tend towards shadowing, as this is the behaviour elsewhere (e.g. filesystem).

What about something like this:
1) install the shipped preset in a system location.
2) on first call of _get_preset_names(), the element checks for existence of a user-defined presets file.
3) if none present, copy system presets to user location.
4) let the user mess with its presets. If he goes to the point that all presets are delete, then the user-defined presets file becomes is empty (just a header).
5) If a power-user knows what she is doing, and delete its own preset file completely, then the system-wide defaults will be brought back as of 2)
6) For casual users, maybe provide a small utility (e.g. gst-preset-defaults "element-name") that just does: delete the user defined preset file.

oups, 2) should read: on first call to _load_presets ()

Copying system presets to user presets would need a mechanism to figure out that there are newer ones 8after system upgrade) and a merging mechanism :/

true, that breaks my KISS strategy...
So let me tweak it:
On upgrade, the new default presets get installed system-wide and the old ones (the user copy) stay untouched. The elements could declare a minimum version number for preset files (the 2nd line of the proposed format), signal a need for upgrade when trying to read the file and provide a merging mechanism.
The gst-preset-defaults could perform any update / merging and maybe get called by packaging scripts. 
If its current version is ok, that means the user will not see any new presets without calling explicitly the gst-preset-defaults tool (again a packaging system could do it for him).

I don't think merging is too hard to do (easy to say, I admit): read user file, build list of old presets, read new system file, build list of new presets, add each preset of the new list to the old list (if it isn't already there). I thought of the problem of new and missing properties, but isn't it a case of API break ?

User presets may not be a too sensitive piece of information, so a do_the_best_I_can strategy would seem sufficient to me.

Yes that sounds good. I'll implement this strategy:
gchar *preset_get_path(self) -> boolean preset_get_paths(self,&user,&system);
open both files
if system.exists
   check version
   if system.version > user.version
     read system
     updated = true
 if user.exists
   read user
 if updated
   write user

I'll also attach a new patch soon, as I had to revert the patch as people demandedfor more time.

Created attachment 99662 [details] [review]
add preset interface

Now uses defines from config.h for the paths'. Some todo updates.

Created attachment 99718 [details] [review]
add preset interface

the new patch has preset merging as discussed in previous comments implemented. that means we can ship presets and install them system wide and the default implementeation merges thme automatically into the user presets.

I mark this now as a core blocker to make sure it gets not postponed once again.

the last patch seems to be something with metadata...

Created attachment 100138 [details] [review]
add preset interface

Sorry, too many unreviewed patches :(

Forget it, this is again the wrong one, I 'll attach the latest tomorrow, its on different computer :/

Created attachment 100165 [details] [review]
add preset interface

Now its the right one.

Quick braindump. I see 3 components:

 (1) A global repository mapping 
     Element name -> multiple preset names -> multiple key/value pairs
     This object should allow for query and modification of the presets.

 (2) A component to use the repository and apply a setting by changing object
     properties. This will usually be done from the application but could 
     also be performed right after loading an element to restore its default
     settings. This could be merged into (1), depending on complexity.

 (3) A system to feed the repository
     From stored core files (in one or more formats)
     From stored user files
     From elements (based on element specific info like RDF, ...)
     From application code.

We definitely want to decouple (1) and (3) similar to the registry so that we can
use alternative file formats or sources for the presets. 

I don't see an immediate need to make elements implement an interface. I see this
as a way to configure GObject properties, which is independant of the GObject in
question. For feeding the presets (3) into the repository (1) you could just as
well push/merge it into the repo when the plugin loads (or ultimately an interface to pull them).

(1) and (2) seem to be below GStreamer, it could operate solely on GObjects. (3) could be part of core or needs at least more interaction with it (paths maybe).

Thats for commenting. Some comments in return:

GObjec vs. GstObject:
* I want to support the GstChildProxy, so it will work on GstObject.

Fileformat:
Actually I don't think it makes sense to have multiple registry formats supported. The binary registry works and therefore the xml one should die. less stuff to maintain. its a cache.
For the presets in the simillar way I don't see why we want to load ourselfs with supporting multiple formats. There is one format that we offer to gstreamer components. The format can be documented, but basically the elements should not care. And that is where the interface comes in. It allows certain plugins, to implement different formats. So far I see this only needed for bridge plugins (wrappers).
The second use of the interface is to signal that it makes sense for the element to have presets. This is especially important for the UI, as you don't want to show widgets to edit presets otherwise.
The thrid use of the interface is that elements can override *some* methods (right now its just get_property_names) to tell which properties should be serialized. Unfortunately we cannot infer this from the GParamFlags :/

I see a need to move some parts from the default implementation into a core-library. Once the preset system gets used people might send their presets and we need tools to merge them into the presets we ship with the plugins. The tools needed for the job, need to parse presets files from given path and save the merge presets to a given path. I could start to work on these tools and factoring parts out into a library, if there is agreement that this is good.

What's going on with this bug? It's a blocker for the release.

We're freezing Monday - is this API ready for release or not?

I don't see any of the above comments being addressed in a new patch yet. I would delay till after the release. We also don't usually make enhancements as blockers.

Wim, if you reffer to comment #44 - it's your thinking. I explained why I designed it the way it is and I still want to get it added as it is.

Jan, regarding the freeze - I marked it as blocker because I think its ready. I works I use it for more that a year in own elements and not getting it in blocks other plugins from using it.

So please lets continue the discussion. Comments on my reasoning in Comment #45 are welcome.

Wim, I read your comments again. This is a total different design. Below I call mine E and your design W.

E: presets are loaded when needed per element
W: all presets are always loaded

E: elements can override behaviour (influence order, skip properties)
W: only generic behaviour

E: elements can override format parsing if they don't want to use a default format  (bridge plugins)
W: elements would need to register a i/o modules if the want to support their own format

Basically I dont see the benefit to have all plugin in one store object. Presets or specific to one element. Other elements cannot use then in a meaningful way.

Core freeze is comming soon. I assume some people have this now in their local checkouts. Any issues. Please speak up and let me know if I can commit this finally.

*sigh* Well, speaking for myself, I didn't look at it until now, no. We're all busy people.

In general, I think it's a decent approach. I don't like:

* the gst_preset_create_preset function. A function to randomise the properties which have been designated for presets doesn't seem a generally useful thing. I think if such a thing is needed, it is better implemented as a utility function which iterates the list of properties and randomises them.
* Doubling up GST_PARAM_CONTROLLABLE to mark properties which should be included in the 'presets' set. It makes sense to have a simple way like that to mark Preset-table properties, but I think it should be a separate flag. It doesn't make sense, for example, for an MPEG encoder to have the GOP size be controllable, but it makes sense for that to appear in a preset.
* The function of gst_preset_set/get_meta methods is obscure to me. What's the point of being able to add generic tagged info to a preset? What does it mean to the application? To the user? Is there a structure to them? What are the rules?
* Christopher Dehais mentioned using GKeyFile for his RB equalizer presets, which, along with GValue transformations seems like a better approach than the existing manual serialising, deserialising and parsing code.
* There are some spelling mistakes in the docs, but those are easy to fix

There are some spurious docs bits in the gstreamer-sections.txt in the patch above that shouldn't be there afaics:
+gst_element_class_set_meta_data
+gst_element_factory_get_meta_data_detail
+gst_query_new_qos
+gst_query_set_qos
+gst_query_parse_qos

You hopefully already realise this, but 1 day before the freeze really isn't the right time to have brought this up again - poking us continuously since the last release would have been better.

(In reply to comment #51)
> *sigh* Well, speaking for myself, I didn't look at it until now, no. We're all
> busy people.

Thanks for taking time to review.
> 
> In general, I think it's a decent approach. I don't like:
> 
> * the gst_preset_create_preset function. A function to randomise the properties
> which have been designated for presets doesn't seem a generally useful thing. I
> think if such a thing is needed, it is better implemented as a utility function
> which iterates the list of properties and randomises them.

Nope. Its an interface with a default implementation. Element can override and provide a resonable randomization. Applications can call it regardless of the implementations. That won't work with utility functions.

> * Doubling up GST_PARAM_CONTROLLABLE to mark properties which should be
> included in the 'presets' set. It makes sense to have a simple way like that to
> mark Preset-table properties, but I think it should be a separate flag. It
> doesn't make sense, for example, for an MPEG encoder to have the GOP size be
> controllable, but it makes sense for that to appear in a preset.

The is an interface function (gst_preset_get_property_names) that elements can override. This function allows them to drop names of parameter that should not be serialized. The use of the GST_PARAM_CONTROLLABLE is indeed debatable, but is useful right now.

> * The function of gst_preset_set/get_meta methods is obscure to me. What's the
> point of being able to add generic tagged info to a preset? What does it mean
> to the application? To the user? Is there a structure to them? What are the
> rules?

That was discussed several times in IRC. I will improve the docs.

> * Christopher Dehais mentioned using GKeyFile for his RB equalizer presets,
> which, along with GValue transformations seems like a better approach than the
> existing manual serialising, deserialising and parsing code.

Can you point me to his patch for a resulable implementation on the gstreamer level. Sarcasm asise, I am aware that they cooked up some own way to handle it, despite my and slomos efforts to make the equalizer usable for them. They are aware of this effort, but have not contributed to it. Regarding GKeyfile, could be used, but won't save many lines. Regarding GValue transformations, we need anything to string and string to anything. I am not sure how GValue to GValue will help. You mean converting a int GValue to a string GValue then get the string content from that?

> * There are some spelling mistakes in the docs, but those are easy to fix
Yes, I'll spellcheck before updating the patch.

> 
> There are some spurious docs bits in the gstreamer-sections.txt in the patch
> above that shouldn't be there afaics:
> +gst_element_class_set_meta_data
> +gst_element_factory_get_meta_data_detail
> +gst_query_new_qos
> +gst_query_set_qos
> +gst_query_parse_qos

That happens when people leave me with uncommittable changes for that long. I'll 
manually patch the section file when updating the patch.

> 
> You hopefully already realise this, but 1 day before the freeze really isn't
> the right time to have brought this up again - poking us continuously since the
> last release would have been better.

I was poking you since last release and relase before. I have sent several emails about this. This time I misread the freeze schedule and took the 10th of march for the freeze as the table seems to include week nuber for odd reasons.

This is slowly getting a hard test for patience. I am happy to discuss the details and make changes. Its seems to be total failure to discuss this in bugzilla. Any suggestions? Review day in irc?

> Can you point me to his patch for a resulable implementation on the gstreamer
> level. Sarcasm asise, I am aware that they cooked up some own way to handle it,
> despite my and slomos efforts to make the equalizer usable for them. They are
> aware of this effort, but have not contributed to it. Regarding GKeyfile, could
> be used, but won't save many lines. Regarding GValue transformations, we need
> anything to string and string to anything. I am not sure how GValue to GValue
> will help. You mean converting a int GValue to a string GValue then get the
> string content from that?
> 

Well, at the time I started the RB equalizer plugin, I wasn't aware of this preset effort. I found GKeyfile compelling because there's native support for them in glib so I didn't have to cook up my own format and also because they handle localized properties (so you can ship a preset file with localized names).
For lack of time and knowledge of gstreamer, I really just followed this preset stuff from a distance, and I would be quite happy to use this infrastructure. I'm sorry I couldn't provide some more concrete help. 

(I should also mention that for quite a long time the main problem was to make any of the equalizer elements to work with the RB pipeline. Things seems to be a little better now with the "equalizer" element landing in -good, but the ladspa ones are still quite a mess (they're not in -bad for no reason).)

Christophe,

I hope I have not offended you, I feel a bit frustrated with this. Could you add links to viewvc of the rb sources where you implemented it? Maybe I can take some of the ideas there and bring them together in one of the next patch iterations.

It would be great to later on migrate the rb presets to the gstreamer infrastructure, if we get consensus here.

(In reply to comment #54)
> Christophe,
> 
> I hope I have not offended you, I feel a bit frustrated with this.

Not at all. I feel a bit ashamed of myself considering that I didn't helped much and still haven't succeeded in polishing the RB equalizer plugin enough. Time, Time, Time !


> Could you
> add links to viewvc of the rb sources where you implemented it? Maybe I can
> take some of the ideas there and bring them together in one of the next patch
> iterations.
> 

The patch is attached to Bug 76522. The relevant code is actually part of the equalizer widget code:

http://bugzilla.gnome.org/attachment.cgi?id=99005&action=diff#plugins/equalizer/rb-equalizer.c_sec1

(search for the "Loading / exporting to Keyfile" comment at the end)

A sample GKeyfile is here:

http://bugzilla.gnome.org/attachment.cgi?id=99005&action=diff#plugins/equalizer/eq-presets.ini_sec1


> It would be great to later on migrate the rb presets to the gstreamer
> infrastructure, if we get consensus here.
> 
That would be great. Maybe how Banshee does this is worth a look too:

http://abock.org/2008/01/25/software-eq-in-banshee/

Also, If I understand your proposal well, all is needed is for the equalizer element to implement the preset interface and then every app using it would have access to the same presets ? If so, that would be quite easy to port the current code in the RB plugin to use that (removing the customized preset handling from the widget itself (which was not that nice an idea to start with)).

(In reply to comment #52)
> (In reply to comment #51)
> > 
> > * the gst_preset_create_preset function. A function to randomise the properties
> > which have been designated for presets doesn't seem a generally useful thing. I
> > think if such a thing is needed, it is better implemented as a utility function
> > which iterates the list of properties and randomises them.
> 
> Nope. Its an interface with a default implementation. Element can override and
> provide a resonable randomization. Applications can call it regardless of the
> implementations. That won't work with utility functions.

More generally I meant that the idea behind the 'gst_preset_create_preset' function seems broken. As I understand it, the definition is 'configure the element in some unspecified way with a set of properties which could be saved as a new preset'. Given that, the only sensible way to implement it is to randomise the properties in some way. By putting it in the interface, it lets the element control how the randomisation is done, but effectively this is a 'gst_preset_randomise_properties' function. My objection is that while the rest of the interface is very generic, 'gst_preset_create_preset' is quite specific to your use case and doesn't seem useful in any others I can see - for example setting up video/audio encoder configurations, or standard colorkey profiles for the alpha plugin. This same functionality could be implemented in several other ways, without shoe-horning it into the interface:

* Elements which can be sensibly randomised could provide a 'virtual' profile called 'Randomise' which performs the randomisation when loaded.
* Provide a signal named 'randomise' and emit it into the element.
* Provide a property named 'randomise' which would do the randomisation when set.

To be clear - I'm raising an objection because I think this function doesn't gel well with the rest of the interface, but I don't mind sufficiently to stop you. At the least though, I'd prefer that randomisation wasn't the default implementation unless the elements requested it in some way, since it really doesn't make sense to randomise theoraenc's properties for example. 

> > * Doubling up GST_PARAM_CONTROLLABLE to mark properties which should be
> > included in the 'presets' set. It makes sense to have a simple way like that to
> > mark Preset-table properties, but I think it should be a separate flag. It
> > doesn't make sense, for example, for an MPEG encoder to have the GOP size be
> > controllable, but it makes sense for that to appear in a preset.
> 
> The is an interface function (gst_preset_get_property_names) that elements can
> override. This function allows them to drop names of parameter that should not
> be serialized. The use of the GST_PARAM_CONTROLLABLE is indeed debatable, but
> is useful right now.

Sure, but then people that want to implement the interface need to do more work. As an idea, we can supply a couple of default functions for implementations to choose from - list all GST_PARAM_CONTROLLABLE properties might be one, present a NULL terminated string list could be another, a property flag GST_PARAM_PRESETS and a function to list those might be another. 

> > * The function of gst_preset_set/get_meta methods is obscure to me. What's the
> > point of being able to add generic tagged info to a preset? What does it mean
> > to the application? To the user? Is there a structure to them? What are the
> > rules?
> 
> That was discussed several times in IRC. I will improve the docs.

OK

> > * Christopher Dehais mentioned using GKeyFile for his RB equalizer presets,
> > which, along with GValue transformations seems like a better approach than the
> > existing manual serialising, deserialising and parsing code.
> 
> Can you point me to his patch for a resulable implementation on the gstreamer
> level. Sarcasm asise, I am aware that they cooked up some own way to handle it,
> despite my and slomos efforts to make the equalizer usable for them. They are
> aware of this effort, but have not contributed to it. Regarding GKeyfile, could
> be used, but won't save many lines. Regarding GValue transformations, we need
> anything to string and string to anything. I am not sure how GValue to GValue
> will help. You mean converting a int GValue to a string GValue then get the
> string content from that?

Right - using the existing g_value_transforms would simplify the loops in gst_preset_default_save_preset and gst_preset_default_load_preset and allow automatic support for any type that has conversions to/from string defined - like all the standard GTypes, and GstFraction, and even complex types like GstCaps. Something like:

for prop_name in props_list:
  g_value_init (&str_value, G_TYPE_STRING);
  g_object_get_property (G_OBJECT (self), prop_name, &in_value);
  if (g_value_transform (&in_value, &str_value)) {
    g_hash_table_insert (data, prop_name, 
        g_strdup (g_value_get_string (&str_value));
  }
  g_value_reset (&str_value);

Created attachment 106977 [details] [review]
add preset interface

This iteration adresses most of jan's points:
* strip anrelated changes
* fix spelling
* remove GST_PARAM_CONTROLLABLE use

Regarding meta - its basically a comment for the preset (as the api docs say). The ui can use this to search presets.

I will think about the preset-randomization. Elements that do not like it can/should override the method.

Besides I still like to get feedback on the @todo: items in the source.

Applied the patch, looking at it now. It seems I have the same comments as everyone else:

* We need GST_PARAM_PRESET (or GST_PARAM_NO_PRESET) to easily filter out unwanted params (like name and likely many other properties in encoders). A flag to disable certain properties from being included in the presets might be easier because then we don't need to change each element, just the ones that have properties that should be excluded.

* A lot of code is needed for reading the file. I wonder if a combination of GKeyFile and The GValue serialisation would simplify things.

* create_preset() indeed needs to be renamed to _randomize(). AFAIK, you give a name to a preset with _save_preset(), which you can then get back with _load_preset().

* do we want to define some standard tag names? Seems like a define for a "comment" and a "description" tag would avoid having a multitude of applications using incompatible random string for the same tags names. Maybe state that you have to use the strings from GST_TAG?

I'll experiment a little with it.

Another thing, It would be nice to activate a preset with a property. I don't care for the randomize or saving of presets with a property, just loading.

ok, it seems people on IRC seem to think the randomize function is a little redundant as it can easily be implemented with a 'random' preset.

I'll definitely look into the GKeyfile and GValue transforms next.

Regarding the "randomize" preset. I don't like that. It would require special code that ensures noone creates a preset named like this.

Regarding GST_PARAM_PRESET:
* GST_PARAM_CONTROLABLE marks parameters that are realtime controlable and thus are good preset candidates
* I would rather add GST_PARAM_EDITABLE (name is a suggestion) as well. This would be a hint to a preset parameter too and also servers a hint for dynamic UI generation.
If we can agree about the flag here, I would like to seize the change and move them to gstobject.h (to avoid later clashes with G_PARAM_USER_SHIFT).

Wim: another one. I was asking for comments on the @todo: markers in the source. There first one is releated to what you meant with loading presets via properties:

 - should there be a 'preset-list' property to get the preset list
   (and to connect a notify:: to to listen for changes)
   we could use gnome_vfs_monitor_add() to monitor the user preset_file.

 - should there be a 'preset-name' property so that we can set a preset via
   gst-launch

I need to check if one chan add interface properties later. If this breaks the interface, it would be good to decide about those now.

More comments:

GKeyfile could be used, but would ev. lead to ugly keys being used:
http://www.buzztard.org/index.php/Preset_handling_interface#formats

Regarding the GValue transforms. Probably not:
http://library.gnome.org/devel/gobject/stable/gobject-Generic-values.html#id2715246

I would like to use this interface before moving the dvb plugin to good/ugly.

If we want a "preset-name" iface property, so that we can specify a preset on gst-launch we need to decide now. If we add this later, elements that implement the iface, but which do not override the property will cause a warning

(gst-inspect-0.10:23733): GLib-GObject-CRITICAL **: Object class GstSimSyn doesn't implement property 'preset-name' from interface 'GstPreset'

Adding it on the preset-iface side is easy: gst_preset_base_init() will need:
    /* create interface properties, each element would need to override this
     *   g_object_class_override_property(gobject_class, PROP_PRESET_NAME, "preset-name");
     * and in _set_propert() do
     *   case PROP_PRESET_NAME: {
     *     gchar *name = g_value_get_string (value);
     *     if (name)
     *       gst_preset_load_preset(self, name);
     *   } break;
     */
    g_object_interface_install_property (g_class,
      g_param_spec_string ("preset-name",
      "preset-name property",
      "load given preset hen setting",
      NULL,
      G_PARAM_WRITABLE));


One more comment. If we decide for adding the iface to gstreamer core. I can do follow-up patches to support it in gst-inspect, so that it can show a list of presets.

Created attachment 107153 [details] [review]
add preset interface

Remove some duplicated code. Longer comment in gst_preset_create_preset().

> GKeyfile could be used, but would ev. lead to ugly keys being used:
> http://www.buzztard.org/index.php/Preset_handling_interface#formats

I don't find the GKeyFile format unbearably ugly to be honest. Also, what's the problem with XML? You say 'bad: xml dependency', but for short files it should be perfectly ok to just use GMarkupParser, no? The only reason we don't use it for the registry is that the registry is very large and libxml2 is much faster.

 
> Regarding the GValue transforms. Probably not:
> http://library.gnome.org/devel/gobject/stable/gobject-Generic-values.html#id2715246

Could you expand on this (or update the link)?  I'm not sure I see the problem here.  We already have gst_value_serialize() and gst_value_deserialize() which we use for basically the same purpose.  It would seem to me it would be best to make those handle the necessary transforms if they don't already, better than to basically re-implement them.

In fact, I wonder if it wouldn't even be possible to use GstStructure serialisation/deserialisation here, possibly in combination with GKeyFile.

Created attachment 107215 [details] [review]
add preset interface

* use gst_value_serialize() and gst_value_deserialize().
* make it work again (try the qualizer Bug #522183)

Pending discussions:
* decide about preset-name / preset-list iface properties
* decide about gstchild-proxy
* agree on _create_preset() vmethod

Preding code todo:
* have a look at GKeyfile
* remove/fixup some debug logging

A ping for everyone about the status.

I lookied into the GKeyfile stuff. Generally not a bad idea if the GKeyfile API would not suck so much. If you want GKeyfile, I need a workaround for Bug #371370 at least. Also Bug #350000 could mean trouble when creating initial presets. So whats the options:
a.) I tweak my parser to read the same format as the GKeyfile would be, then we can later switch to GKeyfile, once the API is good enough.
b.) Do not use GKeyfile

Created attachment 109284 [details] [review]
add preset interface

This is now GKeyFile based. It just saves 170 line sof code as gkeyfile could have some more API (like renaming sections). if we go with this I am going to do some patches for GKeyFile to move some of the implementation to glib.

There are still some todo's in the top-comment:
* we need locks to avoid two instances writing the preset file

* need to add support for GstChildProxy
  I can handle that in the next iteration
  (see http://www.buzztard.org/index.php/Preset_handling_interface#GKeyFile_format for the format)

* preset-list change notifification

* preset-name selection from gst-launch

comments welcome. please help me with commnets to get it ready.

Created attachment 109291 [details] [review]
add preset interface

* fix some spelling errors in comment
* more efficient version parsing
* make value in set_meta const

next is to turn ::create_preset() into either:
* ::randomize() and also add ::reset()
* or add configure(mode), where mode can be DEFAULT|RANDOM and ev. MIN|MAX
  is min/max useful?

Created attachment 109404 [details] [review]
add preset interface

This version now has
  gst_preset_randomize()
  gst_preset_reset()
instead of
  gst_preset_create_preset()

I have updated Bug #522183 with two equalizer presets in the new format.
Buzztard svn has been updated, so you can try those there.
And Bug #522205 has been updated to now only have PRESETTABLE/UI_HINT flags pending.

Can you explain why randomising a preset would ever be a useful thing to do? I can't imagine any case where it'd make sense. Mostly I'd expect it'd result in things that don't even work.

I suspect (but haven't really checked) that it's functionality specific to buzztard that you want - so why not just put the code in buzztard?

(In reply to comment #74)
> Can you explain why randomising a preset would ever be a useful thing to do? I
> can't imagine any case where it'd make sense. Mostly I'd expect it'd result in
> things that don't even work.
> 
> I suspect (but haven't really checked) that it's functionality specific to
> buzztard that you want - so why not just put the code in buzztard?
> 

I suggested to Stefan an enum for reset() (MIN,MAX,DEFAULT), that the implementation could override and enhance. But apparently, he didn't like the idea. :-/

Mike, as explained (e.g. comments #52, #57, #61) already is an interface method and a the patch brings a default. Its is generally useful for effects and generators (both audio and video) that have lots of parameters. For audio one would loop a sequence and hit randomize to get some fresh sounds (which one would tune then and maybe save as a new preset).

If an element stops working after setting a random presets its broken and need to be fixed. The randomization is done within the paramspec bounds.

Now it clearly does not make any sense for e.g. encoders. those can override it with a dummy method. But then I can't imagine why one would create a ui where one can call randomize preset on an encoder.

One alternative would be to have a dummy default (vmethod=NULL) and just provide the randomization as a exported function, so that elements can select it 8and we don't have duplicated code).



Marc-Andre: I was looking at several elements, but could not think of any use-case for min/max.

If we can agree about the remaining detals and the cde can go in, it would be nice to have it covered by tests. Some toughts for this:

* implement it in a core plugin (queue, identity - any suggestions?)
  (e.g. the gsturi test relies on having something in the registry that implements the iface)

* I could do a "test" element, but that would result in a test.prs being generated in users ~/-gstreamer-0.10/presets dir (can be cleaned after test run). is that an issue? (we should have $GST_HOME instead of $GST_REGISTRY).

Created attachment 109671 [details] [review]
add preset interface

This patch now has a testsuite with 5 tests implemented. It also fixes one leak oncovered by the testsuite.

Mike and wtay don't like the api for default() and random(). While I find that quite useful, if its the collective decission, I can definitly just move them to buzztard. If we feel that all of a sudden 10 apps have copied the code, we can still extend the iface :).

I am definitely don't like the ideas to have special named entries in the list. Atleast it needs again some api in turn so that the element can provide the pre-defined entries it supports and a way to ensure that names don't conflict.

Is there anything besides this that needs discussion/changes?

Created attachment 110055 [details] [review]
updated patch

Updated patch:

 - removed _reset() and _randomize() methods for now, I have some ideas to
   nicely do it differently with the current API.
 - remove gst_preset_get_property_names() to gst_preset_list_properties() so
   that it matches g_object_class_list_properties() in signature and 
   functionality. This also alows us to avoid potential races and simplifies
   the code.
 - clean up the code that gets and caches the paths.
 - fix problems where calling some methods did not initialize the GKeyFile for
   the GType.
 - clean up error paths to reduce indentation depth and code clarity to make it
   more gstreamer like. Fix some cleanup errors along the way.
 - Fix keyfile name <-> element index error for the header.
 - Simplify version parsing.
 - fix unneeded GList copying and sorting to make it work mostly in-place.

Created attachment 110404 [details] [review]
updated patch

readd missing Makefile.am, small test cleanup

A couple of tiny comments, otherwise I think this looks okay. What's the plan with the 'randomize' stuff though? Wim said he had an idea?

Comments:
 * When using sscanf to parse the version string, it's probably worth checking that it parsed
    at least 2 integers (num >= 2)
 * gst-lanunch -> gst-launch in a FIXME toward the top

Created attachment 110762 [details] [review]
updated patch

* use "element-name" instead of "name" in header
* ensure that parsed versions have atleast two valid digits
* updated todo comments

Regarding the child-proxy support. One use-case would be a capture-bin or encoder-bin: there one would like to have presets that cover several elements.

Created attachment 110841 [details] [review]
updated patch

updated patch. Changes list_properties() to _get_property_names() to make it possible to use childproxy later. Fixes some leaks too.

I was thinking of something else for the presets that require algorithms:

 gst_preset_get_algorithms (): returns a list of supported algorithms 
 gst_preset_apply_algorithm (): perform an algorithm on the preset properties.

The naming is probably not very good, what I'm trying to achieve is have at least 2 predefined algorithms for changing the properties:

 RESTORE_DEFAULTS
 RANDOMIZE

More can be defined, we need get_algorithms() and apply_algorithm to the vmethods to make this work. It's kindof similar to have presets that start with an identifier (like *defaults and *randomize) but a bit more explicit.

I still prefer just having the special preset name prefix to mark actions, with a couple of predefined strings for the actions we support by default.

Ping? Any chance we'll get this in before core freezes on Monday?

        Patch by: Stefan Kost  <ensonic@users.sf.net>

        * configure.ac:
        Add DATADIR for storing presets.

        * docs/gst/gstreamer-docs.sgml:
        * docs/gst/gstreamer-sections.txt:
        * docs/gst/gstreamer.types.in:
        Add GstPreset to docs.

        * gst/Makefile.am:
        * gst/gst.h:
        * gst/gstpreset.c: (preset_get_paths), (preset_skip_property),
        (preset_open_and_parse_header), (preset_parse_version),
        (preset_merge), (preset_get_keyfile),
        (gst_preset_default_get_preset_names),
        (gst_preset_default_get_property_names),
        (gst_preset_default_load_preset),
        (gst_preset_default_save_presets_file),
        (gst_preset_default_save_preset),
        (gst_preset_default_rename_preset),
        (gst_preset_default_delete_preset), (gst_preset_default_set_meta),
        (gst_preset_default_get_meta), (gst_preset_default_randomize),
        (gst_preset_default_reset), (gst_preset_get_preset_names),
        (gst_preset_get_property_names), (gst_preset_load_preset),
        (gst_preset_save_preset), (gst_preset_rename_preset),
        (gst_preset_delete_preset), (gst_preset_set_meta),
        (gst_preset_get_meta), (gst_preset_class_init),
        (gst_preset_base_init), (gst_preset_get_type):
        * gst/gstpreset.h:
        Add GstPreset to core. Fixes #396779

        * tests/check/Makefile.am:
        * tests/check/gst/gstpreset.c: (gst_preset_test_get_property),
        (gst_preset_test_set_property), (gst_preset_test_class_init),
        (gst_preset_test_base_init), (gst_preset_test_get_type),
        (gst_preset_test_plugin_init), (GST_START_TEST),
        (remove_preset_file), (test_setup), (test_teardown),
        (gst_preset_suite):
        Add GstPreset unit tests.