[Ffmpeg-devel] [PATCH] x264 interface update

[Michael Niedermayer]

Hi

On Sun, Dec 18, 2005 at 04:36:00AM -0000, Robert Swain wrote:
> Hello,
> 
> I've been working on the x264 interface, attempting to update its
> configurability before the next release of ffmpeg, if it goes ahead. I have
> attached a patch which brings the configurability up to a current level. I
> have not done anything about the default values of parameters being reused,
> only those that I have added. Each parameter addition was for a reason, but
> if any of you have reason that may be overriding then I'd appreciate your
> input. I'd like to look into altering the defaults of reused parameters for
> external codecs at a later date and merely provide a default command line
> that matches the defaults with those of x264.
> 
> -gop_size 250 -rc_buffer_size 0 -rc_max_rate 0 -cqp 26 -max_b_frames 
> 0 -b_frame_strategy 1 -bframebias 0 -keyint_min 25 -sc_threshold 40 
> -deblock 0:0 -qmin 10 -qmax 51 -max_qdiff 4 -qcompress 0.6 -qblur 0.5 
> -complexityblur 20.0 -refs 1 -partitions p8x8,b8x8,i8x8,i4x4 -directpred 
> temporal -me_method 7 -me_range 16 -level 51 -ratetol 1.0 -i_quant_factor 
> 0.7 -b_quant_factor 1.3 -chromaoffset 0 -rc_eq "blurCplx^(1-qComp)" 
> -loglevel 2 -thread_count 1 -ac -bpyramid 0 -loop -wpred 0 -brdo 0 
> -mixed_refs 0 -chroma_me 1 -8x8dct 0 -fastpskip 1 -psnr -aud 0
> (Note 1: All parameters are included regardless of whether the command line 
> is functional/good or whether they differ from the ffmpeg defaults)
> (Note 2: for the 'flags2' flags -<flag> 1 means enabled, and 0 disabled, 
> for clarity)
> 
> I have a few questions which I would like to resolve before anything is done
> with this patch and I think they are the only things that remain to be done.
> 
> - Why do 'flags2' flags require an argument despite the argument not
> appearing to have a function? (I tried -brdo 0 and -brdo 1, and both enabled
> brdo)

because the option parser isnt perfect, send patch, furthermore flags1 need an
argument too currently unless they havnt been ported to the new system


[...]

> 
> - Why does setting a float in the default value parameter of the
> libavcodec/utils.c options array not actually set the default value to that
> value?
> I used GDB to print the value of avctx->ratetol for example. In the value in
> options[] (libavcodec/utils.c) was 1, but in AVCodecContext it was 0. Should
> avcodec_get_context_defaults() be used for setting default values?

avcodec_get_context_defaults() could _maybe_ be changed to extract and set all
the defaults for AVCodecContext (patch welcome assuming this has no sideeffects)


> 
> - What is the accepted method for altering the default value of a flag? A

the default of the "flags" or "flags2" entry should do, for example:
{"flags", NULL, OFFSET(flags), FF_OPT_TYPE_FLAGS, CODEC_FLAG_4MV | ... , INT_MIN, INT_MAX, V|A|E|D, "flags"},


> couple of the flags I added should be enabled by default. Furthermore, what
> is the accepted method of disabling a flag? Once a flag is enabled by
> default, how can it be disabled?

enable: -4mv foobar or -flags +4mv 
disable: -flags -4mv 

you could also look at opt.c and ad -no4mv style support

[...]

>@@ -834,10 +844,6 @@
>      */
>     int delay;
>
>-    /* - encoding parameters */
>-    float qcompress;  ///< amount of qscale change between easy & hard scenes (0.0-1.0)
>-    float qblur;      ///< amount of qscale smoothing over time (0.0-1.0)
>-
>     /**
>      * minimum quantizer.
>      * - encoding: set by user.

/me looks left, looks right where are the ABI trolls? only trolling like
always but not looking at submitted patches but a month later they crawl
out and complain again

CODEC_FLAG2_CHROMA_ME is a flag in the cmp functions in lavc
cqp looks a little redundant (CODEC_FLAG_QSCALE, ...) or does this have more
then on/off?

loglevel and ratetol seems redundant too

i dont like the char* style variables at all, they should be flags or enums
IMHO

i do like the zones more then our rc_override thing, maybe we could after
applying this patch mark rc_override as deprecated and use char* zones

-- 
Michael