[vlc-devel] [PATCH] autocrop video filter 2

[Antoine Cellerier]

On Mon, Apr 14, 2008, Geoffroy Couprie wrote:
> + * Authors: Geoffroy Couprie <geo.couprie at gmail.com>

You should keep the old authors since you copied part of the code from
another file.

> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif

This shouldn't be needed.

> +static int AutocropCallback( vlc_object_t *p_this, char const *psz_var,
> +                               vlc_value_t oldval, vlc_value_t newval,
> +                               void *p_data );

Is this function of any use? (It was in the old crop module to get
cropping parameters changes from the interface)

> +#define GEOMETRY_TEXT N_("Crop geometry (pixels)")
> +#define GEOMETRY_LONGTEXT N_("Set the geometry of the zone to crop. This is set as <width> x <height> + <left offset> + <top offset>.")
> +
> +#define AUTOCROP_TEXT N_("Automatic cropping")
> +#define AUTOCROP_LONGTEXT N_("Automatically detect black borders and crop them.")

Same comment.

> +    add_string( "crop-geometry", NULL, NULL, GEOMETRY_TEXT,
> +                                             GEOMETRY_LONGTEXT, VLC_FALSE );
> +    add_bool( "autocrop", 0, NULL, AUTOCROP_TEXT,
> +                                   AUTOCROP_LONGTEXT, VLC_FALSE );

Same here too.
Please also use the same prefix for all options in the module.

> +static const char *ppsz_filter_options[] = {
> +    "factor", NULL
> +};

This array should mirror the config options (minus the prefix).

> +    vlc_bool_t b_autocrop;

This isn't valid anymore (as of yesterday :) )

> +        var_SetInteger( p_vout, "crop-top", i_firstwhite );
> +        var_SetInteger( p_vout, "crop-bottom", i_firstwhite );
> +        var_SetInteger( p_vout, "height", p_filter->p_sys->i_height);

Would it be usefull to crop vertically too? (left/right/width)

-- 
Antoine Cellerier
dionoea