[gstreamer-bugs] [Bug 627476] New profile library and encoding plugin

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #22 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-12-10 14:47:27 UTC ---
This looks pretty neat, looking forward to getting this in!

Random thoughts when looking at the API, maybe people have an opinion one way
or another as well:

 -  shouldn't header be called encoding-profile.h or
     gstencodingprofile.h ?

 -  why GstMiniObjects? (I know slomo suggested them, sorry).
    But why not plain old GObjects/GstObjects with properties,
    so that all these constructors just wrap g_object_new()?
    Also allows for floating refs then.

 -  gst_encoding_profile_add_stream() takes ownership of
    stream, but gst_encoding_target_add_profile() doesn't?
    Documented, but still seems inconsistent. Is there a
    reason for this? (Because typically you would add the
    same profile to multiple targets when building your
    target collection?)

 -  are they _copy() functions used anywhere / good for
    anything? (what for? why is ref not enough? Aren't
    the profiles immutable?)

 - GstEncodingProfile kind of doubles as baseclass
   (name/desc/profile/caps) and as some sort of
   GstContainerEncodingProfile

 - what's the reason multipass is a property on the
    encoding profile and not just on video stream
    profile?

 - should it maybe be Gst{Video,Audio,etc.}StreamEncodingProfile?
   (even if that's a bit unwieldy)

 - wonder if the presence parameter should be
   defines (if not, maybe there should at least be
   some define for 0, to make code more readable);
   also: do we need to differentiate between
   'exactly N' and 'max. N' here?

 - not sure about the extra properties for the stream
   profiles (pass, variableframerate, multipass), think
   it might be nicer code-wise and more extensible if
   there was just a flag parameter and we had flag
   defines for these things, compare e.g.:

    gst_video_encoding_profile_new(caps, NULL, NULL, 0, 1, TRUE);

    gst_video_encoding_profile_new(caps, NULL, NULL, 
        GST_STREAM_ENCODING_PROFILE_PRESENCE_ANY,
        GST_VIDEO_STREAM_ENCODING_FLAG_PASS1 |
        GST_VIDEO_STREAM_ENCODING_FLAG_VARIABLE_FRAME_RATE) 

    (ok, maybe not that much prettier, but at least you
    can read the code and know what it means without
    looking up things in the API reference).

 - why is 'pass' part of the video encoding profile? How
    does this work for 2-pass encoding? Would one create
    two video stream encoding profiles, one for pass 1 and
   one for pass 2? (If so, why is this needed?)

 - do we really need the profile type enum? Isn't the GType
   good enough? It's not like this is performance-critical?
   (maybe the profile should have different API for, and keep
   separate GLists for the different types?)

 - do we want accessor functions?

 - is an audio equivalent of variableframerate needed?
    (e.g. where the encoder/container don't store
    timestamps and we really want an audiorate in front
    of he encoder, or do we always do that anyway)

 - maybe specify profile 'name' scheme more clearly in docs
   e.g.: allowed are only lower-case ASCII letters and hyphens

 - should the target load/save function have a GError * argument?

 - what are the default locations for targets?

 - is it one keyfile per target? where do those files
   come from in future? Will there be some kind of
   target registry/cache in future etc?

 - is there / will there be a way to list available targets?

 - could unify save/load by using path == NULL for
   'default location' (is the path here a directory or file?)
   (but separate functions are good too and maybe nicer API)

 - (maybe leave out loading/saving for now if there
    are open issues?)

 - would it make sense to also have some kind of
   _is_available() for a profile? (we would then check
   the registry for suitable elements and check if the
   required preset is available for one of them)

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