please, don't create a new interface just for a readonly value :)

a virtual function to GstElement instead?

the iface is indeed a bit too much. What about adding this to GstElementDetails?

there could be GST_ELEMENT_DETAILS_WITH_APIDOCS(longname,klass,description,author,element_name)
that also sets the sets the uri to
"file://"DATADIR""G_DIR_SEPARATOR_S"gtk-doc"G_DIR_SEPARATOR_S"html"G_DIR_SEPARATOR_S""PACKAGE""G_DIR_SEPARATOR_S""PACKAGE"-%s.html", element_name

IMHO the interface for a single gobject property would be overkill too... so the addition to GstElementDetails is probably the best. Every other possibility I can think about sounds too complicated :)

But instead of hardcoding the URI to "file:///...." I would just specify the complete URI. IMHO having this kind of automatism seems to be very error-prone and could easily lead to URIs pointing to nowhere... though this is probably just a matter of taste ;)

The UIR I suggested would be generated from defines that configure puts into config.h. Thus it would be automatically correct :)

If people use auto* and gtk-doc, yes. Well, for most cases this should work nonetheless, right :)

Only cases where this would break is when people don't use gtk-doc or only want the docs on some website, etc... and in this cases this could still be done by not using the macro... I'm fine with this

This is kind of what GstPluginDesc::origin is supposed to be.  Origin is more of a base URL, however, a documentation URL makes a lot of sense in addition to the base URL.

The reason for proposing to add this to GstElementDetails are wrapper plugins. I'll make a patch as proposed in comment #2.

Better idea:
  - use a GstStructure *metadata;
How:
  - needs fixes in gstelement.{c,h},gstelementfactory.{c,h}
  - gst_element_class_set_metadata(GstElementClass *klass, const gchar * field, ...)
    -> gst_structure_set_valist(klass->metadata, ...)
       & gst_element_class_set_details_simple()
  - gst_element_class_set_details, gst_element_class_set_details_simple
    gst_structure_set(klass->metadata, ...)

Created attachment 97416 [details] [review]
turns GstElementDetails into a GstStructure

How would it be used:
  gst_element_class_set_meta_data (gstelement_class,
      "longname", G_TYPE_STRING, "File Sink",
      "klass", G_TYPE_STRING, "Sink/File",
      "description", G_TYPE_STRING, "Write stream to a file",
      "author", G_TYPE_STRING, "Thomas <thomas@apestaart.org>",
      NULL);

Question 1: do we want a macro for it?

Question 2: do we want to deprecate the older functions? I made them backwards compatible. So both gst_element_class_set_details_simple and gst_element_class_set_details create the structure too.

Created attachment 97701 [details] [review]
turns GstElementDetails into a GstStructure

I now keep GstElementDetails for backwards compat in both GstElement and GstElementFactory. The new meta element is taking one from the resered_padding.

I am not so much worried that the strings in GstElementDetails cannot be keep syncronous if one does a gst_structure_set on the new meta_data item. First I pt it into private - it should not be accessed directure. Same applied to details field in the past. If one would change the string in GstElement.deatils, the same string in GstElementFactory.factory would not be automatically updated.

I am not yet sure, if I can use one and the same structure for both GstElement and GstElementFactory.
gst_element_register() does
factory->meta_data = gst_structure_copy(klass->meta_data);
as there is no gst_structure_ref() (need to experiment with parents_refcount).

We recently got Bug #491501 filed. If we want to allow that classes set their element_details in base_init and subclases overwrite those fields, the we should use the structure based approach. Atleast then we can also overwrite only some fields: Pipeline subclasses Bin, but "Klass" remains "Generic/Bin".

Another new Detail would be icon or icon-theme-name. I could try to change the patch so that it keeps the current 4 element-details as they are and uses one of the _gst_private pointers for the GstStructure that will hold the extra details. This will be more save. For 0.11 we can rethink the api here.

I like the idea in general but we probably should consider to fix bug #491501 (the details part) first and then get this here done.

Also, IMHO, we should deprecate all the old functions as you intended.

In gstelementfactory.c:283 you should also check for longname being correct, and IMHO we should add a FIXME 0.11 for the description check. Nowadays it seems to be allowed to be empty and we can't change that right now.

Then there are some changes in the registries that seem to be unrelated to this bug, would be nice to have them as an additional patch (they seem to be fine though).

Apart from that, I don't see anything wrong in the latest patch.

Ok, as bug #491501 is done now, please let us get this done too now for the next release. Stefan? :)

