[gstreamer-bugs] [Bug 637735] [encoding-profile] automatic load/save support and registry

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=637735
  GStreamer | gst-plugins-base | git

--- Comment #5 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-12-31 15:13:32 UTC ---
Are you using this already somewhere? It still feels a bit unpolished somehow,
but can't quite put my finger on it :)

Couple of comments:

 - (existing header): the GST_ENCODING_CATEGORY_* defines
   need a comment block explaining a bit more what they're
   meant to be used for (one or two examples)

 - any API that takes a category_name argument should reference
   one of the GST_ENCODING_CATEGORY_* defines in the gtk-doc
   blurb as example

 - new gst_encoding_target_get_profile() API: couldn't it happen
   that there are profiles with the same name under different
   categories?

 - as an example of what I mean by "unpolished", see the code
   example in the gtk-doc comment which is modified in the
   "encoding-target: Add method to get a profile by name"
   commit: it just seems an odd way to be using this API in the
   first place (very narrowly defined). I would expect almost all
   applications to want to get a list of targets, either for all
   categories or for one specific category, and then expose
   that somehow in a selector, no?

 - there's no give-me-a-list-of-all-targets API, which would be
    the most interesting API here

 - another reason why it seems unpolished is maybe the
   implicitness of the registry stuff - you don't have a
   central object or API to query for targets, that then
   also handles the filesystem stuff, it's all hidden in the
   API. The filesystem is the registry, sorta. Which is fine
   I guess, but we end up having gst_encoding_{profile,target}_*()
   API that's not linked to a profile/target object, which
   easily feels unclean if it's too much :)

 - I think the datadir sub-directory should be called "encoding-profiles"
   (ie. plural)

 - don't we need some kind of environment-variable override, or
   way to allow additional paths, e.g. for an uninstalled setup?

 - not a fan of the ".gstprofile" suffix, should be either something
   with three letters (.gep? .dat?) or just ".profile" IMHO. The path
   already takes care of the 'gst' thing.

 - the description for gst_encoding_target_save() says it will try
   to save in the system directory, but doesn't actually (not sure
   it should either)

 - wonder if load_from/save_to should also have 'file' in the
   function name (it's not clear if path is a directory here or
   a filepath/name); also, people will later likely ask for
   saving to and loading from memory/data instead of files.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.