[FFmpeg-devel] [RFC] Specifying KEYINT (-g) has no effect in libx264

[Baptiste Coudurier]

On 05/26/2011 08:18 AM, Stefano Sabatini wrote:
> On date Thursday 2011-05-26 01:45:30 +0200, Etienne Buira encoded:
>> On Mon, May 23, 2011 at 01:32:15AM -0700, Baptiste Coudurier wrote:
>>> On 5/22/11 8:33 PM, Michael Niedermayer wrote:
>>>> On Sun, May 22, 2011 at 10:40:07PM +0200, Stefano Sabatini wrote:
>>>>> Hi,
>>>>>
>>>>> see issue #157.
>>>>>
>>>>> The problem was possibly introduced in:
>>>>>
>>>>> commit 0140d3f0921e5cbb6ea8706acb0307f7ff57a133
>>>>> Author: Baptiste Coudurier <baptiste.coudurier at gmail.com>
>>>>> Date:   Sat Apr 16 16:50:50 2011 -0700
>>>>>
>>>>>     In libx264 wrapper, add -preset and -tune options
>>>>>
>>>>> and depends on the fact that x264_param_default_preset() which is
>>>>> called in x264_init(), calls x264_param_default() which defaults the
>>>>> already set options.
>>>>>
>>>>> Indeed order of options matters, with the current mechanism options
>>>>> are set in the relevant contexts, and globally processed in the object
>>>>> initialization, so the order specified on the commandline is basically
>>>>> ignored.
>>>>>
>>>>> In the case of libx264 we may process profile/preset *before* the
>>>>> other options set in the context, so that specific settings will
>>>>> override profile/preset information.
>>>
>>> No, order doesn't matter, as long as a preset is specified, it will
>>> override other options.
>>>
>>>>> This has the problem that most of the times user-set options are
>>>>> changed to medium profile or libx264 will complain&exit, see:
>>>>>
>>>>> thus making fine-tuned user options pointless (since they are changed
>>>>> to medium preset and/or will be rejected by libx264).
>>>
>>> No, you can still specify fine-tuned options without a preset.
>>>
>>>> I see 3 obvious solutions
>>>>
>>>> A. is mplayers to just use -x264opts which ive added
>>>>
>>>> B. keep track of which options have been set by the user and which are
>>>>    default.
>>>
>>> 'B' is how it should be done. It's trivial, just remap "g" in the
>>> libx264.c the way weightp was mapped.
>>
>> Hi all.
>>
>> Patch attached.
>>
>> But now, I think it would be better to keep track of what options have
>> been set at libavutil level. I don't know if ffmpeg would benefit a lot
>> from that, but certainly libavutil.
>> 
>> [...]
>>
>> +    av_free(x4->aud);
>> +    av_free(x4->interlaced);
>> +    av_free(x4->opengop);
>> +    av_free(x4->repeatheaders);
> 
> Note: we need a function for freeing the private context strings
>   

Yes, the private options array can probably be used if the option type
is OPT_STRING.

> 
>> +    /* Should be cleaned out */
>> +    avctx->has_b_frames          = avctx->flags2 & CODEC_FLAG2_BPYRAMID ? 2 : !!avctx->max_b_frames;
> 
> Please can you explain the meaning of the comment? Also shouldn't this
> set in the framework (that is: what's libx264 specific here?).

Only h.264 supports bypyramid.

> 
> Uhm so the idea is that options are set in the private context.
> 

Yes.

> 
> The idea of removing the libx264 options from AVCodecContext is *good*
> (TM), theoretically this involves an API break but then practically
> should not be a problem.
> 
> So basically the idea is this:
> * set libx264 defaults in x4->params
> * if an option has been set in the private context (that is if the
>   user specified them) set it in the x4->params context. This is
>   needed if there are global FFmpeg options which are mapped to
>   specific libx264 options.
> 
> For other libx264 options which don't have a corresponding in the
> global context there is the possibility to either use x264params
> (preferred?) or specify the option in the private context.
> 
> This way libx264 defaults override the FFmpeg defaults.
> 
> Is my understanding correct?

Yes.

-- 
Baptiste COUDURIER
Key fingerprint          8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                           http://www.ffmpeg.org