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

Fri Jul 23 02:25:04 PDT 2010

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

--- Comment #69 from Robert Swain <robert.swain at gmail.com> 2010-07-23 09:24:57 UTC ---
(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 at 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! :)

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