After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 649542 - Configuration system (for element ranks and other things)
Configuration system (for element ranks and other things)
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-05-06 08:48 UTC by Benjamin Gaignard
Modified: 2018-11-03 12:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
draft of implementation (22.89 KB, patch)
2011-05-06 08:48 UTC, Benjamin Gaignard
needs-work Details | Review
draft of implementation (18.89 KB, patch)
2011-05-09 11:32 UTC, Benjamin Gaignard
rejected Details | Review

Description Benjamin Gaignard 2011-05-06 08:48:02 UTC
Created attachment 187346 [details] [review]
draft of implementation

The idea is to offer a way to customize element ranking without hacking/patch code. 
I need that feature because some decoders (like h264) may request to autoplug a parser between them and the demuxer. Today ins't possible without hack parser code.

This could be done by modify the element rank before write it into cache registry.
Modifications are store in a file (in a humain readable format) and I add a gst tool, named gst-rank, to edit this file.

All comments/review/help is welcome
Comment 1 Sebastian Dröge (slomo) 2011-05-06 11:12:00 UTC
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.
Comment 2 Benjamin Gaignard 2011-05-06 12:30:56 UTC
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).
Comment 3 Sebastian Dröge (slomo) 2011-05-06 12:44:45 UTC
Why is decodebin2 ignoring the parser?
Comment 4 Benjamin Gaignard 2011-05-06 14:35:56 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2011-05-09 07:34:00 UTC
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 :)
Comment 6 Sebastian Dröge (slomo) 2011-05-09 07:46:35 UTC
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>
Comment 7 Benjamin Gaignard 2011-05-09 11:30:58 UTC
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)
Comment 8 Benjamin Gaignard 2011-05-09 11:32:25 UTC
Created attachment 187499 [details] [review]
draft of implementation

fix some of sebastian's remarks
Comment 9 Tim-Philipp Müller 2011-05-09 11:41:10 UTC
> 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.
Comment 10 David Schleef 2011-05-10 21:27:18 UTC
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)
Comment 11 Sebastian Dröge (slomo) 2011-05-11 07:35:21 UTC
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 ;)
Comment 12 Nicolas Dufresne (ndufresne) 2011-11-08 19:28:47 UTC
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.
Comment 13 GStreamer system administrator 2018-11-03 12:15:18 UTC
-- 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.