[FFmpeg-devel] [PATCH] Add example reencoding_filtered_video.c
andrey.krieger.utkin at gmail.com
Thu Jan 30 16:20:37 CET 2014
2014-01-30 Nicolas George <george at nsup.org>:
>> -CFLAGS += -Wall -g
>> +CFLAGS += -Wall -g -O0 -ggdb
> Looks unrelated.
Indeed. Removed. Thanks.
>> + av_opt_set_int(dec_ctx, "refcounted_frames", 1, 0);
> Maybe add a comment to explain why it is useful.
This just was there. Entire file was derived from filtering_video.c
I haven't care much about this option being here. To comment that,
would it be reasonable to copy AVCodecContext.refcounted_frames
>> + avformat_alloc_output_context2(&ofmt_ctx, NULL, NULL, filename);
> ofmt_ctx needs to be NULL before that call. It is true because it is a
> static global, but it is not explicit. I would suggest to add ofmt_ctx=NULL
> just before, even though it is not used.
>> + av_dict_set(&encoder_opts, "b", "100000", 0); /* set bitrate if needed */
> For single pass encoding to file, "crf" would be more appropriate.
AFAIK these are just two different approaches - average bitrate target
vs. target quality. Also "crf" is libx264-specific option, while "b"
is common option.
I see no critical difference which approach to show in this example.
H264 case is shown there just because person who asked me to make up
such example is interested primarily in H264 output. So if anyone has
an idea how to make this demo more generalized without adding much of
complexity, you are welcome.
>> + snprintf(args, sizeof(args),
>> + "video_size=%dx%d:pix_fmt=%d:time_base=%d/%d:pixel_aspect=%d/%d",
>> + dec_ctx->width, dec_ctx->height, dec_ctx->pix_fmt,
>> + dec_ctx->time_base.num, dec_ctx->time_base.den,
>> + dec_ctx->sample_aspect_ratio.num, dec_ctx->sample_aspect_ratio.den);
>> + ret = avfilter_graph_create_filter(&buffersrc_ctx, buffersrc, "in",
>> + args, NULL, filter_graph);
> In this case, you serialize the options into a string, and below, for the
> output, you use av_opt_set. If you want to illustrate the two possibilities,
> please add a comment to explain. And if not, better use options everywhere.
Again, this piece came mostly unchanged from filtering_video.c
I checked how should it be changed to work through av_opt_set, and it
came out that would just take value-to-string conversion for each
individual option. So brevity of code may be reason to use combined
options string. The same way is used in ffplay.c and ffmpeg_filter.c.
I'm not against adding some comment, but i have no deep and recent
practice with setting options on filtering stuff, so i'd thank for
comment text suggestion.
BTW buffer source doc is somewhat inactual: "This source is mainly
intended for a programmatic use, in particular through the interface
defined in 'libavfilter/vsrc_buffer.h'."
There's no such file vstc_buffer.h now.
>> + outputs->name = av_strdup("in");
> Here and at a few other places: unchecked memory allocation.
Again, it's same as in filtering_video.c.
Added assert() check for likely everything around.
>> + if ((ret = avfilter_graph_parse_ptr(filter_graph, filters_descr,
>> + &inputs, &outputs, NULL)) < 0)
Thank you, fixed.
>> + got_frame = 0;
> I believe it is unneeded.
Indeed. Removed. This is in filtering_video.c, too.
>> + frame = av_frame_alloc();
> Unchecked memory allocation.
>> + ret = av_buffersrc_add_frame_flags(buffersrc_ctx, frame, AV_BUFFERSRC_FLAG_KEEP_REF);
>> + av_frame_free(&frame);
> Any reason to set the KEEP_REF flag only to free the frame immediately
Do i understand you right that these two lines can be replaced with
+ ret = av_buffersrc_add_frame_flags(buffersrc_ctx, frame, 0);
> You need to flush the filters and encoder.
> To flush the filters, you call av_buffersrc_add_frame_flags() with frame =
> NULL, and then you pull frames from the buffersink until it returns EOF.
> To flush the encoder, you test for CODEC_CAP_DELAY and call
> avcodec_encode_video2() with frame = NULL until it stops returning packets.
Thanks for reminder, will add.
More information about the ffmpeg-devel