Created attachment 167244 [details] [review]
pbutils: Add gstfactorylists

Created attachment 167245 [details] [review]
playback: Switch to gstfactorylist from pbutils

There's a bug in that last patch. I'm trying to figure it out.

I feel that at least parts of this patch belong into core.

The element factory API in core has always been a bit lacking in this respect, and the list stuff should be in core with the rest of the element factory API, since it's such a common thing to do.

Also, the "klasses" stuff should go into core IMHO (there's nothing "media-specific" about it, it's just a bunch of labels, and just draft-klass.txt in form of API), that's always been missing there.

Created attachment 167259 [details] [review]
pbutils: Add gstfactorylists

Created attachment 167260 [details] [review]
playback: Switch to gstfactorylist from pbutils

(In reply to comment #4)
> I feel that at least parts of this patch belong into core.
> 
> The element factory API in core has always been a bit lacking in this respect,
> and the list stuff should be in core with the rest of the element factory API,
> since it's such a common thing to do.
> 
> Also, the "klasses" stuff should go into core IMHO (there's nothing
> "media-specific" about it, it's just a bunch of labels, and just
> draft-klass.txt in form of API), that's always been missing there.

Now that the patches *work* in -base, I'm going to have a look at merging the factory-only parts of that into core.
Any suggestions to where I should add it ? gstelementfactory.h ? gstutils ?

Review of attachment 167259 [details] [review]:

First of all I don't think this belongs into core... There's stuff about audio and video in here.

Also, why don't you use a GstCaps instead of a GValueArray? That makes things a bit easier I guess... especially because your caps inside the value array only have a single, simple structure ;)

And then you might want to try to port subtitleoverlay to this. It needs a list of subtitle parsers/decoders and subtitle renderers.

Other comments below

::: gst-libs/gst/pbutils/gstfactorylists.c
@@ +348,3 @@
+    while (!gst_structure_foreach (st,
+            (GstStructureForeachFunc) remove_range_foreach, st));
+  }

For this you might want to take a look at http://svn.debian.org/viewsvn/pkg-gstreamer/unstable/gstreamer0.10/debian/gst-codec-info.c?revision=3298&view=markup

Search for "codec_data" and remove_min_max_fields(). I think that's a bit safer and better than simply removing *all* ranges.

@@ +353,3 @@
+
+  /* Explode to individual structures */
+  res2 = gst_caps_normalize (res);

Why is the normalizing here necessary?

@@ +376,3 @@
+    tmpc = gst_caps_new_full (st, NULL);
+
+    /* Check if the caps are already present */

If you were using a GstCaps instead of a GValueArray of normalized, simple caps this could be done by gst_caps_merge_structure() ;)

::: gst-libs/gst/pbutils/gstfactorylists.h
@@ +40,3 @@
+ * @GST_FACTORY_LIST_MEDIA_VIDEO: Elements handling video media types
+ * @GST_FACTORY_LIST_MEDIA_AUDIO: Elements handling audio media types
+ * @GST_FACTORY_LIST_MEDIA_IMAGE: Elements handling image media types

Maybe add something for subtitles, subpictures and metadata here

@@ +61,3 @@
+  GST_FACTORY_LIST_DEPAYLOADER  = (1 << 8),
+
+  GST_FACTORY_LIST_MAX_ELEMENTS = (1 << 16),

If you want to add special purpose elements here 16 might be too small

@@ +101,3 @@
+GValueArray * gst_factory_list_get_elements (GstFactoryListType type, GstRank minrank);
+
+void          gst_factory_list_debug        (GValueArray *array);

Should this debug function really be public?

> First of all I don't think this belongs into core... There's stuff about audio
> and video in here.

Like what? It's an enum or a bunch of flags. It's not more "audio" or "video" than GST_TAG_AUDIO_CODEC.

It seems silly to scatter clearly related API all over the place just to avoid the word AUDIO in an enum.

Code like this in gstplaybin2.c

  if (!gst_factory_list_is_type (factory, GST_FACTORY_LIST_SINK))
    return GST_AUTOPLUG_SELECT_TRY;

