[gstreamer-bugs] [Bug 607798] x264enc needs updating to support new features

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #59 from Robert Swain <robert.swain at gmail.com> 2010-07-09 16:14:04 UTC ---
(In reply to comment #58)
> (In reply to comment #47)
> > Created an attachment (id=165504)
 View: https://bugzilla.gnome.org/attachment.cgi?id=165504
 Review: https://bugzilla.gnome.org/review?bug=607798&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.
> 
> 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[].

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.