[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 #38 from Robert Swain <robert.swain at gmail.com> 2010-06-11 13:58:19 UTC ---
(In reply to comment #37)
> (In reply to comment #28)
> > Created an attachment (id=162313)
 View: https://bugzilla.gnome.org/attachment.cgi?id=162313
 Review: https://bugzilla.gnome.org/review?bug=607798&attachment=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.

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!

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