[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 #7 from Edward Hervey <bilboed at gmail.com> 2011-01-03 18:20:02 UTC ---
(In reply to comment #5)
> 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)

  Added docs locally.

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

  Added docs locally

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

  Yes, but here you're asking a profile from a specific target... which can not
contain more than one profile with the same name.

> 
>  - 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?

  Updated the example accordingly.

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

  Add two new methods:
  gst_encoding_target_list_all
  gst_encoding_target_list_available_categories

> 
>  - 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 :)

  Don't see why that would make it 'unpolished'. It could definitely be
improved later on, with an extra set of API which takes a 'central' registry
object for performance reasons.
  I just went straight to the convenience methods. The addition of the two
methods above filled the missing gap.

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

  Changed.

> 
>  - 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.

  Changed to .gep

> 
>  - 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)

  I'll remove the doc part about saving to system directory.

> 
>  - 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.

  Then we will likely later add methods for that if the need arises. Doesn't
feel like something important.

(In reply to comment #6)
> Sorry, forgot one:
> 
> > "encoding-profile: Add convenience method to find a profileprofileregistry
> > Also register GValue functions to be able to use profiles in gst-launch"
> 
>  - it's cool to be able to use it from gst-launch I think, and
>    it seems ok to abuse the serialisation function here for
>    that trick even though we don't actually serialise the
>    object itself, but I'm not sure if we should expose
>    gst_encoding_profile_find() with the single profilename
>    argument where the caller should pass "target/profile-name" -
>    wouldn't it be more natural to have two arguments for this and
>    let the deserialisation function do the splitting in two?

  Changed to take two arguments.

> 
>  - I think the argument name in the gtk-doc blurb is different
>    than in the function declaration and/or definition

  fixed


Pushed all updates to my personal git repo

-- 
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.