GstElementDetails => core

Stefan, any news on this? Bug #491501 can only be fixed in 0.11 but this one here can be fixed now already and probably should be fixed now

I'll do it.

Created attachment 167500 [details] [review]
allow arbitrary plugin metadata

This patch does not attempt to replace the exiting fields (longname, class, author, ...); If wanted I can do that and maintain a GstElementDetails structure with copies for backwards compatibility. That would allow to port elements already now. Please also give feedback on API names.

Elements would now call this after gst_element_class_set_details_simple()
  gst_element_class_set_meta_data (gstelement_class,
      "Test", G_TYPE_STRING, "test-content",
      "Peng", G_TYPE_STRING, "dum di dum",
      NULL);

Review of attachment 167500 [details] [review]:

Looks good, two minor things below

::: gst/gstelement.c
@@ +1240,3 @@
+
+  if (klass->meta_data) {
+    GST_DEBUG ("clear metadata in class %p to %p", klass, klass->meta_data);

So this means, that the metadata of the parent class is overwritten.

I think for 0.11 we might want to change that, same for the other element details. For example for inheriting the classification of the parent class

@@ +1250,3 @@
+}
+
+/* FIXME-0.11: deperate and remove gst_element_class_set_details*() */

small typo, deprecate ;)

> Looks good, (...)

I might have a few more comments, please don't commit this quite yet :)

