[FFmpeg-devel] libavfilter: extending overlay filter
Stefano Sabatini
stefano.sabatini-lala
Mon Mar 14 12:55:24 CET 2011
On date Sunday 2011-03-13 20:54:09 +0000, Mark Himsley encoded:
> On 13/03/11 19:28, Mark Himsley wrote:
> >On 13/03/11 18:38, Stefano Sabatini wrote:
> >>On date Sunday 2011-03-13 15:18:04 +0000, Mark Himsley encoded:
> >>>On 13/03/11 14:26, Stefano Sabatini wrote:
> >>>>On date Sunday 2011-03-13 14:18:42 +0000, Mark Himsley encoded:
> >>[...]
> >>>--- ffmpeg/libavfilter/vf_overlay.c 2011-02-19 15:20:16.000000000 +0000
> >>>+++ FFmbc-0.6-rc3/libavfilter/vf_overlay.c 2011-03-13
> >>>15:11:23.000000000 +0000
> >>
> >>Please provide a patch created with git format-patch.
> >
> >Thanks Stefano for looking at the patch.
> <SNIP>
> >>>+ if (over->inout_is_rgb != over->blend_is_rgb) {
> >>>+ av_log(ctx, AV_LOG_ERROR,
> >>>+ "Main and overlay are not similar formats, cannot mix YUV and RGB\n");
> >>>+ return AVERROR(EINVAL);
> >>>+ }
> >>>return 0;
> >>
> >>Possibly I would like to avoid this kind of configuration failures, or
> >>we're going to have lots of reports from users failing to interpret
> >>the error message.
> >
> >Exactly. Which is why I said "I don't feel that I can submit [this
> >patch] while it breaks users current filter chains".
> >
> >
> >>A better approach is defining a configuration
> >>parameter for setting the colorspace class,
> >
> >By this I assume you mean a filter configuration parameter.
> >
> >I suppose we could set the default PixelFormats back to only allow YUV
> >and add an 'allow_RGB' switch to overlay.
>
> I've followed the excellent suggestions, and provide this patch for
> your consideration.
>
> The overlay filter now requires the user to add a :1 to the end of
> the overlay args in order to enable accepting RGB types.
>
> I have left the error message because advanced users who use the RGB
> path may still need a hint when they use incompatible formats.
>
>
> >>a yet better solution
> >>would be to extend the framework to manage efficiently such
> >>situations.
> >
> >True.
>
> --
> Mark
> diff --git a/ffmpeg/libavfilter/vf_overlay.c b/FFmbc-0.6-rc3/libavfilter/vf_overlay.c
> index 0eb24b9..3aa0ae2 100644
> --- a/ffmpeg/libavfilter/vf_overlay.c
> +++ b/FFmbc-0.6-rc3/libavfilter/vf_overlay.c
> @@ -57,9 +57,18 @@ enum var_name {
> #define MAIN 0
> #define OVERLAY 1
>
> +enum { RED = 0, GREEN, BLUE, ALPHA };
> +
> typedef struct {
> int x, y; ///< position of overlayed picture
>
> + int allow_packed_rgba;
> + uint8_t inout_is_packed_rgba;
> + uint8_t inout_is_alpha;
> + uint8_t inout_rgba_map[4];
> + uint8_t blend_is_packed_rgba;
> + uint8_t blend_rgba_map[4];
just a minor remark, I wonder if "overlay" is more clear than "blend".
> +
> AVFilterBufferRef *overpicref;
>
> int max_plane_step[4]; ///< steps per pixel for each plane
> @@ -76,8 +85,10 @@ static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> av_strlcpy(over->y_expr, "0", sizeof(over->y_expr));
>
> if (args)
> - sscanf(args, "%255[^:]:%255[^:]", over->x_expr, over->y_expr);
> + sscanf(args, "%255[^:]:%255[^:]:%d", over->x_expr, over->y_expr, &over->allow_packed_rgba);
>
> + over->allow_packed_rgba = !!over->allow_packed_rgba;
> +
> return 0;
> }
>
> @@ -91,10 +102,39 @@ static av_cold void uninit(AVFilterContext *ctx)
>
> static int query_formats(AVFilterContext *ctx)
> {
> + OverlayContext *over = ctx->priv;
> +
> const enum PixelFormat inout_pix_fmts[] = { PIX_FMT_YUV420P, PIX_FMT_NONE };
> const enum PixelFormat blend_pix_fmts[] = { PIX_FMT_YUVA420P, PIX_FMT_NONE };
> - AVFilterFormats *inout_formats = avfilter_make_format_list(inout_pix_fmts);
> - AVFilterFormats *blend_formats = avfilter_make_format_list(blend_pix_fmts);
> + const enum PixelFormat inout_pix_fmts_rgba[] = {
> + PIX_FMT_YUV420P,
> + PIX_FMT_BGR24,
> + PIX_FMT_RGB24,
> + PIX_FMT_ARGB,
> + PIX_FMT_RGBA,
> + PIX_FMT_ABGR,
> + PIX_FMT_BGRA,
> + PIX_FMT_NONE
> + };
> + const enum PixelFormat blend_pix_fmts_rgba[] = {
> + PIX_FMT_YUVA420P,
> + PIX_FMT_ARGB,
> + PIX_FMT_RGBA,
> + PIX_FMT_ABGR,
> + PIX_FMT_BGRA,
> + PIX_FMT_NONE
> + };
> +
> + AVFilterFormats *inout_formats;
> + AVFilterFormats *blend_formats;
> +
> + if (over->allow_packed_rgba) {
> + inout_formats = avfilter_make_format_list(inout_pix_fmts_rgba);
> + blend_formats = avfilter_make_format_list(blend_pix_fmts_rgba);
> + } else {
> + inout_formats = avfilter_make_format_list(inout_pix_fmts);
> + blend_formats = avfilter_make_format_list(blend_pix_fmts);
> + }
>
> avfilter_formats_ref(inout_formats, &ctx->inputs [MAIN ]->out_formats);
> avfilter_formats_ref(blend_formats, &ctx->inputs [OVERLAY]->out_formats);
> @@ -105,13 +145,37 @@ static int query_formats(AVFilterContext *ctx)
>
> static int config_input_main(AVFilterLink *inlink)
> {
> - OverlayContext *over = inlink->dst->priv;
> + AVFilterContext *ctx = inlink->dst;
> + OverlayContext *over = inlink->dst->priv;
> const AVPixFmtDescriptor *pix_desc = &av_pix_fmt_descriptors[inlink->format];
>
> av_image_fill_max_pixsteps(over->max_plane_step, NULL, pix_desc);
> over->hsub = pix_desc->log2_chroma_w;
> over->vsub = pix_desc->log2_chroma_h;
>
> + over->inout_is_packed_rgba = 1;
> + over->inout_is_alpha = 0;
> + switch (inlink->format) {
> + case PIX_FMT_ARGB: over->inout_is_alpha = 1; over->inout_rgba_map[ALPHA] = 0; over->inout_rgba_map[RED ] = 1; over->inout_rgba_map[GREEN] = 2; over->inout_rgba_map[BLUE ] = 3; break;
> + case PIX_FMT_ABGR: over->inout_is_alpha = 1; over->inout_rgba_map[ALPHA] = 0; over->inout_rgba_map[BLUE ] = 1; over->inout_rgba_map[GREEN] = 2; over->inout_rgba_map[RED ] = 3; break;
> + case PIX_FMT_RGBA: over->inout_is_alpha = 1;
> + case PIX_FMT_RGB24: over->inout_rgba_map[RED ] = 0; over->inout_rgba_map[GREEN] = 1; over->inout_rgba_map[BLUE ] = 2; over->inout_rgba_map[ALPHA] = 3; break;
> + case PIX_FMT_BGRA: over->inout_is_alpha = 1;
> + case PIX_FMT_BGR24: over->inout_rgba_map[BLUE ] = 0; over->inout_rgba_map[GREEN] = 1; over->inout_rgba_map[RED ] = 2; over->inout_rgba_map[ALPHA] = 3; break;
> + default:
> + over->inout_is_packed_rgba = 0;
> + }
This should be factored out in a separate function...
> +
> + if (over->inout_is_alpha)
> + av_log(ctx, AV_LOG_INFO,
> + "main has alpha\n");
> +
> + if (over->inout_is_packed_rgba)
> + av_log(ctx, AV_LOG_INFO,
AV_LOG_DEBUG
> + "main pixel locations R:%d G:%d B:%d A:%d\n",
> + over->inout_rgba_map[RED], over->inout_rgba_map[GREEN],
> + over->inout_rgba_map[BLUE], over->inout_rgba_map[ALPHA]);
> +
> return 0;
> }
>
> @@ -147,7 +211,23 @@ static int config_input_overlay(AVFilterLink *inlink)
> NULL, NULL, NULL, NULL, NULL, 0, ctx)) < 0)
> goto fail;
> over->x = res;
> +
> + over->blend_is_packed_rgba = 1;
> + switch (inlink->format) {
> + case PIX_FMT_ARGB: over->blend_rgba_map[ALPHA] = 0; over->blend_rgba_map[RED ] = 1; over->blend_rgba_map[GREEN] = 2; over->blend_rgba_map[BLUE ] = 3; break;
> + case PIX_FMT_ABGR: over->blend_rgba_map[ALPHA] = 0; over->blend_rgba_map[BLUE ] = 1; over->blend_rgba_map[GREEN] = 2; over->blend_rgba_map[RED ] = 3; break;
> + case PIX_FMT_RGBA: over->blend_rgba_map[RED ] = 0; over->blend_rgba_map[GREEN] = 1; over->blend_rgba_map[BLUE ] = 2; over->blend_rgba_map[ALPHA] = 3; break;
> + case PIX_FMT_BGRA: over->blend_rgba_map[BLUE ] = 0; over->blend_rgba_map[GREEN] = 1; over->blend_rgba_map[RED ] = 2; over->blend_rgba_map[ALPHA] = 3; break;
> + default:
> + over->blend_is_packed_rgba = 0;
> + }
Code duplication.
> + if (over->inout_is_packed_rgba)
> + av_log(ctx, AV_LOG_INFO,
AV_LOG_DEBUG
> + "overlay pixel locations R:%d G:%d B:%d A:%d\n",
> + over->blend_rgba_map[RED], over->blend_rgba_map[GREEN],
> + over->blend_rgba_map[BLUE], over->blend_rgba_map[ALPHA]);
> +
> av_log(ctx, AV_LOG_INFO,
> "main w:%d h:%d fmt:%s overlay x:%d y:%d w:%d h:%d fmt:%s\n",
> ctx->inputs[MAIN]->w, ctx->inputs[MAIN]->h,
> @@ -167,6 +247,13 @@ static int config_input_overlay(AVFilterLink *inlink)
> (int)var_values[VAR_MAIN_W], (int)var_values[VAR_MAIN_H]);
> return AVERROR(EINVAL);
> }
> +
> + if (over->inout_is_packed_rgba != over->blend_is_packed_rgba) {
> + av_log(ctx, AV_LOG_ERROR,
> + "Main and overlay are not similar formats, cannot mix YUV and RGB.\nInsert a format filter to change a stream's format.\n");
> + return AVERROR(EINVAL);
> + }
I believe this should be avoided/removed.
> +
> return 0;
>
> fail:
> @@ -187,7 +274,7 @@ static int config_output(AVFilterLink *outlink)
> av_gcd((int64_t)tb1.num * tb2.den,
> (int64_t)tb2.num * tb1.den),
> (int64_t)tb1.den * tb2.den, INT_MAX);
> - av_log(ctx, AV_LOG_INFO,
> + av_log(ctx, AV_LOG_DEBUG,
spurious
> "main_tb:%d/%d overlay_tb:%d/%d -> tb:%d/%d exact:%d\n",
> tb1.num, tb1.den, tb2.num, tb2.den, tb->num, tb->den, exact);
> if (!exact)
> @@ -256,20 +343,71 @@ static void blend_slice(AVFilterContext *ctx,
> start_y = FFMAX(y, slice_y);
> height = end_y - start_y;
>
> - if (dst->format == PIX_FMT_BGR24 || dst->format == PIX_FMT_RGB24) {
> + if (over->inout_is_packed_rgba) {
> uint8_t *dp = dst->data[0] + x * 3 + start_y * dst->linesize[0];
> uint8_t *sp = src->data[0];
> - int b = dst->format == PIX_FMT_BGR24 ? 2 : 0;
> - int r = dst->format == PIX_FMT_BGR24 ? 0 : 2;
> + uint8_t blend; ///< the amount of overlay to blend on to main
> if (slice_y > y)
> sp += (slice_y - y) * src->linesize[0];
> for (i = 0; i < height; i++) {
> uint8_t *d = dp, *s = sp;
> for (j = 0; j < width; j++) {
> - d[r] = (d[r] * (0xff - s[3]) + s[0] * s[3] + 128) >> 8;
> - d[1] = (d[1] * (0xff - s[3]) + s[1] * s[3] + 128) >> 8;
> - d[b] = (d[b] * (0xff - s[3]) + s[2] * s[3] + 128) >> 8;
> - d += 3;
> + // m is the blend multiplication of overlay over the main main
can be removed
> + blend = s[over->blend_rgba_map[ALPHA]];
I'd prefer overlay_alpha
> + // if the main channel has alpha m has to be calculated
> + // to create an un-premultiplied (straight) blend
> + if (over->inout_is_alpha)
> + {
> + switch (blend)
> + {
Nit: the favored style is
if (...) {
switch (...) {
> + case 0:
> + case 0xff:
> + break;
> + default:
> + // the un-premultiplied calculation is:
> + // (255 * 255 * overlay_alpha) / ( 255 * (overlay_alpha + main_alpha) - (overlay_alpha * main_alpha) )
> + blend =
> + // the next line is a faster version of: 0xff * 0xff * blend
> + ( (blend << 16) - (blend << 9) + blend )
> + / (
> + // the next line is a faster version of: 0xff * (blend + d[over->inout_rgba_map[ALPHA]])
> + ((blend + d[over->inout_rgba_map[ALPHA]]) << 8 ) - (blend + d[over->inout_rgba_map[ALPHA]])
> + - d[over->inout_rgba_map[ALPHA]] * blend
> + );
I can't understand this.
When overlaying over the main input, the main alpha should remain
unchanged, only the overlay alpha channel is used for blending over
the main input.
> + }
> + }
> + switch (blend)
> + {
> + case 0:
> + break;
> + case 0xff:
> + d[over->inout_rgba_map[RED] ] = s[over->blend_rgba_map[RED] ];
> + d[over->inout_rgba_map[GREEN]] = s[over->blend_rgba_map[GREEN]];
> + d[over->inout_rgba_map[BLUE] ] = s[over->blend_rgba_map[BLUE] ];
> + break;
> + default:
> + d[over->inout_rgba_map[RED] ] = (d[over->inout_rgba_map[RED] ] * (0xff - blend) + s[over->blend_rgba_map[RED] ] * blend) / 0xff;
> + d[over->inout_rgba_map[GREEN]] = (d[over->inout_rgba_map[GREEN]] * (0xff - blend) + s[over->blend_rgba_map[GREEN]] * blend) / 0xff;
> + d[over->inout_rgba_map[BLUE] ] = (d[over->inout_rgba_map[BLUE] ] * (0xff - blend) + s[over->blend_rgba_map[BLUE] ] * blend) / 0xff;
> + }
> + if ( over->inout_is_alpha )
> + {
> + switch (blend)
> + {
> + case 0:
> + break;
> + case 0xff:
> + d[over->inout_rgba_map[ALPHA]] = s[over->blend_rgba_map[ALPHA]];
> + break;
> + default:
> + d[over->inout_rgba_map[ALPHA]] = (
> + (d[over->inout_rgba_map[ALPHA]] << 8) + (0x100 - d[over->inout_rgba_map[ALPHA]]) * s[over->blend_rgba_map[ALPHA]]
> + ) >> 8;
> + }
> + d += 4;
> + } else {
> + d += 3;
> + }
> s += 4;
d += dst_pixstep;
s += src_pixstep;
more clear and more robust
--
FFmpeg = Fancy & Fostering MultiPurpose Ecumenical Ghost
More information about the ffmpeg-devel
mailing list