[FFmpeg-devel] Path for minor problems related to ffmpeg help text and x264 configuration

[Robert Swain]

On 25 Sep 2008, at 20:43, Stefano Sabatini wrote:
> On date Tuesday 2008-09-23 14:35:03 +0200, Francesco Cosoleto encoded:
>> I am not able to submit this simple patch using the bug-tracker due  
>> to a
>> server error, but here I think file attachment works, then here the  
>> file.
>>
>> * ffmpeg.c: Fixed a problem in printing option when ffmpeg is  
>> compiled
>> without --enable-swscale
>> * libavcodec/utils.c, libavcodec/utils.h: Fixed documentation for  
>> direct mv
>> prediction mode automatic
>> * libavcodec/libx264.c: Added support for PSNR flag, code cosmetic  
>> changes
>> to use x264_param2string()
>> * ffpresets/libx264-fastfirstpass, ffpresets/libx264-default.ffpres,
>> ffpresets/libx264-max.ffpreset, ffpresets/libx264-hq.ffpreset,
>> ffpresets/libx264-normal.ffprese: 'me' replaced with 'me_method',  
>> now 'me'
>> doesn't exist anymore in options list.

[...]

>> Index: libavcodec/libx264.c
>> ===================================================================
>> --- libavcodec/libx264.c	(revisione 15383)
>> +++ libavcodec/libx264.c	(copia locale)
>> @@ -164,7 +164,7 @@
>>     x4->params.b_cabac = avctx->coder_type == FF_CODER_TYPE_AC;
>>     x4->params.i_bframe_adaptive = avctx->b_frame_strategy;
>>     x4->params.i_bframe_bias = avctx->bframebias;
>> -    x4->params.b_bframe_pyramid = avctx->flags2 &  
>> CODEC_FLAG2_BPYRAMID;
>> +    x4->params.b_bframe_pyramid = avctx->flags2 &  
>> CODEC_FLAG2_BPYRAMID ? 1 : 0;
>>     avctx->has_b_frames= avctx->flags2 & CODEC_FLAG2_BPYRAMID ?  
>> 2 : !!avctx->max_b_frames;
>>
>>     x4->params.i_keyint_min = avctx->keyint_min;
>> @@ -173,7 +173,7 @@
>>
>>     x4->params.i_scenecut_threshold = avctx->scenechange_threshold;
>>
>> -    x4->params.b_deblocking_filter = avctx->flags &  
>> CODEC_FLAG_LOOP_FILTER;
>> +    x4->params.b_deblocking_filter = avctx->flags &  
>> CODEC_FLAG_LOOP_FILTER ? 1 : 0;
>>     x4->params.i_deblocking_filter_alphac0 = avctx->deblockalpha;
>>     x4->params.i_deblocking_filter_beta = avctx->deblockbeta;
>>
>> @@ -210,7 +210,7 @@
>>
>>     x4->params.analyse.i_direct_mv_pred = avctx->directpred;
>>
>> -    x4->params.analyse.b_weighted_bipred = avctx->flags2 &  
>> CODEC_FLAG2_WPRED;
>> +    x4->params.analyse.b_weighted_bipred = avctx->flags2 &  
>> CODEC_FLAG2_WPRED ? 1 : 0;
>>
>>     if(avctx->me_method == ME_EPZS)
>>         x4->params.analyse.i_me_method = X264_ME_DIA;
>> @@ -230,10 +230,10 @@
>>     x4->params.analyse.b_bidir_me = avctx->bidir_refine > 0;
>>     x4->params.analyse.b_bframe_rdo = avctx->flags2 &  
>> CODEC_FLAG2_BRDO;
>>     x4->params.analyse.b_mixed_references =
>> -        avctx->flags2 & CODEC_FLAG2_MIXED_REFS;
>> -    x4->params.analyse.b_chroma_me = avctx->me_cmp & FF_CMP_CHROMA;
>> -    x4->params.analyse.b_transform_8x8 = avctx->flags2 &  
>> CODEC_FLAG2_8X8DCT;
>> -    x4->params.analyse.b_fast_pskip = avctx->flags2 &  
>> CODEC_FLAG2_FASTPSKIP;
>> +        avctx->flags2 & CODEC_FLAG2_MIXED_REFS ? 1 : 0;
>> +    x4->params.analyse.b_chroma_me = avctx->me_cmp &  
>> FF_CMP_CHROMA ? 1 : 0;
>> +    x4->params.analyse.b_transform_8x8 = avctx->flags2 &  
>> CODEC_FLAG2_8X8DCT ? 1 : 0;
>> +    x4->params.analyse.b_fast_pskip = avctx->flags2 &  
>> CODEC_FLAG2_FASTPSKIP ? 1 : 0;
>>
>>     x4->params.analyse.i_trellis = avctx->trellis;
>>     x4->params.analyse.i_noise_reduction = avctx->noise_reduction;
>> @@ -267,6 +267,13 @@
>>         x4->params.b_repeat_headers = 0;
>>     }

