Created attachment 152599 [details] [review]
Expose additional upstream features

Attached patch should take care of most (low-level) items mentioned above (give or take tastes on property descriptions and/or defaults).

As for b-adapt, while it is not particularly complex changing this to an enum, this might be considered API/ABI breaking (which may not be the Way To Go ??).

As for the tunings and/or presets in x264:
* on the one hand, I do not know how (well?) this would combine with the "GStreamer level" presets (the latter being restricted to configuring properties that are actually exposed, the former not).
* on the other hand, it would be appreciated (and possible more generally useful) if these tunings and/or presets would in some way be accessible on library level (somehow comparable to x264_param_default).  That would avoid a code level copy-and-paste (in this and possibly other wrappers), which would have a hard(er) time keeping up with upstream modifications and/or additions in that area.

>and possible more generally useful) if these tunings and/or presets would in some way be accessible on library level

That's probably a good idea; I have this on my list of things to do (maybe adding them to x264enc can wait until I've added them to the library).  Here's the two options I've thought of; opinions would be welcome:

1) x264_param_default( &param, PRESET_NAME, TUNE_NAME )
2) x264_param_default( &param, "preset name", "tune name" )

The latter allows us to update presets/tunes without changing API, since it's string-based.  It's also simpler for CLI apps to use, since they can just pass the user's input blindly.

Do any of the above allow for multiple preset and/or tuning
(needs then to be called several times or comma-separated stuff or ...)?

Obviously, it would be convenient if updates would occur automagically :)
Since e.g. bitfields do not seem appealing, IMO involving a string is not particularly a problem.

On GStreamer/gobject level, it remains to sort out:
* whether tuning/presets should be set through GStreamer presets (doing so might in some views be cleaner, but with current state of things that would e.g. not always be convenient and particularly not benefit the cmd line way)
* the command-line semantics of presets/tunings (i.e. first apply presets, then tunings and then override with any other user provided options) are a bit hard to replicate using a "collection of gobject properties".  Some trickery might allow for that, but AFAICS that would probably (at least) require moving away from defaults provided by property specification to defaults provided by the underlying library.  The former would sort-of guarantee property stability, the latter not.  [But the encoder lib does not arrange for such stability anyway, and can always introduce new features, so I see not much point in trying to (pretend) stability can be maintained on wrapper level]

For the strings, we plan to only allow one preset, but allow multiple tunes as long as the tunes don't conflict.  The current CLI specifically allows only one "psy tuning" (e.g. "film" or "animation") but allows mixing that with other, unrelated tunings ("fastdecode", "zerolatency").

I think it makes the most sense to treat presets/tunes as some sort of string-based option in Gstreamer.  This would allow anyone to take advantage of the underlying functionality relatively easily (especially mixing tunes, etc).

Speaking of defaults provided by the underlying library, the current x264enc has very bizarre defaults that don't match up with libx264.  I can understand defaulting to faster settings (which makes sense, given that gstreamer is often used for live applications), but I notice in general that x264enc is often used "blindly" in Gstreamer pipelines without setting any important parameters.  This obviously doesn't make sense in most situations since one usually has to set at least some basic ratecontrol options.

Since presets/tunes are applied before other options, the current system probably won't make much sense--a preset will be applied, and then all of the current Gstreamer defaults will override it anyways.  I'm not sure what the best way to fix this is, since we're not supposed to break API by changing defaults.

> Since presets/tunes are applied before other options, the current system
> probably won't make much sense--a preset will be applied, and then all of the
> current Gstreamer defaults will override it anyways.  I'm not sure what the
> best way to fix this is, since we're not supposed to break API by changing
> defaults.

Well, I think changing defaults in cases like this is sort-of acceptable as long as the change doesn't have "dramatic" consequences, especially when it can be argued that the current defaults have either not been chosen with anything in mind, or don't make sense, or aren't particularly good. We've certainly done it with other encoders before (e.g. theora IIRC where we changed the default quality or somesuch).

Then I'll aim for some shaking up of the parameters (defaults) after next -ugly release (though not excluding other volunteers ;) )

In particular, to approximate "command line semantics", the defaults would probably (about all) become a sort-of "automatic", which internally then means x264_param_default (with possibly a preset/tuning applied).  A fair share of current defaults should already match that, and as for others, well ...

I think we need to check the property ranges too as, for example, I'm pretty sure b-frames and refs should allow 0-16 rather than 0-4 and 0-12 respectively.

Maybe when adding new options we should add them as enums or ints using validation even if they are boolean initially to avoid having to switch from boolean to int later.

For existing booleans that are now ints in x264, we will have to break gst's API a little so we should find an appropriate point at which we can do this.

As regards the presets using the x264 API, I think not being allowed to string together multiple presets is fine because in the x264 API, presets are only specifying quality options. We can manage other GStreamer presets that exert additional limitations on options using other parts of the x264 API. Using the speed/quality trade-off presets from x264 itself will at least leverage upstream maintenance for that.

I hope more comments and discussion will ensue. I'm happy to code this stuff if we can agree on what is needed to be current/flexible for x264 while not breaking things for people upgrading on the GStreamer side.

@Robert

I don't think property ranges are always sensible though.  There are complex interdependencies between options that are difficult to express as ranges, i.e. X can only be above Y if Z is set, and so forth.

Fair enough, but if A <= X <= B, C <= Y <= D and the current property isn't [ MIN(A,C), MAX(B,D) ] then the allowed range of the property in GStreamer is restricting x264's functionality.

In my opinion, at least, the min/max could be documented, else guarantee that all input is completely validated and appropriate error messages are given from the API when provided [ INT_MIN, INT_MAX ] or whatever.

What would you prefer?

Currently, we validate all the input options (to the best of our ability) and silently clip incorrect ones.

Printing errors for every possible invalid option would be completely unreasonable.

What kind of API would be reasonable for specifying valid ranges?

