[FFmpeg-devel] [PATCH 8/9] ffplay: move video filtering graph in the VideoContext

[Marton Balint]

On Fri, 22 Jun 2012, Stefano Sabatini wrote:

> Make room for some factorization, improve consistency.

I'm actually not a big fan of moving stuff into VideoState. Graph is a 
video thread local variable. Also allocating and freeing it in the same 
function is probably better than hiding the allocation inside configure 
video filters. If we use the videoState variables everywhere it will make 
to code less reusable.

So I'd rather not change this only to make it consistent with audio. Maybe 
making audio consistent with this should be the goal. Or is there a place 
where this version has some real advantage?

Regards,
Marton

> ---
> ffplay.c |   29 +++++++++++++++--------------
> 1 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/ffplay.c b/ffplay.c
> index 2569202..cb234f3 100644
> --- a/ffplay.c
> +++ b/ffplay.c
> @@ -230,6 +230,7 @@ typedef struct VideoState {
> #if CONFIG_AVFILTER
>     AVFilterContext *in_video_filter;           ///< the first filter in the video chain
>     AVFilterContext *out_video_filter;          ///< the last filter in the video chain
> +    AVFilterGraph *graph;
>     int use_dr1;
>     FrameBuffer *buffer_pool;
> #endif
> @@ -1547,7 +1548,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame, int64_t *pts, AVPacke
> }
>
> #if CONFIG_AVFILTER
> -static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const char *vfilters)
> +static int configure_video_filters(VideoState *is, const char *vfilters)
> {
>     static const enum PixelFormat pix_fmts[] = { PIX_FMT_YUV420P, PIX_FMT_NONE };
>     char sws_flags_str[128];
> @@ -1557,8 +1558,10 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>     AVFilterContext *filt_src = NULL, *filt_out = NULL, *filt_format;
>     AVCodecContext *codec = is->video_st->codec;
>
> +    is->graph = avfilter_graph_alloc();
> +
>     snprintf(sws_flags_str, sizeof(sws_flags_str), "flags=%d", sws_flags);
> -    graph->scale_sws_opts = av_strdup(sws_flags_str);
> +    is->graph->scale_sws_opts = av_strdup(sws_flags_str);
>
>     snprintf(buffersrc_args, sizeof(buffersrc_args),
>              "video_size=%dx%d:pix_fmt=%d:time_base=%d/%d:pixel_aspect=%d/%d",
> @@ -1569,18 +1572,18 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>     if ((ret = avfilter_graph_create_filter(&filt_src,
>                                             avfilter_get_by_name("buffer"),
>                                             "ffplay_buffer", buffersrc_args, NULL,
> -                                            graph)) < 0)
> +                                            is->graph)) < 0)
>         return ret;
>
> #if FF_API_OLD_VSINK_API
>     ret = avfilter_graph_create_filter(&filt_out,
>                                        avfilter_get_by_name("buffersink"),
> -                                       "ffplay_buffersink", NULL, (void *)pix_fmts, graph);
> +                                       "ffplay_buffersink", NULL, (void *)pix_fmts, is->graph);
> #else
>     buffersink_params->pixel_fmts = pix_fmts;
>     ret = avfilter_graph_create_filter(&filt_out,
>                                        avfilter_get_by_name("buffersink"),
> -                                       "ffplay_buffersink", NULL, buffersink_params, graph);
> +                                       "ffplay_buffersink", NULL, buffersink_params, is->graph);
> #endif
>     av_freep(&buffersink_params);
>     if (ret < 0)
> @@ -1588,7 +1591,7 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>
>     if ((ret = avfilter_graph_create_filter(&filt_format,
>                                             avfilter_get_by_name("format"),
> -                                            "format", "yuv420p", NULL, graph)) < 0)
> +                                            "format", "yuv420p", NULL, is->graph)) < 0)
>         return ret;
>     if ((ret = avfilter_link(filt_format, 0, filt_out, 0)) < 0)
>         return ret;
> @@ -1608,14 +1611,14 @@ static int configure_video_filters(AVFilterGraph *graph, VideoState *is, const c
>         inputs->pad_idx = 0;
>         inputs->next    = NULL;
>
> -        if ((ret = avfilter_graph_parse(graph, vfilters, &inputs, &outputs, NULL)) < 0)
> +        if ((ret = avfilter_graph_parse(is->graph, vfilters, &inputs, &outputs, NULL)) < 0)
>             return ret;
>     } else {
>         if ((ret = avfilter_link(filt_src, 0, filt_format, 0)) < 0)
>             return ret;
>     }
>
> -    if ((ret = avfilter_graph_config(graph, NULL)) < 0)
> +    if ((ret = avfilter_graph_config(is->graph, NULL)) < 0)
>         return ret;
>
>     is->in_video_filter  = filt_src;
> @@ -1642,13 +1645,12 @@ static int video_thread(void *arg)
>     int ret;
>
> #if CONFIG_AVFILTER
> -    AVFilterGraph *graph = avfilter_graph_alloc();
>     AVFilterContext *filt_out = NULL, *filt_in = NULL;
>     int last_w = is->video_st->codec->width;
>     int last_h = is->video_st->codec->height;
>     enum PixelFormat last_format = is->video_st->codec->pix_fmt;
>
> -    if ((ret = configure_video_filters(graph, is, vfilters)) < 0) {
> +    if ((ret = configure_video_filters(is, vfilters)) < 0) {
>         SDL_Event event;
>         event.type = FF_QUIT_EVENT;
>         event.user.data1 = is;
> @@ -1684,9 +1686,8 @@ static int video_thread(void *arg)
>             || last_format != is->video_st->codec->pix_fmt) {
>             av_log(NULL, AV_LOG_INFO, "Frame changed from size:%dx%d to size:%dx%d\n",
>                    last_w, last_h, is->video_st->codec->width, is->video_st->codec->height);
> -            avfilter_graph_free(&graph);
> -            graph = avfilter_graph_alloc();
> -            if ((ret = configure_video_filters(graph, is, vfilters)) < 0) {
> +            avfilter_graph_free(&is->graph);
> +            if ((ret = configure_video_filters(is, vfilters)) < 0) {
>                 av_free_packet(&pkt);
>                 goto the_end;
>             }
> @@ -1765,7 +1766,7 @@ static int video_thread(void *arg)
>     avcodec_flush_buffers(is->video_st->codec);
> #if CONFIG_AVFILTER
>     av_freep(&vfilters);
> -    avfilter_graph_free(&graph);
> +    avfilter_graph_free(&is->graph);
> #endif
>     av_free(frame);
>     return 0;
> -- 
> 1.7.5.4
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>