[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 #41 from Mark Nauwelaerts <mnauw at users.sourceforge.net> 2010-06-11 15:23:57 UTC ---
(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)
 View: https://bugzilla.gnome.org/attachment.cgi?id=162313
 Review: https://bugzilla.gnome.org/review?bug=607798&attachment=162313

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

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