[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 #68 from Mark Nauwelaerts <mnauw at users.sourceforge.net> 2010-07-22 13:08:57 UTC ---
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).

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