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

Fri Dec 10 10:30:21 PST 2010

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

--- Comment #23 from Edward Hervey <bilboed at gmail.com> 2010-12-10 18:30:16 UTC ---
(In reply to comment #22)
> 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 ?

  Could do, yes. gstprofile is a bit too generic.

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

  Seems a bit overkill for such dumb structures that don't *do* anything. They
are just containers of information. Having the refcounting is the nice bonus
(which gstminiobject gives), but having 'heavy' GProperties for them seems a
bit overkill, no ?

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

  No there wasn't any reason why _add_profile() shouldn't steal the ref.
  I'll fix that to be the same as _add_stream().

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

  leftover from pre-miniobject API. Will remove them.

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

  Very good point.

  By having the following 'hierarchy':

ClassName                      | properties
--------------------------------------------------------------
  GstEncodingProfile             format/preset/name/description/presence
   GstEncodingContainerProfile    +streams
   GstEncodingAudioProfile        +restriction
   GstEncodingVideoProfile        +restriction/pass/variableframerate

  We could have the benefit of (in the future, once implemented in encodebin):
  * Handling single-stream encoding
  * Handling container-in-container encoding (DV in MXF for ex)

  While still only have to pass a 'GstEncodingProfile' to most
functions/methods/elements.

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

  To make it faster to figure out if the profile requires a multipass setup (in
encodebin).

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

  Or the naming proposal above (the one with classname/properties) ? Keeps the
GstEncoding part always at the beginning that way.

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

  ok for the PRESENCE_ANY define

  I'm not that thrilled about making them all flags though.

  What I could do is the following:
  * make gst_video_encoding_profile_new not take the extra args.
    Those pass/vfr/multipass args would have sane defaults.
  * add getters/setters to get/set those properties.

  Like that you end up in most of the cases not caring about those extra args,
and if you need it's trivial.

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

  Because you need to set different properties (and ergo presets) depending on
whether it's the first or second pass.

> 
>  - do we really need the profile type enum? Isn't the GType
>    good enough? It's not like this is performance-critical?

  That would require creating other types for the subpicture and image formats.
There's no performance critical issue with that :)

>    (maybe the profile should have different API for, and keep
>    separate GLists for the different types?)

  I don't understand what you mean.

> 
>  - do we want accessor functions?

  yes yes yes yes, I'll seal all fields and add setters/getters.

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

  It's done unconditionally if encoding is needed. I don't see the usage for
not having it always enabled.

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

  Will add that indeed

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

  Yes, will add it.

> 
>  - what are the default locations for targets?

  None right now. The idea was to have locations in the same style as GstPreset
(which I shamingly based most of the encodingtarget code from):
  ${prefix}/share/gstreamer-0.10/encoding-profiles/
  ${home}/.gstreamer-0.10/encoding-profiles/

> 
>  - is it one keyfile per target?

  Yes

> where do those files
>    come from in future?

  They can either (in the future) come from the locations mentionned above, but
you could also have app-private profile files.

> Will there be some kind of
>    target registry/cache in future etc?

  We could do one, yes. But I'm not sure how much benefit we would gain from
it.

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

  Yes, I didn't put it in the API because the global file handling (i.e.
without specifying a specifi path) isn't implemented yet.

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

  The path is a file.

  The versions without _to/_from are the ones that will save to the paths
mentionned a bit further up (based on write access).

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

  I will not merge the save/load variants without explicit paths until they are
implemented.
  I will be merging the ones with specific paths since they already bring some
functionnality to applications that want to store profiles and do the file
handling themselves.

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

  _is_available() in the sense "do I have the GstElement(s) required ? Yah, I
could make that code available (it's already used in encodebin and could be
refactored easily).

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