That would be OK I guess. For the majority of people, I expect presets to be the way to go. For those who want to hand-tune things, they should learn the ranges. Still, GStreamer needs to allow the full ranges of the options and they may as well be correct to act as a suggestion to the user.

FWIW, I'd avoid changing property types at this point (e.g. boolean to int), unless the property in question is new in git and hasn't been released yet, or was so obviously broken that we could be sure that no one was using it (e.g. if setting it caused a segfault or bogus output).

There's always the possibility to just start clean and do a x264newandimprovedenc and just deprecate the old one in some way (e.g. by posting a warning message on the bus when used or somesuch).

From an application perspective, calls to g_object_set() are allowed to do whatever they want to do with the value sent, including clipping to an unspecified range or rounding to some interval.

I've written a patch locally to move all the preset logic into x264.h.

Here's the documentation in x264.h; is this sufficient and reasonable?

/****************************************************************************
 * Advanced parameter handling functions
 ****************************************************************************/

/* These functions expose the full power of x264's preset-tune-profile system for
 * easy adjustment of large numbers of internal parameters.
 *
 * In order to replicate x264CLI's option handling, these functions MUST be called
 * in the following order:
 * 1) x264_param_default_preset
 * 2) Custom user options (via param_parse or directly assigned variables)
 * 3) x264_param_apply_fastfirstpass
 * 4) x264_param_apply_profile */

/* x264_param_default_preset:
 *      The same as x264_param_default, but also use the passed preset and tune
 *      to modify the default settings.
 *      (either can be NULL, which implies no preset or no tune, respectively)
 *
 *      Currently available presets are:
 *      ultrafast, veryfast, faster, fast, medium, slow, slower, veryslow, placebo
 *
 *      Warning: the speed of these presets scales dramatically.  Ultrafast is a full
 *      100 times faster than placebo!
 *
 *      Currently available tunings are:
 *      film, animation, grain, psnr, ssim, fastdecode, zerolatency
 *
 *      Multiple tunings can be used if separated by a delimiter in ",./-+",
 *      however multiple psy tunings cannot be used.
 *      film, animation, grain, psnr, and ssim are psy tunings.
 *
 *      returns 0 on success, negative on failure (e.g. invalid preset/tune name). */
int     x264_param_default_preset( x264_param_t *, const char *preset, const char *tune );

/* x264_param_apply_fastfirstpass:
 *      If first-pass mode is set (rc.b_stat_read == 1, rc.b_stat_write == 0),
 *      modify the encoder settings to disable options generally not useful on
 *      the first pass. */
void    x264_param_apply_fastfirstpass( x264_param_t * );

/* x264_param_apply_profile:
 *      Applies the restrictions of the given profile.
 *      Currently available profiles are: high, main, baseline
 *      (can be NULL, in which case the function will do nothing)
 *
 *      Does NOT guarantee that the given profile will be used: if the restrictions
 *      of "High" are applied to settings that are already Baseline-compatible, the
 *      stream will remain baseline.  In short, it does not increase settings, only
 *      decrease them.
 *
 *      returns 0 on success, negative on failure (e.g. invalid profile name). */
int     x264_param_apply_profile( x264_param_t *, const char *profile );

I've committed a somewhat better version of the above.  See the latest x264 for more details.

We will have to do some work on the internal properties APIs in GStreamer to allow a property's default value to be 'not set' so that the default value can be punted to libx264. I think this will be the most desirable solution for both parties and the best compromise.

If we do this, in the next release notes for -ugly, we must make note of changes that affect compatibility. For example, currently the element outputs Main Profile (baseline + cabac) I think whereas libx264 will default to High Profile. As such, we should make sure to detail how to preserve previous levels of compatibility through specifying which property to set to which value to preserve the 'status quo' w.r.t. H.264 profile.

Jason: If you can slap together a patch of what needs changing/updating with x264enc, it would be a useful starting point for getting it merged into GStreamer once someone has looked into the GStreamer properties issue that will enable the changes.

Created attachment 160799 [details] [review]
reimplement properties

Patch that reimplements a bunch of properties.  This is a pretty good approximation of what x264enc *should* be doing, ignoring any compatibility issues.

This is an API break, so this patch needs some work to either make it API compatible or add a new element x264enc2.

I've been doing some significant work towards creating a backward compatible changeset that allows use of presets, tunings and profiles as well as adding an option string parameter to allow configuration of any properties that are missing or wrongly implemented.

I do like some parts of your patch though so I think I'll try to merge bits of it into what I have. I'll post it here soon, but I haven't forgotten about this at all.

Would it be possible to have thing like the profile, etc be in the caps and negotiated through caps instead of having properties. So it will make it easier to do to SDP-style negotiation (by setting the SDP as the output caps of the rtp payloader).

You probably want to follow whats in RFC 3984, but split profile-level-id into 3 parts (having a hex string isn't very negotiable)

+1 for having profile/level configurable via caps.

> Would it be possible to have thing like the profile, etc be in the caps and
> negotiated through caps instead of having properties.

Why not In addition to having properties? We use properties for this kind of thing elsewhere, and properties also work better with e.g. GstPreset. Lastly, they are easy to discover via gst-inspect, hence make much better API IMHO.

I'm inclined to agree with Tim - my first thought was why not allow both methods? The properties make more sense to me but can someone elaborate as to why they want to be able to set what profile/level is used through caps?

I'm doing a bit more work on it right now to clean up some stuff I had left on my todo. I'll be posting it soon after I've tested that it all works.

I obviously meant "in addition to" supporting it via properties.

The reason for having profile/level configurable via caps is that it's extended information on the stream (which is described by caps). So if you have an element that can only accept video/x-h264,profile=blah,level=booh ... you can (1) look for an element that supports that and (@) have the element automatically configured to output that type of stream.

This is different from properties which aren't stream specific (like number of threads to use) and which are element/implementation specific.

In my case, I start with restrictions that come as SDP. Then I create a pipeline like "x264enc ! rtph264pay ! application/x-rtp, encoding-name=H264, profile-level-id=BLAH". If it was properties, my code would need to know about every decoder and know which properties need to be set to respect the restrictions in the caps. That knowledge (about the details of the encoder) can be restricted to the encoder element if we have standardised caps.

I see. Makes sense to me now too. I'll add support for profile/level from caps as well. Thanks for the comments.

The second patch has introduced the following regression:  When creating an MPEG-TS audio/video stream with faac, x264enc, and mpegtsmux, only the audio plays in QuickTime on iPhone OS or Mac OS X 10.6.  If I back out the patch, the problem goes away.  Perhaps the actual bug is in mpegtsmux, but whatever the problem is, it was brought on by the second patch on this bug report.

Created attachment 162313 [details] [review]
Use x264 APIs to better configure libx264 in x264enc

This is my implementation of what we've been discussing in this thread. It's a bit rough around the edges still but I wanted to get it out into the public for two reasons: so that if I run out of time someone else can pick it up and so that it can be reviewed by interested parties.

I have tested each of the options briefly and they appear to work. I'm not so experienced with handling string variables so maybe there are glaring bugs there. Basically I'm expecting to iterate through a few rounds of review before this is ready to be merged. Please scrutinise it and comment so that it can be improved for the benefit of both easing the using and extending the configurability of x264enc.

Hammer it and give feedback please! :)

