[FFmpeg-devel] [SOCIS] Qualification task

[Stefano Sabatini]

On date Tuesday 2012-07-31 22:17:15 +0200, Michele Orrù encoded:
> https://github.com/mmaker/FFmpeg/commits/master
> 
> The link above summarizes what I've done during those days playing
> with libavfilter.
> I'm sorry for submitting one existent filter, and another non working,
> but there is something I am missing when using planes, and
> unfortunately I did not have had the time to go in depth about this
> topic.

Inline review follows.

> /*
> * This file is part of FFmpeg.

Missing copyright, and of the original author in case this is a port
(in that case make sure that the original author agrees in case or
relicensing).

>  *
>  * FFmpeg is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>  * License as published by the Free Software Foundation; either
>  * version 2.1 of the License, or (at your option) any later version.
>  *
>  * FFmpeg is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>  * Lesser General Public License for more details.
>  *
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with FFmpeg; if not, write to the Free Software
>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>  */
>  
>  /**
>  * @file
>  * puzzle video filter
>  */
>  
>  #include "avfilter.h"
>  #include "internal.h"
>  #include "video.h"
>  #include "libavutil/lfg.h"
>  
>  typedef struct {

>      int* xoffsets;
>      int* yoffsets;
>      int xs;
>      int ys;

Add doxy to these fields, should ease code review/understanding.

>  } PuzzleContext;
>  
>  
>  static void shuffle(int* arr, size_t len)
>  {
>     int replace, index;
>  
>     if (len <= 1) return;
>  

>     av_lfg_get(&index);

I suppose you need to pass an LFG context here, also this statement
has no effects.

>     index = 1 + index % len;
>     replace = arr[0];
>     arr[0] = arr[index];
>     arr[index] = replace;
>  
>     shuffle(arr+1, len-1);
>  }
>  
>  static void memflip(char *a, char *b, size_t n)
>  {
>      static char tmp;
>      for (; n; a++, b++, n--) {
>          tmp = *a;
>          *a = *b;
>          *b = *a;
>      }
>  }
>  
>  static av_cold int init(AVFilterContext *ctx, const char *args)
>  {
>      PuzzleContext *puzzle = ctx->priv;
>  
>  
>      if (args)
>          sscanf(args, "%d:%d", &puzzle->xs, &puzzle->ys);
  
missing validation 

>      else
>          puzzle->xs = puzzle->ys = 5;
>  
>      if (!((puzzle->xoffsets = av_calloc(sizeof(int), puzzle->xs)) &&
>            (puzzle->yoffsets = av_calloc(sizeof(int), puzzle->ys))))
>          return AVERROR(ENOMEM);
>  
>      return 0;
>  }
>  
>  static void uninit(AVFilterContext *ctx)
>  {
>      PuzzleContext *puzzle = ctx->priv;
>  
>      av_free(puzzle->xoffsets);
>      av_free(puzzle->yoffsets);
>  }
>  
>  static int config_props(AVFilterLink* link)

nit: link -> inlink

>  {
>      PuzzleContext *puzzle = link->dst->priv;
>      size_t i;
>  

>      for (i = 0; i != puzzle->xs; i++)
>          puzzle->xoffsets[i] = i;
>      for (i = 0; i != puzzle->ys; i++)
>          puzzle->yoffsets[i] = i;
>  
>      /* shuffle */
>      shuffle(puzzle->yoffsets, puzzle->ys);
>      shuffle(puzzle->xoffsets, puzzle->xs);

Maybe you can do this in init(), should avoid the need of the
config_props() callback.

>      return 0;
>  
>  }
>  
>  
>  static int draw_slice(AVFilterLink *link, int y, int h, int slice_dir)
>  {

nit: inlink

>     PuzzleContext *puzzle = link->dst->priv;
>     AVFilterBufferRef *pic = link->cur_buf;

>     const int block = link->w / puzzle->xs;

block_size seems more meaningful

>     unsigned char *p;
>     size_t item, i;

again "item" seems confusing, maybe block_index or index

>  
>     p = pic->data[0] + y * pic->linesize[0];
>     for (i = 0; i != h; i++) {
>         for (item = 0; item != puzzle->xs; item++) {
>             memflip(p + block * item, p + puzzle->xoffsets[block] * item,
>                     block-1);

In this case I don't think you can do inplace processing, and it will
be inefficient using a pixel-per-pixel memflip, in this case I'd
allocate a new buffer (automatically put in outlink->out_buf if you
use the standard start_frame() callback) and move data from
inlink->cur_buf with memcpy from block line to block line.

Also if you want to shuffle lines, you'll need to process the whole
frame in end_frame(), so it seems per-slice processing won't be
easy/convenient in this case.

>             // put white pixel in the middle
>             p[block*item] = 0xff;
>         }
>         p += pic->linesize[0];
>     }
>     return 0;
>  }

Note: this code seems to work only with gray8 data, you need to
declare this in query_formats() (the default query_formats() will
accepts *all* formats) and then extend the filter once you get the
gray8 case working.

>  static int end_frame(AVFilterLink *link)
>  {
>      return ff_null_end_frame(link);
>  }

pointless wrapper

>  AVFilter avfilter_vf_puzzle = {
>      .name = "puzzle",

>      .description = NULL_IF_CONFIG_SMALL("Create a fuckin' puzzle from the given video"),

Make sure that little boys/girls won't read this ;-).
Nit: missing ending point.

[...]
-- 
FFmpeg = Fierce and Free Monstrous Peaceless Elected Game