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

[Clément Bœsch]

On Fri, Nov 09, 2012 at 12:47:06AM +0100, Nicolas George wrote:
> 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.
> 

I agree with that, and I went through the "trouble" of adding the option
because it was pretty free to add given the design of the filter. Also,
both options go hand in hand in my opinion: I believe users willing to add
some padding between tiles are likely to look for the same for the outer
border in the options of that filter, and redirecting them to another
filter just for this purpose sounds a bit unfriendly.

> Also, maybe mention the pad filter in the documentation.
> 

That's a good point, added "For more advanced padding options (such as
having different values for the edges), refer to the pad video filter." to
the padding option 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?
> 

That was indeed a pretty bad name, changed.

> unsigned?
> 

Sure why not.

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

That's kind of a hint for the developer actually.

> >  
> > -    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?
> 

Added "if (tile->margin || tile->padding)"

> 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.
> 

Mpf, leftover from when the expression was wrong, ternary removed.

> > +}
> > +
> >  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.
> 

Thanks,

I'll wait until tomorrow for comments on the "margin" option, and push if
I don't see any.

-- 
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/20121110/42888175/attachment.asc>