[FFmpeg-devel] [PATCH] smptebars filter

[Stefano Sabatini]

On date Wednesday 2012-06-20 16:59:26 +0000, Paul B Mahol encoded:
> On 6/20/12, Stefano Sabatini <stefasab at gmail.com> wrote:
[...]
> > This code seems the duplicated of that present in vsrc_testsrc.c,
> > should be possible to easily reuse that (and documentation) for free.
> 
> As most of other filters.

Uhm no, of other *test sources*, which mostly have the same options
(size, rate, sample aspect ratio, duration) and need a function which
fills a picture. In case you wonder the color source could be moved
there, but I didn't because it supported a different syntax.

[...]
> >> +static int request_frame(AVFilterLink *outlink)
> >> +{
> >> +    SMPTEBarsContext *smpte = outlink->src->priv;
> >> +    AVFilterBufferRef *picref;
> >> +
> >> +    picref = ff_get_video_buffer(outlink, AV_PERM_WRITE, smpte->w,
> >> smpte->h);
> >> +    picref->video->sample_aspect_ratio = smpte->sar;
> >> +
> >> +    ff_start_frame(outlink, avfilter_ref_buffer(picref, ~0));
> >> +    draw_smptebars(outlink->src, picref);
> >> +    ff_draw_slice(outlink, 0, picref->video->h, 1);
> >> +    ff_end_frame(outlink);
> >> +    avfilter_unref_buffer(picref);
> >> +    return 0;
> >> +}
> >
> > Again this can be dropped in favor of the common code in vsrc_testsrc.c.
> 
> You mean to move this filter into that file or exporting function?

Move the required pieces to vsrc_testsrc.c (query_formats, init,
fill_picture and the filter structure definition).

> >
> > Also, how are you setting the pts?
> 
> Ah, I forgot to copy that one ;-)
> >
> >> +
> >> +static int config_props(AVFilterLink *outlink)
> >> +{
> >> +    AVFilterContext *ctx = outlink->src;
> >> +    SMPTEBarsContext *smpte = ctx->priv;
> >> +
> >> +    ff_draw_init(&smpte->draw, outlink->format, 0);
> >> +
> >
> >> +    if (av_image_check_size(smpte->w, smpte->h, 0, ctx) < 0)
> >> +        return AVERROR(EINVAL);
> >
> > Is this really required? I wonder becasue I never used it, if required
> > the check should be moved to the internal code.
> 
> It is copied from vsrc_color, and few other filters have it.
> 

> But isn't this checked when w & h values are read?

No the parsing function doesn't assume that the size will be used
necessarily for setting the size of a libav* image, so it won't abort
(and rightly so).

IMO the check should be moved up in the framework, but feel free to
keep the check in your code (or simply drop it), it is an unrelated
problem.
-- 
FFmpeg = Furious Fanciful Merciless Purposeless Exciting God