Let's make this a blocker for the next release.

I need to check the strings used in set prop against those in x264's common/common.c x264_param_parse () code to make sure they will be parsed correctly. 'bpyramid' is wrong, it needs to be 'b-pyramid'. I'm not entirely sure why '-' aren't stripped.

Also, I have changed the defaults - I'll strip these changes out so that we maintain the current defaults. They can be reviewed separately unless people are OK with mostly matching the x264 CLI defaults - threads=0, speed-preset=6, profile=high.

threads=0 means 'auto'-matically choose the number of threads to suit the available cores. This has been tested by x264 peoples on a variety of systems and as I recall it comes out to be most efficient at around 1.5 * cores. 1 for single, 3 for dual, 6 for quad etc.

speed-preset=6 means preset medium which is a reasonable tradeoff of speed/quality. It should still be pretty fast but much better quality than the current default values.

profile=high corresponds to restricting options to H.264 High Profile features. This will play in basically all software players including Adobe Flash Player. I would expect the coming encoding profiles to choose the appropriate profile and impose other restrictions necessary to play on a particular target device. We can figure out what is needed to create files that play on iPods and PSPs for example.

If we want to retain compatibility with older x264 versions, we will have to have some ifdeffery and the code is not going to be pretty... The relevant APIs were moved into libx264 from the CLI for API version 86, so I'm told. Maybe most interesting distros that will receive the next release of -ugly will have current enough libx264 for backward compatibility to be irrelevant.

A commit was just made that corrected an error in the tuning documentation - stillimage is a psy tuning so it needs moving over from being a tuning. (Just making note here as a reminder.)

I had to make several fixes with latest x264 git

* "badapt" option string should be "b-adapt"
* tune=foo gets changed to tune=,foo which crashes x264 (bad free)
* "ret" not initialized in gst_x264_enc_parse_options() (compiler warning)

ah, tune=foo does get changed to tune=,foo but that isn't the crash; the crash is an x264 bug I think.