just screams "please, make me part of the GstElementFactory API" IMHO. I'm just talking about the factory list stuff here.


> GST_FACTORY_LIST_AV_SINKS

I think AV should be expanded, and SINKS should maybe be singular to match _ENCODER etc?

Review of attachment 167259 [details] [review]:

::: gst-libs/gst/pbutils/gstfactorylists.c
@@ +71,3 @@
+  const gchar *klass;
+
+  klass = gst_element_factory_get_klass (factory);

what about appending/prepending a '/' and always searching for e.g. "/Sink" or "Sink/" to match on word boundaries?

@@ +146,3 @@
+ */
+GValueArray *
+gst_factory_list_get_elements (GstFactoryListType type, GstRank minrank)

whats the reason for using GValueArray and not the GList?

@@ +211,3 @@
+ * Merges the content of @array2 into @array1 and frees @array2.
+ * The contents of @array1 will be sorted before returning.
+ */

"... will be sorted by decreasing rank"

(In reply to comment #8)
> Review of attachment 167259 [details] [review]:
> 
> First of all I don't think this belongs into core... There's stuff about audio
> and video in here.

  Maybe not everything, but the generic code and the enums could be moved there.

> 
> Also, why don't you use a GstCaps instead of a GValueArray? That makes things a
> bit easier I guess... especially because your caps inside the value array only
> have a single, simple structure ;)

  For the caps part, I agree.

> 
> And then you might want to try to port subtitleoverlay to this. It needs a list
> of subtitle parsers/decoders and subtitle renderers.

  I'll keep that in mind when adding subtitle handling.

> 
> Other comments below
> 
> ::: gst-libs/gst/pbutils/gstfactorylists.c
> @@ +348,3 @@
> +    while (!gst_structure_foreach (st,
> +            (GstStructureForeachFunc) remove_range_foreach, st));
> +  }
> 
> For this you might want to take a look at
> http://svn.debian.org/viewsvn/pkg-gstreamer/unstable/gstreamer0.10/debian/gst-codec-info.c?revision=3298&view=markup
> 
> Search for "codec_data" and remove_min_max_fields(). I think that's a bit safer
> and better than simply removing *all* ranges.

  Agreed, but I wouldn't blindly remove the width/height/formats fields for example. They will be needed, for example, if you have several different encoders available which can only handle certain formats/sizes.

> 
> @@ +353,3 @@
> +
> +  /* Explode to individual structures */
> +  res2 = gst_caps_normalize (res);
> 
> Why is the normalizing here necessary?
> 
> @@ +376,3 @@
> +    tmpc = gst_caps_new_full (st, NULL);
> +
> +    /* Check if the caps are already present */
> 
> If you were using a GstCaps instead of a GValueArray of normalized, simple caps
> this could be done by gst_caps_merge_structure() ;)

  Yes on switching to caps instead of arrays.

> 
> ::: gst-libs/gst/pbutils/gstfactorylists.h
> @@ +40,3 @@
> + * @GST_FACTORY_LIST_MEDIA_VIDEO: Elements handling video media types
> + * @GST_FACTORY_LIST_MEDIA_AUDIO: Elements handling audio media types
> + * @GST_FACTORY_LIST_MEDIA_IMAGE: Elements handling image media types
> 
> Maybe add something for subtitles, subpictures and metadata here

  will do

> 
> @@ +61,3 @@
> +  GST_FACTORY_LIST_DEPAYLOADER  = (1 << 8),
> +
> +  GST_FACTORY_LIST_MAX_ELEMENTS = (1 << 16),
> 
> If you want to add special purpose elements here 16 might be too small

  I'll make it << 24, that should be enough.

> 
> @@ +101,3 @@
> +GValueArray * gst_factory_list_get_elements (GstFactoryListType type, GstRank
> minrank);
> +
> +void          gst_factory_list_debug        (GValueArray *array);
> 
> Should this debug function really be public?

  It was already there and is useful for code using factorylist.

