On Fri, Mar 08, 2013 at 06:45:58PM +0100, Vittorio Giovara wrote:
> While updating the tinterlace filter to the new AVFrame API I noticed a lot
> of cruft and silly code, so I decided to mostly rewrite it, heavily
> simplifying it, and give it a new name. I don't know what this means for
> the (c) list (which has become quite crowded) and if it could be reduced
> (I'd be fine with a no (c) one too) or for the licence (originally this
> module was gpl but it's almost completely rewritten now and wouldn't mind
> switching to lgpl).
We'll have to compare the filters in detail.
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -56,6 +56,7 @@ OBJS-$(CONFIG_FREI0R_FILTER) += vf_frei0r.o
> OBJS-$(CONFIG_GRADFUN_FILTER) += vf_gradfun.o
> OBJS-$(CONFIG_HFLIP_FILTER) += vf_hflip.o
> OBJS-$(CONFIG_HQDN3D_FILTER) += vf_hqdn3d.o
> +OBJS-$(CONFIG_INTERLACER_FILTER) += vf_interlacer.o
> OBJS-$(CONFIG_LUT_FILTER) += vf_lut.o
> OBJS-$(CONFIG_LUTRGB_FILTER) += vf_lut.o
> OBJS-$(CONFIG_LUTYUV_FILTER) += vf_lut.o
Isn't plain "interlacer" a bit generic as a name?
> --- /dev/null
> +++ b/libavfilter/vf_interlacer.c
> @@ -0,0 +1,297 @@
> +
> +typedef struct {
> + const AVClass *class;
> + enum InterlacerMode mode; ///< top or bottom field first mode
> + int lowpass; ///< enable or disable low pass filterning
> + int frame; ///< number of the output frame
> + int vsub; ///< chroma vertical subsampling
> + AVFrame *cur, *next;
> +} InterlacerContext;
Why are these comments Doxygen?
> +#define OFFSET(x) offsetof(InterlacerContext, x)
> +#define V AV_OPT_FLAG_VIDEO_PARAM
> +static const AVOption options[] = {
> + {"mode", "select interlacing mode", OFFSET(mode),
> AV_OPT_TYPE_INT, {.i64 = MODE_TFF}, 0, 1, .flags = V },
> + {"tff", "top field first", 0, AV_OPT_TYPE_CONST,
> {.i64 = MODE_TFF}, INT_MIN, INT_MAX, .flags = V, .unit = "mode" },
> + {"bff", "bottom field first", 0, AV_OPT_TYPE_CONST,
> {.i64 = MODE_BFF}, INT_MIN, INT_MAX, .flags = V, .unit = "mode" },
> + {"lowpass", "enable vertical low-pass filter", OFFSET(lowpass),
> AV_OPT_TYPE_INT, {.i64 = 1 }, 0, 1, .flags = V },
> + {NULL}
> +};
spaces inside {}
spacing seems a bit arbitrary
> +static int query_formats(AVFilterContext *ctx)
> +{
> + static const enum AVPixelFormat pix_fmts[] = {
> + AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV444P,
> + AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV410P, AV_PIX_FMT_YUVA420P,
> + AV_PIX_FMT_GRAY8, AV_PIX_FMT_YUVJ420P, AV_PIX_FMT_YUVJ422P,
> + AV_PIX_FMT_YUVJ444P, AV_PIX_FMT_YUVJ440P, AV_PIX_FMT_NONE
> + };
I'd move the table out of the function and use a less generic name.
> + av_log(ctx, AV_LOG_VERBOSE, "%s interlacing %s lowpass filter\n",
> + (s->mode == MODE_TFF) ? "tff" : "bff", (s->lowpass) ? "with" :
> "without");
pointless () around s->lowpass
> +/**
> + * Copy picture field from src to dst.
> + *
> + * @param src_field copy from upper, lower field or both
> + * @param dst_field copy to upper or lower field,
> + * only meaningful when interleave is selected
> + * @param lowpass vertical lowpass filter (less twitter and moire pattern)
> + */
> +static inline
> +void copy_picture_field(AVFrame *src_frame, AVFrame *dst_frame,
> + enum AVPixelFormat format, int w, int src_h,
> + int src_field, int dst_field, int lowpass)
Keep function declarations on one line.
> +{
> + uint8_t **dst = dst_frame->data;
> + int *dst_linesize = dst_frame->linesize;
> + uint8_t **src = src_frame->data;
> + int *src_linesize = src_frame->linesize;
nit: align
> + for (h = lines; h > 0; h--) {
> + const uint8_t *srcp_above = srcp - src_linesize[plane];
> + const uint8_t *srcp_below = srcp + src_linesize[plane];
> + if (h == lines) srcp_above = srcp; // there is no line above
> + if (h == 1) srcp_below = srcp; // there is no line below
Break the lines please.
> +static int filter_frame(AVFilterLink *inlink, AVFrame *picref)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + AVFilterLink *outlink = ctx->outputs[0];
> + InterlacerContext *s = ctx->priv;
> + AVFrame *out;
> + int tff, ret;
> +
> + av_frame_free(&s->cur);
> + s->cur = s->next;
> + s->next = picref;
nit: align
> + av_frame_copy_props(out, s->cur);
> + out->interlaced_frame = 1;
> + out->top_field_first = tff;
> + out->pts /= 2; // adjust pts to new framerate
same
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