This applies to all the above: I think !!(flag & FLAG) would be  
preferred to the ternary stuff. And regardless, are they actually  
needed? Or is this the cosmetic stuff you mention?

>>
>>
>> +    // x4->params.analyse.b_ssim = 0;
>> +    x4->params.analyse.b_psnr = avctx->flags & CODEC_FLAG_PSNR ?  
>> 1 : 0;
>> +
>> +    /* char *res = (char *) x264_param2string(&x4->params, 0);
>> +    av_log(avctx, AV_LOG_INFO, "Options: %s", res);
>> +    x264_free(res); */
>> +
>>     x4->enc = x264_encoder_open(&x4->params);
>>     if(!x4->enc)
>>         return -1;
>
> Why libx264 doesn't already set all those values to 0 by default?

Why is this commented code being added?

>> Index: libavcodec/avcodec.h
>> ===================================================================
>> --- libavcodec/avcodec.h	(revisione 15383)
>> +++ libavcodec/avcodec.h	(copia locale)
>> @@ -2103,7 +2103,7 @@
>> #define X264_PART_B8X8 0x100  /* Analyze b16x8, b8x16 and b8x8 */
>>
>>     /**
>> -     * direct MV prediction mode - 0 (none), 1 (spatial), 2  
>> (temporal)
>> +     * direct MV prediction mode - 0 (none), 1 (spatial), 2  
>> (temporal), 3 (auto)
>>      * - encoding: Set by user.
>>      * - decoding: unused
>>      */
>
> Looks OK, but I prefer to leave it to someone else with expertise in
> libx264.

Yes, that's OK.

>> Index: ffpresets/libx264-fastfirstpass.ffpreset
>> ===================================================================
>> --- ffpresets/libx264-fastfirstpass.ffpreset	(revisione 15383)
>> +++ ffpresets/libx264-fastfirstpass.ffpreset	(copia locale)
>> @@ -2,7 +2,7 @@
>> flags=+loop
>> cmp=+chroma
>> partitions=-parti8x8-parti4x4-partp8x8-partp4x4-partb8x8
>> -me=dia
>> +me_method=dia
>> subq=1
>> me_range=16
>> g=250
>> Index: ffpresets/libx264-default.ffpreset
>> ===================================================================
>> --- ffpresets/libx264-default.ffpreset	(revisione 15383)
>> +++ ffpresets/libx264-default.ffpreset	(copia locale)
>> @@ -2,7 +2,7 @@
>> flags=+loop
>> cmp=+chroma
>> partitions=+parti8x8+parti4x4+partp8x8+partb8x8
>> -me=hex
>> +me_method=hex
>> subq=5
>> me_range=16
>> g=250
>> Index: ffpresets/libx264-max.ffpreset
>> ===================================================================
>> --- ffpresets/libx264-max.ffpreset	(revisione 15383)
>> +++ ffpresets/libx264-max.ffpreset	(copia locale)
>> @@ -2,7 +2,7 @@
>> flags=+loop
>> cmp=+chroma
>> partitions=+parti8x8+parti4x4+partp8x8+partp4x4+partb8x8
>> -me=tesa
>> +me_method=tesa
>> subq=7
>> me_range=32
>> g=250
>> Index: ffpresets/libx264-hq.ffpreset
>> ===================================================================
>> --- ffpresets/libx264-hq.ffpreset	(revisione 15383)
>> +++ ffpresets/libx264-hq.ffpreset	(copia locale)
>> @@ -2,7 +2,7 @@
>> flags=+loop
>> cmp=+chroma
>> partitions=+parti8x8+parti4x4+partp8x8+partb8x8
>> -me=umh
>> +me_method=umh
>> subq=7
>> me_range=16
>> g=250
>> Index: ffpresets/libx264-normal.ffpreset
>> ===================================================================
>> --- ffpresets/libx264-normal.ffpreset	(revisione 15383)
>> +++ ffpresets/libx264-normal.ffpreset	(copia locale)
>> @@ -2,7 +2,7 @@
>> flags=+loop
>> cmp=+chroma
>> partitions=+parti8x8+parti4x4+partp8x8+partb8x8
>> -me=hex
>> +me_method=hex
>> subq=6
>> me_range=16
>> g=250
>
> This is surely correct, I'll apply in few days with the first one if
> no one objects.

The above are OK, as long as they've been tested working. I hadn't  
noticed the 'me' option had been removed. :)

I suppose I could add myself as maintainer of ffpreset files, if  
everyone else is happy with that.

Regards,
Rob