common.c

 static int x264_param_apply_tune( x264_param_t *param, const char *tune )
 {
-    char *tmp = x264_malloc( strlen( tune ) );
+    char *tmp = x264_malloc( strlen( tune ) + 1);

btw x264 is parsing these options with strtok() which is not threadsafe; I'm not sure this is strictly OK in a gstreamer context.

I can change it to use strtok_r, but keep in mind that some common platforms do not have strtok_r, and will revert to using the non-safe strtok anyways.

(In reply to comment #28)
> Created an attachment (id=162313) [details] [review]
> Use x264 APIs to better configure libx264 in x264enc
> 
> Hammer it and give feedback please! :)

* _init performs a _build_tunings_string and _default_preset, but that seems not yet necessary at that stage, as it will be done when needed in _init_encoder.

* when parsing (user provided option string), g_strsplit need not return 2 strings; is other code sufficiently prepared to handle some NULL safely?

* provided option-string is internally appended to option_string (which is also appended to by setting "normal properties").  So if option-string is set, and then again set to e.g. "", the previously set options still remain in effect.
Intended semantics are likely achieved by keeping this option-string in some separate string (from the one where other options are appended to), and then only append option-string to the "normal properties" at actual configuration time.

* why is there a x264enc_defaults (string) variable?  It seems to be used to set x264param according to the defaults of the properties.  However:

- why mixed ?, i.e. part are set using the x264enc_defaults string, and other parameter struct members are set directly.
- [nitpick++]  it seems not all properties are currently covered this way.  The remaining ones may have matching defaults on the properties and in libx264, but if the latter ones change in some release, then the element may not behave according to the default setting it claims.

(In reply to comment #37)
> (In reply to comment #28)
> > Created an attachment (id=162313) [details] [review] [details] [review]
> > Use x264 APIs to better configure libx264 in x264enc
> > 
> > Hammer it and give feedback please! :)
> 
> * _init performs a _build_tunings_string and _default_preset, but that seems
> not yet necessary at that stage, as it will be done when needed in
> _init_encoder.

Will fix.

> * when parsing (user provided option string), g_strsplit need not return 2
> strings; is other code sufficiently prepared to handle some NULL safely?

Ah, good point. Will fix.

> * provided option-string is internally appended to option_string (which is also
> appended to by setting "normal properties").  So if option-string is set, and
> then again set to e.g. "", the previously set options still remain in effect.
> Intended semantics are likely achieved by keeping this option-string in some
> separate string (from the one where other options are appended to), and then
> only append option-string to the "normal properties" at actual configuration
> time.

I think, and please correct me if I'm wrong, that currently if one specifies the same option twice, the latter will be respected as it will override the former. This is precisely what I want to preserve.

Maybe I need to make sure a separator is also appended in the correct locations to avoid miss-parsing due to concatenation of strings.

> * why is there a x264enc_defaults (string) variable?  It seems to be used to
> set x264param according to the defaults of the properties.  However:

This is so that if no preset or tuning are selected, the property defaults will be used. I need to revert my changes to that string though so that it does indeed reflect the property defaults and so that we don't use presets/tunings by default.

> - why mixed ?, i.e. part are set using the x264enc_defaults string, and other
> parameter struct members are set directly.

The ones that are set directly should be either interdependent on other such options or should be independent of presets/tunings. I think that there is no need to handle these parameters with strings.

> - [nitpick++]  it seems not all properties are currently covered this way.  The
> remaining ones may have matching defaults on the properties and in libx264, but
> if the latter ones change in some release, then the element may not behave
> according to the default setting it claims.

I can make the x264enc_defaults more accurate and explicit to current property defaults and post another patch. Hopefully that will clear this up unless I misunderstood you.

Thanks for the review!

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #28)
> > > Created an attachment (id=162313) [details] [review] [details] [review] [details] [review]
> > > Use x264 APIs to better configure libx264 in x264enc
> > > 
<snip>
> > * provided option-string is internally appended to option_string (which is also
> > appended to by setting "normal properties").  So if option-string is set, and
> > then again set to e.g. "", the previously set options still remain in effect.
> > Intended semantics are likely achieved by keeping this option-string in some
> > separate string (from the one where other options are appended to), and then
> > only append option-string to the "normal properties" at actual configuration
> > time.
> 
> I think, and please correct me if I'm wrong, that currently if one specifies
> the same option twice, the latter will be respected as it will override the
> former. This is precisely what I want to preserve.

Yes, this overriding applies to the "normal properties", but not to a custom user set option-string property.  Consider case where some (gui) code sets option-string property to some value, then sets it to empty again.  Clearly, the element should then act according to no option-string provided.  However, current implementation will still have the originally set option-string flying around [since only appending is ever done, and appending "" can hardly override anything previously, nor does overriding apply if the options set 2nd time are different than the 1st time ones].

> 
> Maybe I need to make sure a separator is also appended in the correct locations
> to avoid miss-parsing due to concatenation of strings.
> 
> > * why is there a x264enc_defaults (string) variable?  It seems to be used to
> > set x264param according to the defaults of the properties.  However:
> 
> This is so that if no preset or tuning are selected, the property defaults will
> be used. I need to revert my changes to that string though so that it does
> indeed reflect the property defaults and so that we don't use presets/tunings
> by default.
> 
> > - why mixed ?, i.e. part are set using the x264enc_defaults string, and other
> > parameter struct members are set directly.
> 
> The ones that are set directly should be either interdependent on other such
> options or should be independent of presets/tunings. I think that there is no
> need to handle these parameters with strings.
> 

Well, those would not necessarily have to be handled with a string.  Rather, why is there a string at all (?), and why are not all such set directly in the struct (mainly the mix being strange/confusing here)?

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (In reply to comment #28)
> > > > Created an attachment (id=162313) [details] [review] [details] [review] [details] [review] [details] [review]
> > > > Use x264 APIs to better configure libx264 in x264enc
> > > > 
> <snip>
> > > * provided option-string is internally appended to option_string (which is also
> > > appended to by setting "normal properties").  So if option-string is set, and
> > > then again set to e.g. "", the previously set options still remain in effect.
> > > Intended semantics are likely achieved by keeping this option-string in some
> > > separate string (from the one where other options are appended to), and then
> > > only append option-string to the "normal properties" at actual configuration
> > > time.
> > 
> > I think, and please correct me if I'm wrong, that currently if one specifies
> > the same option twice, the latter will be respected as it will override the
> > former. This is precisely what I want to preserve.
> 
> Yes, this overriding applies to the "normal properties", but not to a custom
> user set option-string property.  Consider case where some (gui) code sets
> option-string property to some value, then sets it to empty again.  Clearly,
> the element should then act according to no option-string provided.  However,
> current implementation will still have the originally set option-string flying
> around [since only appending is ever done, and appending "" can hardly override
> anything previously, nor does overriding apply if the options set 2nd time are
> different than the 1st time ones].

Aha, I see now. Will fix this too.

> > Maybe I need to make sure a separator is also appended in the correct locations
> > to avoid miss-parsing due to concatenation of strings.
> > 
> > > * why is there a x264enc_defaults (string) variable?  It seems to be used to
> > > set x264param according to the defaults of the properties.  However:
> > 
> > This is so that if no preset or tuning are selected, the property defaults will
> > be used. I need to revert my changes to that string though so that it does
> > indeed reflect the property defaults and so that we don't use presets/tunings
> > by default.
> > 
> > > - why mixed ?, i.e. part are set using the x264enc_defaults string, and other
> > > parameter struct members are set directly.
> > 
> > The ones that are set directly should be either interdependent on other such
> > options or should be independent of presets/tunings. I think that there is no
> > need to handle these parameters with strings.
> > 
> 
> Well, those would not necessarily have to be handled with a string.  Rather,
> why is there a string at all (?), and why are not all such set directly in the
> struct (mainly the mix being strange/confusing here)?

Use of the strings is mandatory to get the correct order of assignment and to only assign user-set options.

Consider the case where a preset is selected. If we assign values directly to the struct, we have no idea if they were user-set or if they are property defaults. So what would happen is we would call the x264 API with the preset to initialise the defaults, then assign all property values to the x264 parameters (whether user-set or prop defaults) which would override the preset values and nullify the point of a preset.

However, if we use setprop to assign to the struct and append to a string, we can just parse the user-set properties from the string and ignore the property defaults so that the preset is respected.

Maybe get prop should read from the x264param directly rather than GstX264Enc...?

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #38)
> > > (In reply to comment #37)
> > > > (In reply to comment #28)
> > > > > Created an attachment (id=162313) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review]
> > > > > Use x264 APIs to better configure libx264 in x264enc
> > > > > 
<snip++>
> > > Maybe I need to make sure a separator is also appended in the correct locations
> > > to avoid miss-parsing due to concatenation of strings.
> > > 
> > > > * why is there a x264enc_defaults (string) variable?  It seems to be used to
> > > > set x264param according to the defaults of the properties.  However:
> > > 
> > > This is so that if no preset or tuning are selected, the property defaults will
> > > be used. I need to revert my changes to that string though so that it does
> > > indeed reflect the property defaults and so that we don't use presets/tunings
> > > by default.
> > > 
> > > > - why mixed ?, i.e. part are set using the x264enc_defaults string, and other
> > > > parameter struct members are set directly.
> > > 
> > > The ones that are set directly should be either interdependent on other such
> > > options or should be independent of presets/tunings. I think that there is no
> > > need to handle these parameters with strings.
> > > 
> > 
> > Well, those would not necessarily have to be handled with a string.  Rather,
> > why is there a string at all (?), and why are not all such set directly in the
> > struct (mainly the mix being strange/confusing here)?
> 
> Use of the strings is mandatory to get the correct order of assignment and to
> only assign user-set options.
> 
> Consider the case where a preset is selected. If we assign values directly to
> the struct, we have no idea if they were user-set or if they are property
> defaults. So what would happen is we would call the x264 API with the preset to
> initialise the defaults, then assign all property values to the x264 parameters
> (whether user-set or prop defaults) which would override the preset values and
> nullify the point of a preset.

I am particularly talking about the following code fragment, which is the only place where x264enc_defaults is actually used:
----
  /* if no preset nor tuning, use property defaults */
  if (!encoder->speed_preset && !encoder->tunings->len) {
    if (gst_x264_enc_parse_options (encoder, x264enc_defaults) == FALSE) {
      GST_DEBUG_OBJECT (encoder, "Failed to parse defaults string");
      return FALSE;
    }
    encoder->x264param.b_deblocking_filter = 1;
    encoder->x264param.i_deblocking_filter_alphac0 = 0;
    encoder->x264param.i_deblocking_filter_beta = 0;
  }
----
Hence, one need not consider the case where a preset is selected, because that fails the if condition.  So, again, in the above conditions, it is not clear why some are set by a string in _parse_options, and the others set directly a few lines below.  [Suffice to say this is hardly critical/problematic, but the above fragment does simply evoke "why?"]

> 
> However, if we use setprop to assign to the struct and append to a string, we
> can just parse the user-set properties from the string and ignore the property
> defaults so that the preset is respected.
> 
> Maybe get prop should read from the x264param directly rather than
> GstX264Enc...?

See above, preset etc do not apply for x264enc_defaults' "scope".

(In reply to comment #41)

[...]

> I am particularly talking about the following code fragment, which is the only
> place where x264enc_defaults is actually used:
> ----
>   /* if no preset nor tuning, use property defaults */
>   if (!encoder->speed_preset && !encoder->tunings->len) {
>     if (gst_x264_enc_parse_options (encoder, x264enc_defaults) == FALSE) {
>       GST_DEBUG_OBJECT (encoder, "Failed to parse defaults string");
>       return FALSE;
>     }
>     encoder->x264param.b_deblocking_filter = 1;
>     encoder->x264param.i_deblocking_filter_alphac0 = 0;
>     encoder->x264param.i_deblocking_filter_beta = 0;
>   }
> ----
> Hence, one need not consider the case where a preset is selected, because that
> fails the if condition.  So, again, in the above conditions, it is not clear
> why some are set by a string in _parse_options, and the others set directly a
> few lines below.  [Suffice to say this is hardly critical/problematic, but the
> above fragment does simply evoke "why?"]

Oh, I see. I think that's kind of an accident. I created the x264enc_defaults string from the property defaults and then edited it as I went along. There aren't any properties for the deblocking filter parameters, so they're being set directly via some hard-coded means.

I don't know if this distinction makes sense. Maybe those options should be merged into the x264enc_defaults string. I don't really mind. Else I can add a note that these options are being hard-coded because properties don't exist for them.

Created attachment 164001 [details] [review]
Expose additional upstream features

This is merely a slightly bug fixed version of oldest patch [so it is not to say that is the way to go, the other approach is just fine, but in case anyone might want to use a simpler version with a few properties in the meantime ...]

Since it does not complicate matters further, the following commit then at least exposes some additional properties (which it was originally about), pending further work on the other patch:

commit 27025d0ebd048b62881dd1c2d91fbe0e44c1c010
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Fri Jun 18 14:35:00 2010 +0200

    x264enc: conditionally expose enhanced upstream capabilities
    
    API: GstX264Enc:sliced-threads
    API: GstX264Enc:sync-lookahead
    API: GstX264Enc:intra-refresh
    API: GstX264Enc:mb-tree
    API: GstX264Enc:rc-lookahead
    
    See #607798.

Rebased on top of master. Comments 30, 32, 37 points 1 and 3 have been addressed in the patch that follows.

Comment 37, point 2:

* when parsing (user provided option string), g_strsplit need not return 2
strings; is other code sufficiently prepared to handle some NULL safely?

The x264 API x264_param_parse () handles this. If the value passed to it is NULL, the value becomes "true". This is so that one can do things like "something=this:cabac:option=blah" and cabac will be set appropriately.

Point 4 about the x264enc_defaults string: I shall review the defaults and clarify this string and the deblocking filter parameters that get assigned directly to x264 in the adjacent code.

Regarding the thoughts about libx264 API versions at the end of comment 31, using strings for everything that can be parsed by a string (except the inter-dependent rate control options and those that use other APIs such as the presets) avoids direct access to libx264 API struct members and so #ifdef-ing the code becomes unnecessary. However, I do need to make the code just give a warning rather than erroring out if invalid parameters are encountered as this will occur if an older x264 is used and the parameter doesn't exist. And of course I need to protect this new API that is being used.

So, to summarise the TODO list:
- Check defaults and fix x264enc_defaults string.
- Check API version for these newly-used APIs.

After that, I think it should be pretty much good to go... touch wood. :)

Created attachment 165500 [details] [review]
Use x264 APIs to better configure libx264 through x264enc

Created attachment 165504 [details] [review]
Use x264 APIs to better configure libx264 through x264enc

Fix a couple of compile bugs. I think the g_string_assign () in _set_property () is OK but would welcome verification.

Review of attachment 165504 [details] [review]:

::: ext/x264/gstx264enc.c
@@ +1495,3 @@
   GstX264Enc *encoder;
   GstState state;
+  const gchar *me_string[] = { "dia", "hex", "umh", "esa" };

There's also "tesa" ME method available in newer x264.

Why aren't we using x264_motion_est_names?  It's in the x264.h header!

BTW, why do you call the property "speed-preset" instead of "preset" as x264 does? It does relate to encoding speed/quality, but it changes the compatibility features like cabac, bframes, number of references, partitions, etc. If a user doesn't enforce a profile or set other properties manually then setting a speed-preset might cause a compatibility issues with decoder user wants to use.

The presets are intended to be speed presets.  Effects on compatibility are merely incidental -- if a user wants to be compatible with a device that doesn't support X, he should disable X directly, via profile or the actual option.  The presets never increase complexity beyond the users' own limits.

Also, partitions are not a compatibility issue, ever.

But the speed- prefix gives false impression that it affects speed only. I've seen a TV that wasn't able to decode properly encodes that were done with slower preset with forced profile and level, and it was able to do all right with faster preset. The issue was not the level/profile, but the number of bframes and/or b-pyramid (I don't know switching which helped). And the TV was able to decode each of these encodes, but the slower preset caused something we called "strobo", as it seamed like decoder wasn't performant enough to deal with decoding more complex bitstream (looked like it was dropping frames).

That means the user didn't set the level, profile, or other bitstream restrictions correctly.  The faster preset was working via sheer luck.  If you believe otherwise, file a bug report.

In short -- "placebo" means "use the most of all the features x264 is allowed to use".  Anything less than placebo is not guaranteed to use all of the features x264 is allowed to use.  Therefore, even if "all the features x264 is allowed to use" doesn't correctly match up with "what features I want x264 to be able to use", a faster preset may work by coincidence.

This does not mean the presets are intended to "change compatibility".

(In reply to comment #49)
> Why aren't we using x264_motion_est_names?  It's in the x264.h header!

I think it was for some compatibility reason, but I may as well use what's in the x264 header and have it self-update.

Ok, then if I'm not using placebo I'm trading in not only speed, which increases, but also quality and features of the output stream. So why is it called "speed-preset"? :-) Actually I think x264 gets it right with "preset" name, but I might be biased as I don't particularly like long property names that makes me wrap lines of my code ;-). 

I'm new here so I thought I might contribute by proposing simpler, or "lesser", interface for a plugin, which actually makes the name compatible with x264 interface I'm used too. I guess I had, as they say, a "tough start".

I agree that changing the name is pointless and breaks compatibility needlessly.

(In reply to comment #47)
> Created an attachment (id=165504) [details] [review]
> Use x264 APIs to better configure libx264 through x264enc
> 
> Fix a couple of compile bugs. I think the g_string_assign () in _set_property
> () is OK but would welcome verification.

The g_string_assign looks OK.

A few other "nitpicks", in gst_x264_enc_init_encoder:

* there are several exit points (return) that bail out while GST_OBJECT_LOCK() is taken.  That should at least then be unlocked, and it might be considered if re-flowing those exits to some ERROR section at end is cleaner (or maybe not).

* there is twice a debug message "Failed to parse option string", where one parses a user-provided one and another an internally generated one.  While the message is correct in both cases, a slightly different message for each case might be more beneficial (as debug messages go).

(In reply to comment #58)
> (In reply to comment #47)
> > Created an attachment (id=165504) [details] [review] [details] [review]
> > Use x264 APIs to better configure libx264 through x264enc
> > 
> > Fix a couple of compile bugs. I think the g_string_assign () in _set_property
> > () is OK but would welcome verification.
> 
> The g_string_assign looks OK.

Good. :)

> A few other "nitpicks", in gst_x264_enc_init_encoder:
> 
> * there are several exit points (return) that bail out while GST_OBJECT_LOCK()
> is taken.  That should at least then be unlocked, and it might be considered if
> re-flowing those exits to some ERROR section at end is cleaner (or maybe not).

I've made these cases print their own error then goto an unlock and return label.

> * there is twice a debug message "Failed to parse option string", where one
> parses a user-provided one and another an internally generated one.  While the
> message is correct in both cases, a slightly different message for each case
> might be more beneficial (as debug messages go).

I've edited the internal option string debug print message and the parsing code to handle these a bit better. The theory is this:

* x264enc_defaults parsing - this shouldn't produce parsing errors. If it does, it's a bug and so we should error out.

* option-string parsing - the user should provide a valid string and so we should error out if something is wrong here.

* Internal option string parsing for properties - there can be API mismatch as we could have properties that an old libx264 does not have. This should not cause an error.

* Any parse error from x264_param_parse () in any case will print error messages to notify of invalid parameters. These are currently GST_ERROR_OBJECT () error messages. Shout if you want them to be at a less significant level.

* As a consequence of not always wanting to error out completely if we encounter a single invalid parameter as part of an option string, I have edited gst_x264_enc_parse_options () slightly to make it attempt to parse all key=value pairs before returning.

See the patch that follows.

Re: preset vs speed-preset - this was a suggestion to indicate the purpose of the preset so that your every day person has an idea of what the preset is about. It is solely about the speed versus quality (or compression) tradeoff. If your target device has additional restrictions, you need to learn what they are and impose them separately. I have documented this in the property but would prefer to stick with speed-preset.

I have also edited the ME type generation so that it uses x264_motion_est_names[].

Created attachment 165564 [details] [review]
Use x264 APIs to better configure libx264 through x264enc

Created attachment 165569 [details] [review]
Use x264 APIs to better configure libx264 through x264enc

I've made an attempt to make the code backward compatible with libx264 API version < 86. I only tested by compiling and gst-inspect x264enc. I will test more once I've sorted out the defaults. Review of the version #if logic is welcome as it's pretty easy to make mistakes in there.

Properties whose existence depends on a library version are not a good idea.  If the library is too old, use a g_warning().

Please break up the patch into parts we are definitely going to change, like adding new properties vs. other stuff.  This element should really be fixed before the next -ugly release.  Otherwise I'm going to just start applying my own, potentially incompatible, changes.

(In reply to comment #62)
> Properties whose existence depends on a library version are not a good idea. 
> If the library is too old, use a g_warning().

All the properties for this element depend on some x264 API version or another. Ubuntu Lucid only ships libx264 with API version 85. For the preset/profile APIs one needs version 86 or newer. 86 was only made available in February. It was deemed reasonable to do some #if work depending on the version but perhaps your suggestion is better.

In short, you're suggesting the properties should exist always but if the library version is less than what we require, we issue a g_warning () from one of the init function or the set properties function.

> Please break up the patch into parts we are definitely going to change, like
> adding new properties vs. other stuff.  This element should really be fixed
> before the next -ugly release.  Otherwise I'm going to just start applying my
> own, potentially incompatible, changes.

If someone decides there's some part we don't want to change, say so and I'll split it out. I intend to split out the defaults change. Otherwise as far as I'm concerned I'm just iterating until it's good enough to land in master (I still have things on my to do list for this patch so I don't think it is ready yet) and I will have time to do so before the next -ugly freeze.

Created attachment 166204 [details] [review]
Use x264 APIs to better configure libx264 through x264enc

This iteration fixes up the defaults so that the only changes relate to reworking everything for the addition and proper application of the new properties.

I have another patch for improving the defaults but I will submit that in a separate ticket.

I need to go through and test all the options again - I should be able to do that today.

The patch may need splitting into separate patches for addition of the new properties and re-working the code to construct and apply defaults and user-set properties in the correct order. I don't have much confidence that this will look any cleaner in the history, but if desired, I will do it. Please mention if you would like to see any further splitting as well.

Comments and review are welcome as always. :)

I've split the changes into multiple patches - one each for option-string, fast-first-pass and profile; one for refactoring needed for the presets/tunings; one for the presets/tunings and then a few other misc ones. Rather than attach all the patches here, I'll push them somewhere in the morning.

The patch stack can be found on the x264enc_final branch at:

git://git.collabora.co.uk/git/user/robswain/gst-plugins-ugly

I may do non-fast-forward pushes to that repo if necessary to rebase on top of master and such.

Here goes somewhat lengthy review:

* Add option-string property (0b40b1ed7699eddec9ca56a7ac2360851911cf0a):
Looks OK.

* Add profile property (367a7f012fb17a896c80bb3e193ea760f6d90af6):
I suppose it is possible for a profile restriction to override other properties, in which case that might also be mentioned/warned in the property description.
Otherwise, code looks good except for the 
#if (X264_BUILD >= 86)
that are littered all over.  On the one hand, if a particular release is a turning point and needs often #if, then it might be useful/cleaner to introduce a symbolic #define and then use a #ifdef elsewhere, see e.g. current practice at the top of x264enc.  On the other, only limited code actually needs to be #if'ed.  There is no need for it in the header file, the ARG_ enum declaration, in get and set property, etc.  Only where it would actually fail (typically in interaction with x264 structures), and the declaration of the corresponding property, need be made conditional (see also current practice).

* Add fast-first-pass property (61242031227f98adfa1836580323b2dc21bbce1c):
The internal (conditional) implementation change to x264_param_apply_fastfirstpass looks ok, but do we need a new property for this where there was none before?  In particular, is there ever a reason other than internal libx264 debugging to have this set to FALSE (as it claims to make no functional/operational difference whatsoever, other than speed of course)?

* Update available me types (512159e3b06897ce52425740f7f07ea0bb1f3b83):
Looks good, but does it need any conditional compilation (e.g. minding x264_motion_est_names)?

* Refactor code (6616b4b1cd03f8b72e9d7aafb1822fa1d487c5ae):
Some caution in moving the log handler setup.  It is now being setup after calls to libx264 have already been done (option parsing).  While those calls may not lead to any debug handling, one never knows there ...  That is, wherever moved, it should be before as much as possible libx264 calls (other than x264_param_default, which resets the log handler).

* Add speed-preset and [psy-]tuning properties (37371bd90bf062e02abdd757abf4256ff93ffde5):
Opinions may vary, but it looks like the part in commit message as of 'For example' would go better in the plugin documentation comment section, then at least that is also updated.
Also, same comment w.r.t. #if's as before.

Remaining ones are OK (but some additional debug might be merged with appropriate earlier commits).

(In reply to comment #68)
> Here goes somewhat lengthy review:
> 
> * Add option-string property (0b40b1ed7699eddec9ca56a7ac2360851911cf0a):
> Looks OK.

Pushed.

Module: gst-plugins-ugly
Branch: master
Commit: f2be62695c9bcadc70a778a6825298a579f7c013
URL:    http://cgit.freedesktop.org/gstreamer/gst-plugins-ugly/commit/?id=f2be62695c9bcadc70a778a6825298a579f7c013

Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 15:40:27 2010 +0200

x264enc: Add option-string property

Adds support for an x264 format option-string to specify advanced parameters

Addresses part of bug #607798

> * Add profile property (367a7f012fb17a896c80bb3e193ea760f6d90af6):
> I suppose it is possible for a profile restriction to override other
> properties, in which case that might also be mentioned/warned in the property
> description.

OK. Fixed.

> Otherwise, code looks good except for the 
> #if (X264_BUILD >= 86)
> that are littered all over.  On the one hand, if a particular release is a
> turning point and needs often #if, then it might be useful/cleaner to introduce
> a symbolic #define and then use a #ifdef elsewhere, see e.g. current practice
> at the top of x264enc.  On the other, only limited code actually needs to be
> #if'ed.  There is no need for it in the header file, the ARG_ enum declaration,
> in get and set property, etc.  Only where it would actually fail (typically in
> interaction with x264 structures), and the declaration of the corresponding
> property, need be made conditional (see also current practice).

There are a few APIs (preset, tuning, profile, fast first pass) that all depend on this particular API revision. I can make a separate define if you wish but I'm not sure what a suitable name might be. I suppose the highlight is presets so perhaps X264_PRESETS.

I'll edit the #ifdefs to just affect the code that will cause interaction with x264. However, is there any problem with exposing properties that aren't supported by the used libx264 version? What about David's g_warning() suggestion if the property isn't supported? It should be easy enough to add that into an appropriate #else case I think.

> * Add fast-first-pass property (61242031227f98adfa1836580323b2dc21bbce1c):
> The internal (conditional) implementation change to
> x264_param_apply_fastfirstpass looks ok, but do we need a new property for this
> where there was none before?  In particular, is there ever a reason other than
> internal libx264 debugging to have this set to FALSE (as it claims to make no
> functional/operational difference whatsoever, other than speed of course)?

Not really. I'll edit the patch so that it just swaps to using the new API when available and not add the property.

> * Update available me types (512159e3b06897ce52425740f7f07ea0bb1f3b83):
> Looks good, but does it need any conditional compilation (e.g. minding
> x264_motion_est_names)?

X264_BUILD >= 36, so not really necessary I think.

> * Refactor code (6616b4b1cd03f8b72e9d7aafb1822fa1d487c5ae):
> Some caution in moving the log handler setup.  It is now being setup after
> calls to libx264 have already been done (option parsing).  While those calls
> may not lead to any debug handling, one never knows there ...  That is,
> wherever moved, it should be before as much as possible libx264 calls (other
> than x264_param_default, which resets the log handler).

I've moved that part back and then it gets duplicated in the next commit. This is necessary because every time one of the _param_default_* () functions is called, these log options will be reset to some x264 defaults. I added a note to this effect.

> * Add speed-preset and [psy-]tuning properties
> (37371bd90bf062e02abdd757abf4256ff93ffde5):
> Opinions may vary, but it looks like the part in commit message as of 'For
> example' would go better in the plugin documentation comment section, then at
> least that is also updated.
> Also, same comment w.r.t. #if's as before.

OK. I've moved the example to the top of the file with the other commentary.

> Remaining ones are OK (but some additional debug might be merged with
> appropriate earlier commits).

OK. I'll wait on these until the rest are OKed so that they can be applied in order. I've merged most of the debug stuff into appropriate earlier commits but I forgot to merge one hunk relevant to the option string parsing so that remains in a separate commit.

Thanks for the review and I look forward to the next! :)

I made a few changes and pushed them to the x264enc_fin branch from the same repository as I screwed up a bit with the x264enc_final branch it seems. Apologies.

Looks mainly OK now; except for
* a bit more #ifdef are needed around the various enum type definitions (at top of file)
* some more of the commit message(s) might go into documentation

I have pushed a proposed end-result with those changes to
git://git.collabora.co.uk/git/user/manauw/gst-plugins-ugly

They will likely need merging and/or gradual introduction along with previous commits.

OK, then unless there are any further objections, I will hopefully push this on Monday.

Pushed.

commit 60d647dcf520e7c8c46acde8344c89bb20a47f8c
Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 17:24:33 2010 +0200

    x264enc: Improve x264enc defaults
    
    - medium x264 speed/quality preset
    - threads defaults to 0 which automatically uses 1.5x number of cpu cores
    
    Addresses part of bug #607798

commit b27ce43ab892f9d6c47927a1d8dfef3f419398d8
Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 17:09:20 2010 +0200

    x264enc: Add speed-preset and [psy-]tuning properties
    
    Use of a rate control method (pass, bitrate, quantizer, etc properties), a
    preset and possibly a profile and/or tuning are now the recommended way to
    configure x264 through x264enc.
    
    If a preset/tuning are specified then these will define the default values and
    the property defaults will be ignored. After this the option-string property is
    applied, followed by the user-set properties, fast first pass restrictions and
    finally the profile restrictions.
    
    Addresses part of bug #607798

commit f269e0679f5ac378700c9ef4190767bce3d804d3
Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 16:56:06 2010 +0200

    x264enc: Refactor code in preparation for presets/tunings
    
    - Make defaults append the appropriate default value to a string. This is
      needed to differentiate between something user-set and the actual prop
      default.
    - Add an internal option string to which _set_property () cases append for the
      majority of properties.
    - Use gst_x264_enc_parse_options () to clean up application of settings. This
      will make order of application with respect to the presets and tunings quite
      simple.
    
    Addresses part of bug #607798

commit d6f766d10f2b7162bdcf45294a83260075261a3f
Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 15:59:12 2010 +0200

    x264enc: Use new libx264 API to affect fast first pass
    
    Uses new x264 API to apply reduced complexity values to the parameters to
    increase encoding speed in the first pass of a multi-pass encode. This does
    not impact on final quality.
    
    Addresses part of bug #607798

commit 76cc4dbc07052a0547a6c4a6b3165351028044e9
Author: Robert Swain <robert.swain@collabora.co.uk>
Date:   Wed Jul 21 15:52:28 2010 +0200

    x264enc: Add profile property
    
    In X264_BUILD >= 86 there is a new API for applying restrictions to an H.264
    Profile. This makes it easier to achieve Baseline Profile for example.
    
    Addresses part of bug #607798

Closing this one, as the most blocking issues have been addressed.

There are still some remaining issues: someone needs to go through the properties and check them against the x264 api (boolean/int changes, ranges etc.). There'll be new bug opened for that