[FFmpeg-devel] [PATCH] telecine filter
Paul B Mahol
onemda at gmail.com
Mon Apr 8 04:10:07 CEST 2013
On 4/8/13, Clement Boesch <ubitux at gmail.com> wrote:
> On Mon, Apr 08, 2013 at 12:33:00AM +0000, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>> ---
>> Changelog | 1 +
>> doc/filters.texi | 38 +++++++
>> libavfilter/Makefile | 1 +
>> libavfilter/allfilters.c | 1 +
>> libavfilter/version.h | 2 +-
>> libavfilter/vf_telecine.c | 247
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 289 insertions(+), 1 deletion(-)
>> create mode 100644 libavfilter/vf_telecine.c
>>
>> diff --git a/Changelog b/Changelog
>> index 5faa414..bee29c3 100644
>> --- a/Changelog
>> +++ b/Changelog
>> @@ -16,6 +16,7 @@ version <next>:
>> filtergraph description to be read from a file
>> - OpenCL support
>> - audio phaser filter
>> +- telecine filter
>>
>>
>> version 1.2:
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index 483d8a1..9093bf9 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -428,6 +428,44 @@ slope
>> Determine how steep is the filter's shelf transition.
>> @end table
>>
>> + at section telecine
>> +
>> +Apply telecine process to the video.
>> +
>> +The filter accepts parameters as a list of @var{key}=@var{value}
>> +pairs, separated by ":".
>> +
>> +A description of the accepted parameters follows.
>> +
>> + at table @option
>> + at item first_field
>
> Maybe "Select the first field. Available values are:"
>
>> + at table @option
>
> @table @samp
>
>> + at item t
>> +top field first
>> + at item b
>> +bottom field first
>> + at end table
>> +
>
> What is the default?
Added.
>
> Also, is it really problematic to use "top" and "bottom" instead of one
> letter?
Added.
>
>> + at item pattern
>> +String representing for how many fields a frame is to be displayed.
>> + at end table
>> +
>> +Some typical patterns:
>> +
>> +NTSC output (30i):
>> +27.5p: 32222
>> +24p: 23 (classic)
>> +24p: 2332 (preferred)
>> +20p: 33
>> +18p: 334
>> +16p: 3444
>> +
>> +PAL output (25i):
>> +27.5p: 12222
>> +24p: 222222222223 ("Euro pulldown")
>> +16.67p: 33
>> +16p: 33333334
>> +
>
> A sentence or two about the pattern would be welcome.
>
> I also see no note about the pts you are resetting. BTW, I still don't get
> why you are not setting them in the filter itself.
>
> [...]
>> +#define OFFSET(x) offsetof(TelecineContext, x)
>> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
>> +
>> +static const AVOption telecine_options[] = {
>> + {"first_field", "", OFFSET(first_field), AV_OPT_TYPE_INT, {.i64=0},
>> 0, 1, FLAGS, "field"},
>
> "select first field" or something as description.
Changed.
>
>> + {"t", "top", 0, AV_OPT_TYPE_CONST, {.i64=0}, 0, 0, FLAGS,
>> "field"},
>> + {"b", "bottom", 0, AV_OPT_TYPE_CONST, {.i64=1}, 0, 0, FLAGS,
>> "field"},
>
> "top", "top field first"
> "bottom", "bottom field first'
Fixed.
>
>> + {"pattern", "pattern that describe for how many fields a frame is to
>> be displayed", OFFSET(pattern), AV_OPT_TYPE_STRING, {.str="23"}, 0, 0,
>> FLAGS},
>> + {NULL}
>> +};
>> +
>> +AVFILTER_DEFINE_CLASS(telecine);
>> +
>> +static av_cold int init(AVFilterContext *ctx, const char *args)
>> +{
>> + TelecineContext *tc = ctx->priv;
>> + const char *p;
>> + int max = 0;
>> +
>> + for (p = tc->pattern; *p; p++)
>> + if (*p < '0' || *p > '9') {
>
> av_isdigit()
Fixed.
>
> Also, you can include this check in the following loop.
Done.
>
>> + av_log(ctx, AV_LOG_ERROR, "invalid pattern\n");
>> + return AVERROR(EINVAL);
>> + }
>> +
>> + for (p = tc->pattern; *p; p++) {
>> + max = FFMAX(*p - '0', max);
>> + tc->pts.num += 2;
>> + tc->pts.den += *p - '0';
>> + }
>> +
>
> What if the string is empty?
Fixed.
>
> Now, what if p is "0"? Isn't this going to /0 somewhere?
It will crash.
>
>> + tc->out_cnt = (max + 1) / 2;
>> + av_log(ctx, AV_LOG_INFO,
>> + "telecine pattern %s yields up to %d frames per frame, pts
>> advance factor: %d/%d\n",
>> + tc->pattern, tc->out_cnt, tc->pts.num, tc->pts.den);
>> +
>
> nit: align
>
>> + return 0;
>> +}
>> +
>> +static int query_formats(AVFilterContext *ctx)
>> +{
>> + AVFilterFormats *pix_fmts = NULL;
>> + int fmt;
>> +
>> + for (fmt = 0; fmt < AV_PIX_FMT_NB; fmt++) {
>> + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(fmt);
>> + if (!(desc->flags & PIX_FMT_HWACCEL))
>> + ff_add_format(&pix_fmts, fmt);
>> + }
>> +
>
> So gray, rgb, alpha variants, 10-bits yuv, ... are supported?
It just copy some "random" strides, so virtually everything sane
enough is supported.
>
>> + ff_set_common_formats(ctx, pix_fmts);
>> + return 0;
>> +}
>> +
>> +static int config_input(AVFilterLink *inlink)
>> +{
>> + TelecineContext *tc = inlink->dst->priv;
>> + const AVPixFmtDescriptor *desc =
>> av_pix_fmt_desc_get(inlink->format);
>> + int i, ret;
>> +
>> + tc->temp = ff_get_video_buffer(inlink, inlink->w, inlink->h);
>> + if (!tc->temp)
>> + return AVERROR(ENOMEM);
>> + for (i = 0; i < tc->out_cnt; i++) {
>> + tc->frame[i] = ff_get_video_buffer(inlink, inlink->w,
>> inlink->h);
>> + if (!tc->frame[i])
>> + return AVERROR(ENOMEM);
>> + }
>> +
>> + if ((ret = av_image_fill_linesizes(tc->stride, inlink->format,
>> inlink->w)) < 0)
>> + return ret;
>> +
>> + tc->planeheight[1] = tc->planeheight[2] = inlink->h >>
>> desc->log2_chroma_h;
>> + tc->planeheight[0] = tc->planeheight[3] = inlink->h;
>> +
>> + tc->nb_planes = av_pix_fmt_count_planes(inlink->format);
>> +
>> + return 0;
>> +}
>> +
>> +static int filter_frame(AVFilterLink *inlink, AVFrame *inpicref)
>> +{
>> + AVFilterContext *ctx = inlink->dst;
>> + AVFilterLink *outlink = ctx->outputs[0];
>> + TelecineContext *tc = ctx->priv;
>> + int i, len, ret = 0, nout = 0;
>> +
>> + len = tc->pattern[tc->pattern_pos] - '0';
>> +
>> + tc->pattern_pos++;
>> + if (!tc->pattern[tc->pattern_pos])
>> + tc->pattern_pos = 0;
>> +
>> + if (len == 0) // do not output any field from this frame
>> + return 0;
>> +
>> + if (tc->occupied) {
>> + for (i = 0; i < tc->nb_planes; i++) {
>> + // fill in the EARLIER field from the buffered pic
>> + av_image_copy_plane(
>> + &tc->frame[nout]->data[i][tc->frame[nout]->linesize[i] *
>> tc->first_field],
>> + tc->frame[nout]->linesize[i] * 2,
>> + &tc->temp->data[i][inpicref->linesize[i] *
>> tc->first_field],
>> + inpicref->linesize[i] * 2,
>> + tc->stride[i],
>> + (tc->planeheight[i] - tc->first_field + 1) / 2);
>> + // fill in the LATER field from the new pic
>> + av_image_copy_plane(
>> + &tc->frame[nout]->data[i][tc->frame[nout]->linesize[i] *
>> !tc->first_field],
>> + tc->frame[nout]->linesize[i] * 2,
>> + &inpicref->data[i][inpicref->linesize[i] *
>> !tc->first_field],
>> + inpicref->linesize[i] * 2,
>> + tc->stride[i],
>> + (tc->planeheight[i] - !tc->first_field + 1) / 2);
>> + }
>> + tc->frame[nout]->pts = AV_NOPTS_VALUE;
>> + nout++;
>> + len--;
>> + tc->occupied = 0;
>> + }
>> +
>> + while (len >= 2) {
>> + // output THIS image as-is
>> + for (i = 0; i < tc->nb_planes; i++)
>> + av_image_copy_plane(
>> + tc->frame[nout]->data[i], tc->frame[nout]->linesize[i],
>> + inpicref->data[i], inpicref->linesize[i],
>> + tc->stride[i],
>> + tc->planeheight[i]);
>> + tc->frame[nout]->pts = AV_NOPTS_VALUE;
>> + nout++;
>> + len -= 2;
>> + }
>> +
>> + if (len >= 1) {
>> + // copy THIS image to the buffer, we need it later
>> + for (i = 0; i < tc->nb_planes; i++)
>> + av_image_copy_plane(
>> + &tc->temp->data[i][0], inpicref->linesize[i],
>
> Are you sure temp is going to have inpicref linesize all the time?
Fixed.
>
> [...]
>
> --
> Clement B.
>
More information about the ffmpeg-devel
mailing list