On Wed, Apr 27, 2011 at 10:49 PM, Anton Khirnov <[email protected]> wrote:
> On Wed, 20 Apr 2011 15:53:48 -0700, Alex Converse <[email protected]>
> wrote:
>> diff --git a/cmdutils.c b/cmdutils.c
>> index f1cbd55..4aef03d 100644
>> --- a/cmdutils.c
>> +++ b/cmdutils.c
>> @@ -361,8 +361,9 @@ void set_context_opts(void *ctx, void *opts_ctx, int
>> flags, AVCodec *codec)
>> /* We need to use a differnt system to pass options to the private
>> context because
>> it is not known which codec and thus context kind that will be
>> when parsing options
>> we thus use opt_values directly instead of opts_ctx */
>> - if(!str && priv_ctx && av_get_string(priv_ctx, opt_names[i], &opt,
>> buf, sizeof(buf))){
>> - av_set_string3(priv_ctx, opt_names[i], opt_values[i], 1, NULL);
>> + if(!str && priv_ctx) {
>> + if (av_find_opt(priv_ctx, opt_names[i], NULL, flags, flags))
>> + av_set_string3(priv_ctx, opt_names[i], opt_values[i], 0,
>> NULL);
>> }
>> }
>
> This part looks unrelated.
>
I will fold it into 2/6
>> }
>> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>> index 5825945..df4c577 100644
>> --- a/libavcodec/libx264.c
>> +++ b/libavcodec/libx264.c
>> @@ -19,6 +19,7 @@
>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
>> USA
>> */
>>
>> +#include "libavutil/opt.h"
>> #include "avcodec.h"
>> #include <x264.h>
>> #include <math.h>
>> @@ -27,12 +28,17 @@
>> #include <string.h>
>>
>> typedef struct X264Context {
>> + AVClass *class;
>> x264_param_t params;
>> x264_t *enc;
>> x264_picture_t pic;
>> uint8_t *sei;
>> int sei_size;
>> AVFrame out_pic;
>> + const char *preset;
>> + const char *tune;
>> + const char *profile;
>> + int fastfirstpass;
>
> The strings are leaked.
>
I'm not sure where in the options goop this is freed but if I free it
myself I get a double free.
>> } X264Context;
>>
>> static void X264_log(void *p, int level, const char *fmt, va_list args)
>> @@ -163,32 +169,7 @@ static av_cold int X264_init(AVCodecContext *avctx)
>> x4->sei_size = 0;
>> x264_param_default(&x4->params);
>>
>> - x4->params.pf_log = X264_log;
>> - x4->params.p_log_private = avctx;
>> -
>> x4->params.i_keyint_max = avctx->gop_size;
>> - x4->params.b_intra_refresh = avctx->flags2 &
>> CODEC_FLAG2_INTRA_REFRESH;
>> - x4->params.rc.i_bitrate = avctx->bit_rate / 1000;
>> - x4->params.rc.i_vbv_buffer_size = avctx->rc_buffer_size / 1000;
>> - x4->params.rc.i_vbv_max_bitrate = avctx->rc_max_rate / 1000;
>> - x4->params.rc.b_stat_write = avctx->flags & CODEC_FLAG_PASS1;
>> - if (avctx->flags & CODEC_FLAG_PASS2) {
>> - x4->params.rc.b_stat_read = 1;
>> - } else {
>> - if (avctx->crf) {
>> - x4->params.rc.i_rc_method = X264_RC_CRF;
>> - x4->params.rc.f_rf_constant = avctx->crf;
>> - x4->params.rc.f_rf_constant_max = avctx->crf_max;
>> - } else if (avctx->cqp > -1) {
>> - x4->params.rc.i_rc_method = X264_RC_CQP;
>> - x4->params.rc.i_qp_constant = avctx->cqp;
>> - }
>> - }
>> -
>> - // if neither crf nor cqp modes are selected we have to enable the RC
>> - // we do it this way because we cannot check if the bitrate has been set
>> - if (!(avctx->crf || (avctx->cqp > -1)))
>> - x4->params.rc.i_rc_method = X264_RC_ABR;
>>
>> x4->params.i_bframe = avctx->max_b_frames;
>> x4->params.b_cabac = avctx->coder_type == FF_CODER_TYPE_AC;
>> @@ -217,13 +198,6 @@ static av_cold int X264_init(AVCodecContext *avctx)
>>
>> x4->params.i_frame_reference = avctx->refs;
>>
>> - x4->params.i_width = avctx->width;
>> - x4->params.i_height = avctx->height;
>> - x4->params.vui.i_sar_width = avctx->sample_aspect_ratio.num;
>> - x4->params.vui.i_sar_height = avctx->sample_aspect_ratio.den;
>> - x4->params.i_fps_num = x4->params.i_timebase_den = avctx->time_base.den;
>> - x4->params.i_fps_den = x4->params.i_timebase_num = avctx->time_base.num;
>> -
>> x4->params.analyse.inter = 0;
>> if (avctx->partitions) {
>> if (avctx->partitions & X264_PART_I4X4)
>> @@ -277,20 +251,65 @@ static av_cold int X264_init(AVCodecContext *avctx)
>> if (avctx->level > 0)
>> x4->params.i_level_idc = avctx->level;
>>
>> + x4->params.rc.b_mb_tree = !!(avctx->flags2 &
>> CODEC_FLAG2_MBTREE);
>> + x4->params.rc.f_ip_factor = 1 / fabs(avctx->i_quant_factor);
>> + x4->params.rc.f_pb_factor = avctx->b_quant_factor;
>> + x4->params.analyse.i_chroma_qp_offset = avctx->chromaoffset;
>> +
>
> So those parameters can be overwritten by preset/profile? Do we really
> want that?
>
[See below]
>> + if (x4->preset || x4->tune) {
>> + if (x264_param_default_preset(&x4->params, x4->preset, x4->tune) <
>> 0)
>> + return -1;
>> + }
>> +
>> + if (x4->fastfirstpass)
>> + x264_param_apply_fastfirstpass(&x4->params);
>> +
>> + if (x4->profile)
>> + if (x264_param_apply_profile(&x4->params, x4->profile) < 0)
>> + return -1;
>> +
>> + x4->params.pf_log = X264_log;
>> + x4->params.p_log_private = avctx;
>> + x4->params.i_log_level = X264_LOG_DEBUG;
>> +
>> + x4->params.b_intra_refresh = avctx->flags2 &
>> CODEC_FLAG2_INTRA_REFRESH;
>> + x4->params.rc.i_bitrate = avctx->bit_rate / 1000;
>> + x4->params.rc.i_vbv_buffer_size = avctx->rc_buffer_size / 1000;
>> + x4->params.rc.i_vbv_max_bitrate = avctx->rc_max_rate / 1000;
>> + x4->params.rc.b_stat_write = avctx->flags & CODEC_FLAG_PASS1;
>> + if (avctx->flags & CODEC_FLAG_PASS2) {
>> + x4->params.rc.b_stat_read = 1;
>> + } else {
>> + if (avctx->crf) {
>> + x4->params.rc.i_rc_method = X264_RC_CRF;
>> + x4->params.rc.f_rf_constant = avctx->crf;
>> + x4->params.rc.f_rf_constant_max = avctx->crf_max;
>> + } else if (avctx->cqp > -1) {
>> + x4->params.rc.i_rc_method = X264_RC_CQP;
>> + x4->params.rc.i_qp_constant = avctx->cqp;
>> + }
>> + }
>> +
>> + // if neither crf nor cqp modes are selected we have to enable the RC
>> + // we do it this way because we cannot check if the bitrate has been set
>> + if (!(avctx->crf || (avctx->cqp > -1)))
>> + x4->params.rc.i_rc_method = X264_RC_ABR;
>> +
>> if (avctx->rc_buffer_size && avctx->rc_initial_buffer_occupancy &&
>> (avctx->rc_initial_buffer_occupancy <= avctx->rc_buffer_size)) {
>> x4->params.rc.f_vbv_buffer_init =
>> (float)avctx->rc_initial_buffer_occupancy /
>> avctx->rc_buffer_size;
>> }
>>
>> - x4->params.rc.b_mb_tree = !!(avctx->flags2 &
>> CODEC_FLAG2_MBTREE);
>> - x4->params.rc.f_ip_factor = 1 / fabs(avctx->i_quant_factor);
>> - x4->params.rc.f_pb_factor = avctx->b_quant_factor;
>> - x4->params.analyse.i_chroma_qp_offset = avctx->chromaoffset;
>> + x4->params.i_width = avctx->width;
>> + x4->params.i_height = avctx->height;
>> + x4->params.vui.i_sar_width = avctx->sample_aspect_ratio.num;
>> + x4->params.vui.i_sar_height = avctx->sample_aspect_ratio.den;
>> + x4->params.i_fps_num = x4->params.i_timebase_den = avctx->time_base.den;
>> + x4->params.i_fps_den = x4->params.i_timebase_num = avctx->time_base.num;
>>
>> x4->params.analyse.b_psnr = avctx->flags & CODEC_FLAG_PSNR;
>> x4->params.analyse.b_ssim = avctx->flags2 & CODEC_FLAG2_SSIM;
>> - x4->params.i_log_level = X264_LOG_DEBUG;
>>
>> x4->params.b_aud = avctx->flags2 & CODEC_FLAG2_AUD;
>>
>> @@ -305,6 +324,11 @@ static av_cold int X264_init(AVCodecContext *avctx)
>> if (avctx->flags & CODEC_FLAG_GLOBAL_HEADER)
>> x4->params.b_repeat_headers = 0;
>>
>> + // update AVCodecContext with x264 parameters
>> + avctx->has_b_frames = x4->params.i_bframe_pyramid ? 2 :
>> !!x4->params.i_bframe;
>> + avctx->bit_rate = x4->params.rc.i_bitrate*1000;
>> + avctx->crf = x4->params.rc.f_rf_constant;
>> +
>
> I'm wondering what's all this random shuffling for. It also seems to
> disagree with recommendation in x264.h:
> In order to replicate x264CLI's option handling, these functions MUST
> be called in the following order:
> 1) x264_param_default_preset
> 2) Custom user options (via param_parse or directly assigned
> variables)
> 3) x264_param_apply_fastfirstpass
> 4) x264_param_apply_profile
>
I part of the problem we run into related to option ordering is that
we don't have a concept of unset in AVOptions so we wind up clobbering
everything with (broken) defaults.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
