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

[Geoffroy Couprie]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On Mon, Apr 14, 2008 at 5:14 PM, Antoine Cellerier  wrote:
> On Mon, Apr 14, 2008, Geoffroy Couprie wrote:
>  > + * Authors: Geoffroy Couprie
>
>  You should keep the old authors since you copied part of the code from
>  another file.
Done
>
>  > +#ifdef HAVE_CONFIG_H
>  > +# include "config.h"
>  > +#endif
>
>  This shouldn't be needed.
In file included from ../../include/vlc/common.h:136,
                 from ../../include/vlc/vlc.h:35,
                 from autocrop.c:31:
../../include/vlc_common.h:34:2: error: #error You probably forgot to
include config.h!!

mmmh, I think I need it :)

>
>  > +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)
I removed it. I doubt it is really helpful for the user, to change the
autocrop conf while playing a video.

>
>  > +#define GEOMETRY_TEXT N_("Crop geometry (pixels)")
>  > +#define GEOMETRY_LONGTEXT N_("Set the geometry of the zone to crop. This is set as  x  +  + .")
>  > +
>  > +#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.
Removed.

>  Please also use the same prefix for all options in the module.
Done
>
>  > +static const char *ppsz_filter_options[] = {
>  > +    "factor", NULL
>  > +};
>
>  This array should mirror the config options (minus the prefix).
Done
>
>  > +    vlc_bool_t b_autocrop;
>
>  This isn't valid anymore (as of yesterday :) )
Yes, I saw it this morning. But I was waiting for the review to post
another patch :)
>
>  > +        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)
I don't know. Cropping is done mostly to get rid of the black borders
one gets when recording from a TV, and they generally fit to the width
of the screen. But it could be done too :)

I also need some help to change the ratio while the filter crops.
Maybe there's a callback I can use? I tried to set the aspect-ratio
manually with var_SetString, but it seems it doesn't accept other
ratios that the ones in conf.

Here is the new patch with fixes from above.

- --
Geoffroy Couprie

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIA5MzuaM3AP8orqARAhMHAJ9u9zN+V40i9rnsIcBTBorqf72phQCfSFtV
xTS3eENwymwbPU8DOGOjk6s=
=sP4Y
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fixes-to-autocrop-video-filter.patch
Type: text/x-patch
Size: 18732 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20080414/be8309de/attachment.bin>