[FFmpeg-devel] [PATCH 2/7] lavfi/tile: add margin and padding options.

[Nicolas George]

L'octidi 18 brumaire, an CCXXI, Clément Bœsch a écrit :
> ---
>  libavfilter/vf_tile.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)

I must say, I am not keen on the principle of this patch. The inner margin
can be added by inserting a pad filter before tile, and the outer margin
with a pad filter after. It makes the filter description slightly more
verbose, but it has additional options, such as having different horizontal
and vertical margins and specifying the color.

But the additional code is not much, so I will personally not object. Wait
and see if someone does, though.

Also, maybe mention the pad filter in the documentation.

> 
> diff --git a/libavfilter/vf_tile.c b/libavfilter/vf_tile.c
> index 716cc46..8f5a4f3 100644
> --- a/libavfilter/vf_tile.c
> +++ b/libavfilter/vf_tile.c
> @@ -34,6 +34,8 @@
>  typedef struct {
>      const AVClass *class;
>      unsigned w, h;
> +    unsigned margin;
> +    unsigned padding;
>      unsigned current;
>      FFDrawContext draw;
>      FFDrawColor blank;
> @@ -47,6 +49,8 @@ typedef struct {
>  static const AVOption tile_options[] = {
>      { "size", "set tile size", OFFSET(w), AV_OPT_TYPE_IMAGE_SIZE, {.str = "6x5"}, 0, 0, FLAGS },
>      { "s",    "set tile size", OFFSET(w), AV_OPT_TYPE_IMAGE_SIZE, {.str = "6x5"}, 0, 0, FLAGS },
> +    { "margin",  "set outer border margin in pixels",    OFFSET(margin),  AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1024, FLAGS },
> +    { "padding", "set inner border thickness in pixels", OFFSET(padding), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1024, FLAGS },
>      {NULL},
>  };
>  
> @@ -83,19 +87,21 @@ static int config_props(AVFilterLink *outlink)
>      AVFilterContext *ctx = outlink->src;
>      TileContext *tile   = ctx->priv;
>      AVFilterLink *inlink = ctx->inputs[0];
> +    const int tw_pad_margins = (tile->w - 1) * tile->padding + 2*tile->margin;
> +    const int th_pad_margins = (tile->h - 1) * tile->padding + 2*tile->margin;

total_margin_w and _h?

unsigned?

Also, the const seems useless: compilers are not that stupid.

>  
> -    if (inlink->w > INT_MAX / tile->w) {
> +    if (inlink->w > (INT_MAX - tw_pad_margins) / tile->w) {
>          av_log(ctx, AV_LOG_ERROR, "Total width %ux%u is too much.\n",
>                 tile->w, inlink->w);
>          return AVERROR(EINVAL);
>      }
> -    if (inlink->h > INT_MAX / tile->h) {
> +    if (inlink->h > (INT_MAX - th_pad_margins) / tile->h) {
>          av_log(ctx, AV_LOG_ERROR, "Total height %ux%u is too much.\n",
>                 tile->h, inlink->h);
>          return AVERROR(EINVAL);
>      }
> -    outlink->w = tile->w * inlink->w;
> -    outlink->h = tile->h * inlink->h;
> +    outlink->w = tile->w * inlink->w + tw_pad_margins;
> +    outlink->h = tile->h * inlink->h + th_pad_margins;
>      outlink->sample_aspect_ratio = inlink->sample_aspect_ratio;
>      outlink->frame_rate = av_mul_q(inlink->frame_rate,
>                                     (AVRational){ 1, tile->w * tile->h });
> @@ -123,17 +129,33 @@ static int start_frame(AVFilterLink *inlink, AVFilterBufferRef *picref)
>      avfilter_copy_buffer_ref_props(outlink->out_buf, picref);
>      outlink->out_buf->video->w = outlink->w;
>      outlink->out_buf->video->h = outlink->h;
> +
> +    /* fill surface once for margin/padding */
> +    ff_fill_rectangle(&tile->draw, &tile->blank,
> +                      outlink->out_buf->data, outlink->out_buf->linesize,
> +                      0, 0, outlink->w, outlink->h);

This is expensive, and only useful if there are margins. Maybe add a test?

OTOH, if this is done, then filling the empty last frames becomes
unnecessary.

>      return 0;
>  }
>  
> +static void get_current_tile_pos(AVFilterContext *ctx, unsigned *x, unsigned *y)
> +{
> +    TileContext *tile    = ctx->priv;
> +    AVFilterLink *inlink = ctx->inputs[0];
> +    const unsigned tx = tile->current % tile->w;
> +    const unsigned ty = tile->current / tile->w;
> +
> +    *x = tile->margin + (tx ? (inlink->w + tile->padding) * tx : 0);
> +    *y = tile->margin + (ty ? (inlink->h + tile->padding) * ty : 0);

The conditional looks useless: 0*anything = 0.

> +}
> +
>  static int draw_slice(AVFilterLink *inlink, int y, int h, int slice_dir)
>  {
>      AVFilterContext *ctx  = inlink->dst;
>      TileContext *tile    = ctx->priv;
>      AVFilterLink *outlink = ctx->outputs[0];
> -    unsigned x0 = inlink->w * (tile->current % tile->w);
> -    unsigned y0 = inlink->h * (tile->current / tile->w);
> +    unsigned x0, y0;
>  
> +    get_current_tile_pos(ctx, &x0, &y0);
>      ff_copy_rectangle2(&tile->draw,
>                         outlink->out_buf->data, outlink->out_buf->linesize,
>                         inlink ->cur_buf->data, inlink ->cur_buf->linesize,
> @@ -147,9 +169,9 @@ static void draw_blank_frame(AVFilterContext *ctx, AVFilterBufferRef *out_buf)
>  {
>      TileContext *tile    = ctx->priv;
>      AVFilterLink *inlink  = ctx->inputs[0];
> -    unsigned x0 = inlink->w * (tile->current % tile->w);
> -    unsigned y0 = inlink->h * (tile->current / tile->w);
> +    unsigned x0, y0;
>  
> +    get_current_tile_pos(ctx, &x0, &y0);
>      ff_fill_rectangle(&tile->draw, &tile->blank,
>                        out_buf->data, out_buf->linesize,
>                        x0, y0, inlink->w, inlink->h);

Apart from those minor remarks, ok for the code itself.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121109/93bc84cb/attachment.asc>