If the decoder needs a parser it should specify this with its sinkpad caps and decodebin2 will the autoplug the parser if it has a high enough rank (>= MARGINAL).

I don't think this is the correct approach... and apart from that I think customizing element factory ranks should be done with some kind of configuration system, if at all, like by using GSettings or an key-value configuration file somewhere.

sebastian,
yes the decoder should specify on his sinkpad caps some needs (like format value for h264parse) by while the parser rank is > MARGINAL decodebin2 will ignore it.
What I propose is only to change the element ranking to make it plugable if needed.

I have use key-value configuration file (a GKeyFile solution).

Why is decodebin2 ignoring the parser?

number of parser element have rank set to 0 (h263parse, h264parse, jpegparse, vorbisparse, theoraparse, asfparse ...) that is normal because most of the software decoder don't need them to correctly work. This also avoid unnecessary processing on stream.

In Linaro context we manage N different hardware decoders some require parsed frames and some not. We are compiling only one ppa for all platforms.

I propose to use a system configuration file for setting default element ranking at contruction time.

Ideally this would be fixed by a) giving the parsers a sensible rank, b) making sure that they are efficient and c) that decodebin2 plugs the parsers correctly and that they can negotiate the correct output format during autoplugging.

Could you file separate bugs for this? Of course this doesn't mean that this patch here is wrong but I still don't think it's the solution for your problem :)

Review of attachment 187346 [details] [review]:

First a general comment, I would prefer a generic "configuration" system over something that is specialized to changing ranks of elements. I'll still add some comments about the current implementation though, maybe others disagree and would like to have a special rank configuration system :)

::: docs/design/draft-customizeranking.txt
@@ +2,3 @@
+--------------------
+
+Base class to change the default element ranking by a custom one.

Not really a base class but that's nitpicking ;)

::: gst/Makefile.am
@@ +17,3 @@
+else
+GST_CUSTOMIZE_RANKING_SRC = gstcustomizeranking.c
+endif

You're not adding the header here, also instead of not including the API when disabled you should make the API just do nothing. There are only few things that are worse than an API that changes depending on configure parameters ;)

Also include the header from gst.h

::: gst/gstcustomizeranking.c
@@ +39,3 @@
+G_DEFINE_TYPE (GstCustomizeRankingEntry, gst_customize_ranking_entry,
+    GST_TYPE_OBJECT);
+static GstObjectClass *entry_parent_class = NULL;

G_DEFINE_TYPE does this already, but as gst_customize_ranking_entry_parent_class. It also sets the pointer in class_init already

@@ +121,3 @@
+/* customize ranking functions */
+G_DEFINE_TYPE (GstCustomizeRanking, gst_customize_ranking, GST_TYPE_OBJECT);
+static GstObjectClass *parent_class = NULL;

Same comment as above for G_DEFINE_TYPE

@@ +290,3 @@
+  if (custom->file == NULL) {
+    custom->file = g_build_filename (g_get_home_dir (),
+        ".gstreamer-" GST_MAJORMINOR, "custom_ranking." HOST_CPU ".bin", NULL);

For 0.10 this is fine but for 0.11 please use g_get_system_config_dirs() in combination with g_get_user_config_dir().

::: gst/gstcustomizeranking.h
@@ +26,3 @@
+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#endif

Don't do this in public headers, it would require that code using gstreamer also has configure checks for unistd.h, uses the same #define for this, etc. Stuff from config.h should only ever be used in source files, not headers  (also you're not including config.h here but that wouldn't make it better)

@@ +55,3 @@
+	/* private fiels */
+	gchar *name;
+	gint rank;

Why not make this a GstMiniObject for example? Or a plain struct? Also, please add padding to this and the class struct

@@ +66,3 @@
+
+void gst_customize_ranking_entry_set_name (GstCustomizeRankingEntry * entry, gchar *name);
+void gst_customize_ranking_entry_set_rank (GstCustomizeRankingEntry * entry, gint rank);

Instead of these two functions maybe just a constructor that sets both fields? Also you have no constructor at all

@@ +85,3 @@
+	GKeyFile *key_file;
+	gchar *file;
+	GList *entries;

Padding and really GstObject? What about GObject here, I don't think you need floating refs and all that

::: tools/gst-rank.c
@@ +31,3 @@
+#include <glib/gprintf.h>
+
+#include "gst/gstcustomizeranking.h"

<gst/gstcustomizeranking.h> or even better <gst/gst.h>

thank for your review sebastian.

I have used your remarks to fix and simply the code.
I have check padding of the 2 structuress are multiple of 4.

customization API isn't exported/used outside gstregistry (and new gst-rank tool) so I have keep the code under GST_DISABLE_CUSTOMIZE_RANKING compilation flag (like it is done for gstregistrybinary)

Created attachment 187499 [details] [review]
draft of implementation

fix some of sebastian's remarks

> First a general comment, I would prefer a generic "configuration" system over
> something that is specialized to changing ranks of elements. 

Same here. This would require some more thought about what else should be configurable though.

I am against adding any kind of rank adjustment system until after we fix the ranks.

At some point, I had a patch that read rank adjustment information from an environment variable.  I stopped using it because it adjusted ranks when *building* the registry rather than *reading* the registry, but otherwise it worked quite well.

IMO, ranks for decoders and parsers should be adjusted as follows (starting from PRIMARY, assuming they're PRIMARY quality):

  -10  all parsers
  -20  decoders without built-in parsers
  +2   decoders from an external source (because you had to go and get them, thus you presumably want to use them)
  +4   decoders that you paid for (same, but even more so)

Agreed, something like your concept should be implemented in 0.11 for the ranks. But why do you give parsers a lower rank than third party decoders? This is only for third party decoders without builtin parsers, right?


And for this bug here, I guess it's correct to say that an approach like the one taken in the attached patch will not be accepted. In general we would prefer a real configuration system (concept needed) and for rank adjustments there also should be a concept for ranks that is more elaborated than what we have now ;)

We would also need to consider how optimal an element is. As an example I could (hypothetically) have:

  ffdec_h264, glshaderh264dec, vdpauh264dec

So essentially vdpau decoder is expected to be more optimal then generic shader, which is also considered more optimal then software ffmpeg decoder.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/23.