(In reply to comment #19)
> Review of attachment 167500 [details] [review]:
> 
> Looks good, two minor things below
> 
> ::: gst/gstelement.c
> @@ +1240,3 @@
> +
> +  if (klass->meta_data) {
> +    GST_DEBUG ("clear metadata in class %p to %p", klass, klass->meta_data);
> 
> So this means, that the metadata of the parent class is overwritten.
> 
> I think for 0.11 we might want to change that, same for the other element
> details. For example for inheriting the classification of the parent class

Oh and you might want to enforce the clearing of the metadata in GstElement::base_init to get really the same behaviour as for the other element details.


And if we agree that something like your patch should go in, we should first define metadata fields and their usage. Otherwise elements might use different fields for the same thing...

(In reply to comment #19)
> Review of attachment 167500 [details] [review]:
> 
> Looks good, two minor things below
> 
> ::: gst/gstelement.c
> @@ +1240,3 @@
> +
> +  if (klass->meta_data) {
> +    GST_DEBUG ("clear metadata in class %p to %p", klass, klass->meta_data);
> 
> So this means, that the metadata of the parent class is overwritten.
> 
> I think for 0.11 we might want to change that, same for the other element
> details. For example for inheriting the classification of the parent class

We could also use GstTaglist a-like merging. Like we copy the parent meta-data and replace some value with the new ones.

> 
> @@ +1250,3 @@
> +}
> +
> +/* FIXME-0.11: deperate and remove gst_element_class_set_details*() */
> 
> small typo, deprecate ;)

fixed locally.


(In reply to comment #21)
> (In reply to comment #19)
> > Review of attachment 167500 [details] [review] [details]:
> > 
> > Looks good, two minor things below
> > 
> > ::: gst/gstelement.c
> > @@ +1240,3 @@
> > +
> > +  if (klass->meta_data) {
> > +    GST_DEBUG ("clear metadata in class %p to %p", klass, klass->meta_data);
> > 
> > So this means, that the metadata of the parent class is overwritten.
> > 
> > I think for 0.11 we might want to change that, same for the other element
> > details. For example for inheriting the classification of the parent class
> 
> Oh and you might want to enforce the clearing of the metadata in
> GstElement::base_init to get really the same behaviour as for the other element
> details.
> 
> 
> And if we agree that something like your patch should go in, we should first
> define metadata fields and their usage. Otherwise elements might use different
> fields for the same thing...

This could be also done like for GstTaglist. Right now I would just use it for:
<string> help-uri
I was once thinking of a processor field (CPU, GPU, DSP, ...), this way one could maybe prefer hw-accelerated elements, but thats not really good either.
Any other thoughts?

A summary of open questions:

the patch currently adds two methods:
gst_element_class_set_meta_data (klass, fieldname, ...);
const gchar * gst_element_factory_get_meta_data_detail(factory,detail)

The setter is imho fine. Only having this getter has the limitation that it only works for string details and one cannot iterate details (gst-inspect is grabbing the GstStructure directly, which is bad as it is supposed to be private).


Regarding the known keys, we could do it like gst_tag_register(). I don't think we need flags. Any opinion on a merge behaviour for base-classes? How to name the beast - gst_element_meta_data_register()? Do we actually want to allow to externaly register new ones. I could also just internally register some and have #defines in the public API (like GST_TAG_TITLE). I think that is sufficient.

Created attachment 167691 [details] [review]
allow arbitrary plugin metadata

Small update. Fixes the type and adds two meta-data key defines as proposed in last comment. There is no enforcement of the keys. People can still use arbitrary strings but are suggested to use the defined ones and file enhancement requests for new ones.

I'm a bit undecided about this. What's the intention here exactly? Just make things more extensible at the API level and more generic at the implementation level?

Do we want to allow 'unsanctioned' new fields? (This I'm not sure about - of course it doesn't *hurt*, but is it useful? Does it make good API?)

If we do not want to allow arbitrary fields, why not just add

  gst_element_class_set_doc_uri (),
  gst_element_class_set_icon_name(), etc. ?

If we do want to allow arbitrary fields, I think simple API like:

  gst_element_class_set_string_detail (klass, "key", "value"), or
  gst_element_class_set_detail_string (klass, "key", "value"), or just
  gst_element_class_set_detail (klass, "key", "value");

would be both sufficient for now and much nicer (not least for bindings). I prefer the latter, fwiw :) I would avoid the word 'METADATA' then, and just go for GST_ELEMENT_DETAIL_DOC_URI or somesuch.

I would avoid vararg functions, and G_TYPE_* and all that. Surely another two or three function calls here aren't really signficant performance-wise? Avoiding this would also make it easier to use a different implementation that's more suitable/efficient for serialisation/deserialisation at a later stage (e.g. GVariant).

I'm not in favour of a registration system for fields here, that seems a bit over the top (I saw that the patch doesn't implement that, just saying).


> /*< private >*/
> GstStructure		 *meta_data;

I think we should hide the implementation of this field for the reasons described above. Even if we declare it private, it never really is. We can use a dummy pointer typedef here that is then typedef'ed in gst_private.h to GstStructure *. That way we can be 100% sure we can change it later.


> gst_element_factory_get_meta_data_detail (GstElementFactory * factory,
>                                                                                 const gchar              * detail);

Similar to what I wrote above, I'd avoid the "meta_data" phrase here and just make it

    gst_element_factory_get_detail (factory, detail);

    (or gst_element_factory_get_detail_string (factory, detail);
     or gst_element_factory_get_string_string (factory, detail); if needed).

> I prefer the latter, fwiw :)

Just to clarify: I think I generally prefer the _set_doc_uri() variant that doesn't allow arbitrary tags, but if the consensus is that arbitrary metadata is desirable, then I favour the_set_detail() variant without the 'string' bit in the name.

Okay, so what about using the GstStructure for the new fields (and in 0.11 for the existing ones too), but add _get|set functions. Thus no gst_element_factory_get_detail() or what so ever, but:

  gst_element_class_set_doc_uri ()
  gst_element_class_set_icon_name()

  gst_element_factory_get_doc_uri ()
  gst_element_factory_get_icon_name ()

In gst-inspect we add new calls, when ever there are new fields. String based getters will return NULL in case the detail does not exist. If we add e.g. int based getters we need to do as in gst_structure (out variable, plus boolean return).

> /*< private >*/
> GstStructure		 *meta_data;

I would tend to keep this as it is. But I can also make it a gpointer and cast in the few methods using it.

I'll make a new patch if that sounds like a good plan.

I generally like the specific getter/setter too (set_doc_uri() etc) but it might be too limited in some cases. For example if there is the need for an audio effect specific detail later, it would mean adding a new function that is audio specific to core...

So in the end I'd prefer the gst_element_class_set_detail_string() and gst_element_class_get_detail_string() variants to allow the biggest extendability (e.g. media specific details, integers, etc)

> I generally like the specific getter/setter too (set_doc_uri() etc) but it
> might be too limited in some cases.

Ok, how about this then:

Have a general key/value setter/getter, but instead of providing key #defines we just provide

#define gst_element_class_set_doc_uri(klass,uri) \
    gst_element_class_set_detail_string (klass, "gst-doc-uri", (uri))

? That still results in nice-looking code, but also allows arbitrary keys, and since we don't provide API to iterate over / enumerate the details, no one needs any defines to strcmp against either for now.

(I can live with defines though if people don't like the above)

> #define gst_element_class_set_doc_uri(klass,uri) \
>     gst_element_class_set_detail_string (klass, "gst-doc-uri", (uri))

Hrm, or maybe that's awkward for dynamic bindings that do stuff at runtime based on the .gir files? Can they handle such things?

The setters indeed have the disadvantage of becoming somewhat domain specific. That was solved in the GstTaglist by having more tag defines elsewhere. Although sometimes those are not so easy to discover. People probably only find then as they share the namespace. In the same way we could introduce such defines/methods in audio/video libs as well. We'd just call them gst_element_class_set_audio_xxx() (or having a define GST_ELEMENT_METADATA_AUDIO_XXX) I'd use a 'namespace' for the actual key internaly (e.g. "audio::xxx");
If that is okay, we could still use methods instead of macros (to make it easier for bindings). Still having member spread across the packages would probably still confuse bindings.

A plan B would be to only add generic metadata in core and expose the GstStructure.:
- apps could iterate it (the only way for gst-inspect to show all of them if we start to define more in e.g. gst-plugin-base and unless we register known keys). 
- wrapper plugins can add own keys and just document them in the plugin

Just as an example, it has help the camera development a lot, that we can just use arbitrary strings in a taglist for r&d phase.

So, unless new arguments show up, my preference would be #defines for the key (for no only in core) and setters/getters as in #comment 28. The naming of the defines is still ugly though (GST_ELEMENT_METADATA_XXX, GST_DELEMENT_DETAIL_XXX).

I see no problem with having the defines or methods in different packages. We're already doing it now for tags and nobody complained so far :) But setter/getter macros might really be a problem for some bindings.

Instead of exposing the GstStructure directly for iterating we could have a getter, that returns a list of struct { const char *name; const GValue *value; }.

gst-inspect can use private headers from core for the iteration, and we can add API for the list/iteration later when someone actually has a good use case for it IMHO.

Created attachment 169235 [details] [review]
allow arbitrary plugin metadata

New implementation using methods instead of #defines.

> gst_structure_set (klass->meta_data, key, value, NULL);

 - ^^^ that works? I think there's a G_TYPE_STRING
   argument missing

 - I still would prefer to avoid exposing the GstStructure
   as such directly in the public header (reason: it'd be
   nice to be able to switch it to a GVariant later when we
   can use that)

Created attachment 169369 [details] [review]
allow arbitrary plugin metadata

Fixed and // comment and gst_element_class_add_meta_data(). Also now using a gpointer in the public headers and casting in the implementation.

> Created an attachment (id=169369) [details] [review]
> allow arbitrary plugin metadata
> 
> Fixed and // comment and gst_element_class_add_meta_data(). Also now using a
> gpointer in the public headers and casting in the implementation.

Looks ok to me. Three more comments:

 - maybe make gst-inspect only use the public API for now?
   (since we don't support arbitrary keys, there's no need to
   iterate yet, so it's safer not to do the cast to GstStructure
   here, for the unlikely corner case where
   gst-inspect version != libgstreamer version)

 - is the deserialisation of an empty meta string correct here? 

 - lastly, we could put the GstStructure * into a GstElementFactoryPrivate
   instead, then we keep it private but also don't need the casts
   (sorry, didn't think of that before)

* moving meta_data to  GstElementFactoryPrivate, break serialisation for the registry.
* the empty string serialization is okay (used already like this for plugin->cache_data)
* I'll keep the gst-inspect like it for simplicity, lets change that as soon as we add a first !string detail

commit 65356fbb7a74568cd528907c12f77eb6a42e7ad7
Author: Stefan Kost <ensonic@users.sf.net>
Date:   Tue Aug 10 14:05:22 2010 +0300

    element-details: allow for arbitrary element details
    
    Add a GstStructure to GstElementClass and GstElementFactory. Add setters/getter.
    Handle it in the registry code. Print items in gst-inspect.
    Fixes #396774.
    
    API: gst_element_class_set_XXX(), gst_element_factory_get_XXX()

Irks. Just after pushing, I noticed that I have not updated the API: comment in the commit :/

Comment on attachment 169369 [details] [review]
allow arbitrary plugin metadata

committed with small cleanup (win32 update and small serialisation change)

That commit broke the build (because it uses an uninitialized variable).

commit c5888dc6cf686bd150bd3b835a7abb256b371147
Author: Olivier Crête <olivier.crete@collabora.co.uk>
Date:   Mon Sep 6 14:09:52 2010 +0300

    registrychunks: Use the correct variable for debug message
    
    Debug print was using a variable that was not initialized.