GNOME Bugzilla – Bug 627476
New profile library and encoding plugin
Last modified: 2010-12-21 11:13:01 UTC
This is a backport of the gstprofile and encoding plugin from gst-convenience.
Created attachment 168378 [details] [review] pbutils: New Profile library
Created attachment 168379 [details] [review] gst: New encoding plugin
Also available in the 'profile' branch here : http://cgit.freedesktop.org/~bilboed/gst-plugins-base/
Looks like I forgot to remove some dead code. Will do that in the next iteration.
Review of attachment 168378 [details] [review]: ::: gst-libs/gst/pbutils/gstprofile.c @@ +125,3 @@ + /* FIXME : Free stream profiles */ + g_list_foreach (prof->encodingprofiles, + (GFunc) gst_stream_encoding_profile_free, NULL); This will work better if the stream profiles are mini objects... that could all be unreffed that same way @@ +153,3 @@ + GstEncodingProfile *prof; + + prof = g_new0 (GstEncodingProfile, 1); GSlice unless you use miniobjects @@ +157,3 @@ + prof->name = g_strdup (name); + if (format) + prof->format = gst_caps_copy (format); Maybe only ref the caps? Otherwise make the parameter const @@ +186,3 @@ + } + /* FIXME : Maybe we should have a better way to detect if an exactly + * similar profile is already present */ Add a compare vfunc maybe... or compare by caps @@ +214,3 @@ + for (tmp = profile->encodingprofiles; tmp; tmp = g_list_next (tmp)) { + GstStreamEncodingProfile *sprof = (GstStreamEncodingProfile *) tmp->data; + gst_caps_append (res, gst_stream_encoding_profile_get_output_caps (sprof)); gst_caps_merge() might be better @@ +250,3 @@ + prof = g_new0 (GstStreamEncodingProfile, 1); + prof->type = type; + prof->format = gst_caps_copy (format); Maybe just ref here... and const parameters ::: gst-libs/gst/pbutils/gstprofile.h @@ +43,3 @@ + * @GST_ENCODING_PROFILE_AUDIO: audio stream + * @GST_ENCODING_PROFILE_TEXT: text/subtitle stream + * @GST_ENCODING_PROFILE_IMAGE: image How are subpictures handled? What's the difference between image and video except that image has a fixed 0/1 framerate? @@ +77,3 @@ + gchar *name; + gchar *category; + GList *profiles; Struct padding missing here and everywhere else @@ +95,3 @@ + GstCaps *format; + gchar *preset; + gboolean multipass; You could make a generic GstEncodingProfileBase and let the others inherit from this one. GstStreamEncodingProfile shares all but one field with GstEncodingProfile. @@ +109,3 @@ + * Invididual stream profile, to be used in #GstEncodingProfile + */ +struct _GstStreamEncodingProfile { Use GstMiniObjects here maybe :) @@ +138,3 @@ +GType gst_encoding_profile_get_type (void); +GstEncodingProfile * gst_encoding_profile_new (gchar *name, GstCaps *format, + gchar *preset, Mark the char * as const and copy internally (you probably already do that) @@ +140,3 @@ + gchar *preset, + gboolean multipass); +GstEncodingProfile * gst_encoding_profile_copy (GstEncodingProfile *profile); const here too and for every other copy function and the char * @@ +179,3 @@ + +/* + * Application convenience methods (possibly to be added in gst-pb-utils) That's what you're doing here, drop the comment ;) Or do you mean into another header? @@ +184,3 @@ +GstElement *gst_pb_utils_create_encoder(GstCaps *caps, gchar *preset, gchar *name); +GstElement *gst_pb_utils_create_encoder_format(gchar *format, gchar *preset, + gchar *name); const for the parameters again and for the other similar functions @@ +192,3 @@ + +/* + * GstPreset modifications ...and move this to core, with a separate bugreport and patch ::: gst-libs/gst/pbutils/gstprofileutils.c @@ +1,1 @@ +/* GStreamer encoding profiles library utilities And nothing is implemented here yet ;)
Created attachment 168420 [details] [review] pbutils: New Profile library
Created attachment 168421 [details] [review] gst: New encoding plugin
Latest version contains switch to miniobject, padding, const fixes and removal of all unimplemented code (I'll tackle the registry part in a week or so)
Could you provide a high-level overview / documentation for the profile API? Here in the bug and in the gtk-doc chunk in the library code. This is a *lot* of code and API and some guidance would be helpful when looking at it. - what is it trying to solve? (big picture, use-cases) - how is it supposed to be used? (examples for one or two use cases) - what's GstStreamEncodingProfile vs. GstEncodingProfile ? (EncodingProfile is the container and StreamEncodingProfile is audio/video/subs?) - pbutils/Makefile.am seems to remove missing-plugins.h from headers
Whilst I suspect that these comments could also apply to bug 626550, I had a quick chat with Christophe Fergeau on how this might integrate with media-player-info. My thoughts were that it would be ideal if one could * Get a list of acceptable encoding profiles from device media-player-info files * Find the encoding profile from the profile library/registry * Pas the profile/profile name to EncodeBin * Finished
(In reply to comment #10) > Whilst I suspect that these comments could also apply to bug 626550, I had a > quick chat with Christophe Fergeau on how this might integrate with > media-player-info. > > My thoughts were that it would be ideal if one could > * Get a list of acceptable encoding profiles from device media-player-info > files > * Find the encoding profile from the profile library/registry > * Pas the profile/profile name to EncodeBin > * Finished The profile registry is still work in progress alas. In my git branch I've already got saving/loading of profiles to/from files, the next step I need to complete is having some form of registry (but much more lightweight than gstregistry). The overall concept is: * Profile file contains one GstEncodingTarget * GstEncodingTarget has a name ("my pony device 2000"), a well-known category ("Devices") * An encoding target can then have multiple GstEncodingProfiles. Those are the missing part for the first 2 items. The last 2 items are already functional. Another interesting point I'm not sure how to solve is how to ease the media-player-info-device-id to target mapping. Have media-player-info provide those and know about the mapping ? Allow generic metadata fields to be added to GstEncodingTarget ?
> > Another interesting point I'm not sure how to solve is how to ease the > media-player-info-device-id to target mapping. Have media-player-info provide > those and know about the mapping ? Allow generic metadata fields to be added to > GstEncodingTarget ? just so we are clear on terms; - m-p-i = media-player-info - ID_MEDIA_PLAYER be the device id added by udev (via m-p-i). - m-p-i store files in /usr/share/media-player-info/ID_MEDIA_PLAYER I don't necessarily think there needs to be a mapping. The developer could parse the media-player-info file for the device, it will contain a section like [Encoding] GstEncodingProfile=foo;bar;baz The developer can the go an look at the profile file in /usr/share/gstreamer-encoding-profiles/{foo,bar,baz) Of course this requires that those m-p-i files that want to encode all be modified. Another alternative might be putting some default files for common devices in /usr/share/gstreamer-encoding/profiles/$ID_MEDIA_PLAYER but I don't think that is any cleaner, since at least in the case of ipod, the single m-p-i file (hence single ID_MEDIA_PLAYER key) describes multiple devices that also support many encoding
m-p-i files are mainly used for usb mass storage devices for now, and I'm not sure how practical it would be to have such files for iPods/iOS devices/MTP devices/... I was thinking that an additional key could be added to udev database (something like ID_MEDIA_PLAYER_ENCODING_PROFILE) which would be used to find the appropriate profile, and if it's not set, it could fallback to using ID_MEDIA_PLAYER_ID. But it's indeeed less expressive than having this GstEncodingProfile key, and I don't know if this would work well for mtp device (I think libmtp has an udev rule for mtp devices but I forgot what it does). Another data point that might or might not be helpful is that iPods can describe the various audio/video codecs they support, as well as the resolution, ..., don't hesitate to ask for more information if you think you could reuse this data.
We're going to be using encodebin in gupnp-dlna to help with MediaServers that want to provide trancoding support (i.e., Rygel). Just as quick background, the DLNA specification provides a set of "profiles" that roughly correspond to a set of constraints on codec, profile, level, bitrate, resolution, etc. that a given device can play, or a given file conforms to. The idea is to have Rygel offer streams transcoded to match certain (probably arbitrary) profiles. Our internal representation of these DLNA profiles is a superset of GstEncodingProfiles. encodebin makes things a lot simpler for us (the current implementation being to implement one transcoder pipeline for each profile). API-wise, gupnp-dlna offers an API that, given a DLNA profile, returns a GstEncodingProfile that can be used to encode a stream to that profile. Rygel will feed this GstEncodingProfile to encodebin and we get automagic transcoding. Now there are some things that we need to consider: * Format vs. raw restrictions: GstStreamEncodingProfiles have separate format caps (e.g. audio/mpeg, mpegversion=4) and raw caps (e.g. audio/x-raw-int, rate=44100, ...). As things happen just putting everything in format caps will (or at least should) work. I don't really want to have gupnp-dlna rely on this incidental behaviour though. We /could/ add some filtering code in gupnp-dlna to split out some standard raw caps fields, but this could be a problem that affects other clients, so a generic solution would be better. * Presets and properties: This is really a larger problem, but since we also need to be able to control the profile and bitrate of the encoded stream, we need to use the GstPreset API. The main problem here is that since we don't know a-priori what encoder will be used for a given codec, and what properties that encoder will expose, we need to work around this by adding some sort of configuration to provide this information (something like a map of codec caps -> (element name, list of properties to populate, bitrate_is_in_bps, ...). While some of this could go away in the future with the addition of a GstEncoder baseclass or interface as suggested in bug #626550, we still need to handle these problems in gupnp-dlna till a leaner way to do this appears. * Persistence: Since the constraints on a DLNA profile might be updated, we don't want to make our encoding profile consistent. I don't know if there are any other types of applications that could make use of a non-persistent registry, but having one would make things cleaner in a sense. gupnp-dlna would register a bunch of encoding profiles keyed by the DLNA profile name, and the app would just use that. This isn't a huge deal for us, so if there aren't any other apps that work like this, we're fine with having our own API to get encoding profiles.
(In reply to comment #14) > > Now there are some things that we need to consider: > > * Format vs. raw restrictions: GstStreamEncodingProfiles have separate format > caps (e.g. audio/mpeg, mpegversion=4) and raw caps (e.g. audio/x-raw-int, > rate=44100, ...). As things happen just putting everything in format caps > will (or at least should) work. I don't really want to have gupnp-dlna rely > on this incidental behaviour though. We /could/ add some filtering code in > gupnp-dlna to split out some standard raw caps fields, but this could be a > problem that affects other clients, so a generic solution would be better. Only putting the restrictions in the format caps relies on the encoders being able to properly convey that information upstream via get_caps and check it can use them with set_caps. So yes... it should work, but I've seen encoders that don't handle that. You're also relying on the encoders forwarding *all* restriction fields (and not just the obvious width/height/framerate, but also pixel-aspect-ratio for ex). To sum up : if you trust the encoders you can ignore the restriction field, if not, you'll need to split up. > > * Presets and properties: This is really a larger problem, but since we also > need to be able to control the profile and bitrate of the encoded stream, we > need to use the GstPreset API. The main problem here is that since we don't > know a-priori what encoder will be used for a given codec, and what > properties that encoder will expose, we need to work around this by adding > some sort of configuration to provide this information (something like a map > of codec caps -> (element name, list of properties to populate, > bitrate_is_in_bps, ...). While some of this could go away in the future with > the addition of a GstEncoder baseclass or interface as suggested in bug > #626550, we still need to handle these problems in gupnp-dlna till a leaner > way to do this appears. I tried to came up with the least intrusive way of describing encoded stream properties and encoder settings (we want this API to be usable without requiring as much knowledge/pain that it's barely more useful than doing everything yourself from scratch). Yes, GstEncoder base classes would definitely be a good way towards solving this issue. The least-intrusive way I found means having presets for various encoders (if needed) where you set the various settings, save those presets, and then specify them in the profile. This means writing presets, sure. But it would be a good way to have a more extensive collection of presets for various encoders/muxers. The other thing that could be improved in elements, is to have more format caps. Ex : for having a h263 profile 0 level 45 stream, have the encoders be able to configure themselves properly if downstream caps are : video/x-h263,profile=0,level=45. Then the only thing remaining to set would be the tweaking of 'how' the stream is encoded. > > * Persistence: Since the constraints on a DLNA profile might be updated, we > don't want to make our encoding profile consistent. I don't know if there > are any other types of applications that could make use of a non-persistent > registry, but having one would make things cleaner in a sense. gupnp-dlna > would register a bunch of encoding profiles keyed by the DLNA profile name, > and the app would just use that. This isn't a huge deal for us, so if there > aren't any other apps that work like this, we're fine with having our own > API to get encoding profiles. I still need to finish the profile registry, but (de)serialization of encoding profiles is already working in my profileregistry branch. This allows you to handle your own collection of profiles without exposing them system-wide. That being said... you could install them system-wide with a proper encoding category, like "UPNP-DLNA". Maybe some other apps would benefit from having those profiles.
Pushed the design/research docs to my profileregistry branch in docs/design/design-encoding.txt This should help understand the decisions that have been made for the API
I've pushed the gtk-doc generated API page for gstprofile here for easier review : http://people.collabora.co.uk/~edward/gstprofile/libs/gst-plugins-base-libs-gstprofile.html
If you have a simple profile that only takes a couple of lines, it might be helpful to have that included as an example in the description part of the docs.
Just for reference the branch with the latest changes (as the patches don't apply anymore): http://cgit.freedesktop.org/~bilboed/gst-plugins-base/tree/?h=profile Thanks for adding the examples: * In the "Loading a profile from disk" example it is not obvious where the "profilename" comes from. Should apps scan a particular dir for available profiles? Also see my comment on gst_encoding_target_load/gst_encoding_target_save below. A small ambiguity is that in the example you call the gst_encoding_target_load_from() with a parameter called uir, where in the API docs you refer to it as a path. Would be good to fix either one. Regarding the API: * gst_video_encoding_profile_new() the 'vfr' parameter is not docuemnted and I wonder if you could perhaps spell it out. The parameter is documented in GstVideoEncodingProfile. * if there is a gst_stream_encoding_profile_new() and gst_video_encoding_profile_new(), shouldn't there be a gst_audio_encoding_profile_new() too? Or is it so that right now GstAudioEncodingProfile==GstVideoEncodingProfile (aka no specific parameters for audio)? So basically the only open Item for me is, how an application would look up available targets. * gst_encoding_target_load/gst_encoding_target_save is not implemented. I believe specifying the locations (like done for GstPreset) would make sense.
(In reply to comment #19) > Just for reference the branch with the latest changes (as the patches don't > apply anymore): > http://cgit.freedesktop.org/~bilboed/gst-plugins-base/tree/?h=profile > > Thanks for adding the examples: > * In the "Loading a profile from disk" example it is not obvious where the > "profilename" comes from. Should apps scan a particular dir for available > profiles? Also see my comment on > gst_encoding_target_load/gst_encoding_target_save below. That was just an example for getting a specific profile from a target. You can also just load a target and iterate the available profiles. > > A small ambiguity is that in the example you call the > gst_encoding_target_load_from() with a parameter called uir, where in the API > docs you refer to it as a path. Would be good to fix either one. fixed > > Regarding the API: > > * gst_video_encoding_profile_new() > the 'vfr' parameter is not docuemnted and I wonder if you could perhaps spell > it out. The parameter is documented in GstVideoEncodingProfile. I renamed it to variableframerate. Long... but explanatory. > > * if there is a gst_stream_encoding_profile_new() and > gst_video_encoding_profile_new(), shouldn't there be a > gst_audio_encoding_profile_new() too? Or is it so that right now > GstAudioEncodingProfile==GstVideoEncodingProfile (aka no specific parameters > for audio)? I did a separate constructor for video since there are extra fields (passes). > > So basically the only open Item for me is, how an application would look up > available targets. > > * gst_encoding_target_load/gst_encoding_target_save is not implemented. I > believe specifying the locations (like done for GstPreset) would make sense.
Comment on attachment 168421 [details] [review] gst: New encoding plugin Please follow the git branch and not the patches.
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)
(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).
> > [GstMiniObject vs. GObject] > > 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 ? Maybe. Both work for me. Just thought full objects would give us floating refs in addition, and make things easier for bindings (basically constructors = g_object_newv()). No strong opinion either way. > > - 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). Couldn't that also (better?) be done with additional API like gst_encoding_profile_is_multipass (profile) ? Which could iterate the streams and check if multipass flag is set on any of them. Again, no strong opinion, just wondering. > [pass/vfr/etc. option args] > > 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. That sounds like a good idea to me. Means you can't do something like gst_encoding_profile_add_stream (profile, gst_video_encoding_profile_new (..)); any more for those cases, but that's not really a big loss. > > - why is 'pass' part of the video encoding profile? (...) > > Because you need to set different properties (and ergo presets) depending on > whether it's the first or second pass. Ah right, forgot we only standardised the "multipass-cache-file" property, but not the other properties. > > - 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 :) I think I'd create dedicated GTypes for all stream types anyway, and if it's just for consistency (whether we keep the enum or not). Are you/is anyone using the image/subtitle types yet btw? (rygel?) > > (maybe the profile should have different API for, and keep > > separate GLists for the different types?) > > I don't understand what you mean. This was in connection with the profile enum type question. I meant that if the profile kept audio/video/xyz streams in separate lists in the profile, then it'd be easier to get all streams for a certain type (you just hardcode that via the API then instead of passing an enum), if that was the goal. (= nevermind) > > - is an audio equivalent of variableframerate needed? > > It's done unconditionally if encoding is needed. I don't see the usage for > not having it always enabled. Agreed. > > 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. I meant: is there a plan to have a dedicated profiles repository/module where targets are collected? What package will install them in these locations? (This just out of curiosity, not really related to the code). > > - 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). I was wondering what happens if you have a suitable encoder for a stream profile, but the required/specified preset isn't available. So two checks seem necessary to me: (a) encoder element, (b) profile availability. (This is also not directly related to this patch of course.)
Also had a quick look at encodebin and the other elements: - a gtk-doc chunk/section with some feature/usage details would be nice - gstencodebin.h: leftover constructor declaration: gst_encode_bin_new () - gstencodebin.h: could be made static gst_encode_bin_set_profile () - src pad: is there a reason the src pad is a sometimes pad and created in response to g_object_set()ing the profile; would it be possible to make it an always pad (which may or may not have a target to start with), so people can just link it right away without worrying about the sometimes-pad aspect? - "use-smartencoder" property - a name derived from the feature/function and not the implementation detail might be nicer, if anyone can think of one - the "request-pad" action signal is a bit confusing - how does it work together with the normal request pad procedure? Does it need to be released again? Maybe we should instead add some kind of new gst_element_request_pad_for_caps() API to core/GstElementClass instead? - not sure about the labelling of unknown/other streams as 'private' - when it's really just unknown/other - docs/design/design-encoding.txt code example refers to "profile" property of string type: g_object_set (encbin, "profile", "N900/H264 HQ", NULL), whereas it seems to be a mini object now - "smartencoder" - can anyone think of a better name for this? ("smart" doesn't really tell anyone anything unless they already know what it does exactly) :) - smartencoder: should post an error with GST_ELEMENT_ERROR when returning GST_FLOW_ERROR - streamsplitter: should post an error with GST_ELEMENT_ERROR when returning GST_FLOW_ERROR - a small section in the design docs how streamsplitter/combiner + smartencoder work together would be nice - are streamsplitter/combiner/smartencoder useful outside of encodebin or should they maybe be kept private for now?
(In reply to comment #25) > Also had a quick look at encodebin and the other elements: > > - a gtk-doc chunk/section with some feature/usage details would be nice > > - gstencodebin.h: leftover constructor declaration: gst_encode_bin_new () > > - gstencodebin.h: could be made static gst_encode_bin_set_profile () Ok to above. > > - src pad: is there a reason the src pad is a sometimes pad and > created in response to g_object_set()ing the profile; would it be > possible to make it an always pad (which may or may not have a > target to start with), so people can just link it right away without > worrying about the sometimes-pad aspect? I'll try to figure that out, makes sense. > > - "use-smartencoder" property - a name derived from the > feature/function and not the implementation detail might > be nicer, if anyone can think of one "avoid-reencoding" ? > > - the "request-pad" action signal is a bit confusing - how does > it work together with the normal request pad procedure? Does > it need to be released again? Maybe we should instead add > some kind of new gst_element_request_pad_for_caps() API to > core/GstElementClass instead? Yes, they need to be released like for 'normal' request pads. I'd be delighted to have a new core API for this... but might be pushing the deadline for merging a bit. > > - not sure about the labelling of unknown/other streams as > 'private' - when it's really just unknown/other > > - docs/design/design-encoding.txt code example refers to > "profile" property of string type: > g_object_set (encbin, "profile", "N900/H264 HQ", NULL), > whereas it seems to be a mini object now The idea was to register a GValue conversion method to search for the profile of that name. > > - "smartencoder" - can anyone think of a better name for this? > ("smart" doesn't really tell anyone anything unless they > already know what it does exactly) :) > > - smartencoder: should post an error with GST_ELEMENT_ERROR > when returning GST_FLOW_ERROR > > - streamsplitter: should post an error with GST_ELEMENT_ERROR > when returning GST_FLOW_ERROR > > - a small section in the design docs how streamsplitter/combiner + > smartencoder work together would be nice > > - are streamsplitter/combiner/smartencoder useful outside of encodebin > or should they maybe be kept private for now? Will hide smartencoder/streamsplitter elements, solving all problems above :)
> > [- the "request-pad" action signal is a bit confusing] (...) > > Yes, they need to be released like for 'normal' request pads. > I'd be delighted to have a new core API for this... but might be pushing the > deadline for merging a bit. It's just one function, should be quick. What is it we need exactly? Just GstPad * gst_element_get_request_pad_{from|for?}_caps (GstElement * element, GstCaps * caps); and struct _GstElementClass { ... GstPad * (*request_new_pad_{from|for?}_caps) (GstElement * element, GstCaps * caps); - gpointer _gst_reserved[GST_PADDING-1]; + gpointer _gst_reserved[GST_PADDING-2]; }; ? (Any additional parameters? e.g. name?) > > g_object_set (encbin, "profile", "N900/H264 HQ", NULL), > > The idea was to register a GValue conversion method to search for the profile > of that name. That's a good idea and would make things work in gst-launch, but not the code above (the vararg func will collect arguments based on the property GType, it doesn't know that you're passing a string).
The beast is now merged. commit a5994446b31f011fdc29ef7aff570c7001661891 Author: Edward Hervey <edward.hervey@collabora.co.uk> Date: Mon Sep 13 10:08:47 2010 +0200 examples: encoding example Along with gstcapslist commit 8a3b45aa1f5d6fcff5d8020a6ceddc0b7fc84353 Author: Edward Hervey <edward.hervey@collabora.co.uk> Date: Fri Aug 13 17:36:38 2010 +0200 gst: New encoding plugin https://bugzilla.gnome.org/show_bug.cgi?id=627476 commit 82b4f9bfef332393f95325d00d28d97ae784748c Author: Edward Hervey <edward.hervey@collabora.co.uk> Date: Fri Aug 13 17:27:52 2010 +0200 pbutils: New Profile library https://bugzilla.gnome.org/show_bug.cgi?id=627476