(In reply to comment #10)
> Review of attachment 167259 [details] [review]:
> 
> ::: gst-libs/gst/pbutils/gstfactorylists.c
> @@ +71,3 @@
> +  const gchar *klass;
> +
> +  klass = gst_element_factory_get_klass (factory);
> 
> what about appending/prepending a '/' and always searching for e.g. "/Sink" or
> "Sink/" to match on word boundaries?

  Apart from the fact that it would be extra processing for no gain, you'd still need to do a match for "Sink" for the case where an element only has that as the klass... and that would make the other checks useless.

> 
> @@ +146,3 @@
> + */
> +GValueArray *
> +gst_factory_list_get_elements (GstFactoryListType type, GstRank minrank)
> 
> whats the reason for using GValueArray and not the GList?

  It was using GValueArray previously, I didn't see any point in switching to a GList.

> 
> @@ +211,3 @@
> + * Merges the content of @array2 into @array1 and frees @array2.
> + * The contents of @array1 will be sorted before returning.
> + */
> 
> "... will be sorted by decreasing rank"

(In reply to comment #11)

> > For this you might want to take a look at
> > http://svn.debian.org/viewsvn/pkg-gstreamer/unstable/gstreamer0.10/debian/gst-codec-info.c?revision=3298&view=markup
> > 
> > Search for "codec_data" and remove_min_max_fields(). I think that's a bit safer
> > and better than simply removing *all* ranges.
> 
>   Agreed, but I wouldn't blindly remove the width/height/formats fields for
> example. They will be needed, for example, if you have several different
> encoders available which can only handle certain formats/sizes.

Right, for width/height/depth you probably want to check if it's a range over all possible values
 
> > @@ +61,3 @@
> > +  GST_FACTORY_LIST_DEPAYLOADER  = (1 << 8),
> > +
> > +  GST_FACTORY_LIST_MAX_ELEMENTS = (1 << 16),
> > 
> > If you want to add special purpose elements here 16 might be too small
> 
>   I'll make it << 24, that should be enough.

I was more thinking of a 64 bit enum

(In reply to comment #13)
> > > @@ +61,3 @@
> > > +  GST_FACTORY_LIST_DEPAYLOADER  = (1 << 8),
> > > +
> > > +  GST_FACTORY_LIST_MAX_ELEMENTS = (1 << 16),
> > > 
> > > If you want to add special purpose elements here 16 might be too small
> > 
> >   I'll make it << 24, that should be enough.
> 
> I was more thinking of a 64 bit enum

Something like this ?

typedef guint64 GstFactoryListType;

#define  GST_FACTORY_LIST_DECODER       (1 << 0)
#define  GST_FACTORY_LIST_ENCODER       (1 << 1)
#define  GST_FACTORY_LIST_SINK          (1 << 2)
#define  GST_FACTORY_LIST_SRC           (1 << 3)
#define  GST_FACTORY_LIST_MUXER         (1 << 4)
#define  GST_FACTORY_LIST_DEMUXER       (1 << 5)
#define  GST_FACTORY_LIST_PARSER        (1 << 6)
#define  GST_FACTORY_LIST_PAYLOADER     (1 << 7)
#define  GST_FACTORY_LIST_DEPAYLOADER   (1 << 8)

#define  GST_FACTORY_LIST_MAX_ELEMENTS  ((guint64) 1 << 48)
#define  GST_FACTORY_LIST_VIDEO         ((guint64) 1 << 49)
#define  GST_FACTORY_LIST_AUDIO         ((guint64) 1 << 50)
#define  GST_FACTORY_LIST_IMAGE         ((guint64) 1 << 51)
#define  GST_FACTORY_LIST_SUBTITLE      ((guint64) 1 << 52)
#define  GST_FACTORY_LIST_METADATA      ((guint64) 1 << 53)

Or

typedef enum {
  GST_FACTORY_LIST_DECODER = (G_GUINT64_CONSTANT (1) << 0),
  ...
} GstFactoryListType;

Maybe you also want to add an ANY constant, which is ~G_GUINT64_CONSTANT (0)

(In reply to comment #15)
> Or
> 
> typedef enum {
>   GST_FACTORY_LIST_DECODER = (G_GUINT64_CONSTANT (1) << 0),
>   ...
> } GstFactoryListType;
> 
> Maybe you also want to add an ANY constant, which is ~G_GUINT64_CONSTANT (0)

Apart from the fact that some compilers will go nuts on that, it will be unwrappable by GEnumValue/GFlagValue (which is limited to gint/guint). (Try adding a item in that enum which exceeds 2**32 and admire how it fails).

That's why I proposed the 64bit version with #defines

Created attachment 167979 [details] [review]
GstElementFactory: Add listing features

This last patch is against core and only contains the GstElementFactory parts. I'm still pondering what to do with the caps list parts.

Created attachment 168034 [details] [review]
playback: Switch to gstfactorylist from core

Looks good but you say that the first flags are exclusive. As such the ANY define doesn't make sense. If anything there should be an ANY_MEDIA define, which has bit 48 and above set only.

Now there's only the question if we really want to use a GValueArray here or if some other data structure would be more appropiate. IMHO GValueArray is a bit uncomfortable to use from C :)

What are you going to do with the gst_caps_list_container_formats() and other similar functions? New patch against -base later?

(In reply to comment #20)
> Looks good but you say that the first flags are exclusive. As such the ANY
> define doesn't make sense. If anything there should be an ANY_MEDIA define,
> which has bit 48 and above set only.

Gah, good point. let me change that so that ANY is only for the lower 47 bits.

ANY_MEDIA can't be done with all higher bits set, since you might have some factories that don't have any media-specific class... and that would fall into the search with no media specified.

> 
> Now there's only the question if we really want to use a GValueArray here or if
> some other data structure would be more appropiate. IMHO GValueArray is a bit
> uncomfortable to use from C :)

  I'm fine with switching it to a GList of refcounted GstPluginFeature and share the same code as the other plugin feature list code from gstpluginfeature/gstregistry.

> 
> What are you going to do with the gst_caps_list_container_formats() and other
> similar functions? New patch against -base later?

  New patch later on, that part is still work in progress and not 100% critical imho.

  I'm cooking up a new patch.

Created attachment 168294 [details] [review]
GstElementFactory: Add listing features

Created attachment 168295 [details] [review]
GstElementFactory: Add listing features

Created attachment 168296 [details] [review]
playback: Switch to gstfactorylist from core

Looks good to me, no more comments from my side :)

(In reply to comment #12)
> (In reply to comment #10)
> > Review of attachment 167259 [details] [review] [details]:
> > 
> > ::: gst-libs/gst/pbutils/gstfactorylists.c
> > @@ +71,3 @@
> > +  const gchar *klass;
> > +
> > +  klass = gst_element_factory_get_klass (factory);
> > 
> > what about appending/prepending a '/' and always searching for e.g. "/Sink" or
> > "Sink/" to match on word boundaries?
> 
>   Apart from the fact that it would be extra processing for no gain, you'd
> still need to do a match for "Sink" for the case where an element only has that
> as the klass... and that would make the other checks useless.

I meant prepending the klass var and the matching with an additional '/'. And yes it is extra processing, but also some safety. But then, this can also be done when the first bug pops up. Was just an idea.

Created attachment 168551 [details] [review]
GstElementFactory: Add listing features

Created attachment 168552 [details] [review]
playback: Switch to gstfactorylist from core

The latest round of patches exports the pluginfeature rank/name GCompareFunc and uses it in playbin2 (in order to sort the concatenation of two lists).

commit 17f92542646a6a08b87093402e688f348f651f8d
Author: Edward Hervey <edward.hervey@collabora.co.uk>
Date:   Mon Aug 16 19:01:15 2010 +0200

    GstElementFactory: Add listing features
    
    https://bugzilla.gnome.org/show_bug.cgi?id=626181

commit 9e0358930d05c7306fb3998c280f8aafb9f3f39d
Author: Edward Hervey <edward.hervey@collabora.co.uk>
Date:   Fri Aug 6 11:53:38 2010 +0200

    playback: Switch to gstfactorylist from core
    
    https://bugzilla.gnome.org/show_bug.cgi?id=626181