[FFmpeg-devel] [PATCH] telecine filter
Clément Bœsch
ubitux at gmail.com
Mon Apr 8 03:11:02 CEST 2013
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?
Also, is it really problematic to use "top" and "bottom" instead of one
letter?
> + 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.
> + {"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'
> + {"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()
Also, you can include this check in the following loop.
> + 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?
Now, what if p is "0"? Isn't this going to /0 somewhere?
> + 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?
> + 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?
[...]
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130408/5295f11b/attachment.asc>
More information about the ffmpeg-devel
mailing list