[vlc-devel] [PATCH] Added some more error checking, fixed close function, and fixed some comments

[Tristan Matthews]

On Wed, Feb 3, 2016 at 1:55 PM, Odd-Arild Kristensen
<oddarildkristensen at gmail.com> wrote:
> ---
>  modules/video_filter/Makefile.am     |   3 +
>  modules/video_filter/edgedetection.c | 231 +++++++++++++++++++++++++++++++++++
>  2 files changed, 234 insertions(+)
>  create mode 100644 modules/video_filter/edgedetection.c
>
> diff --git a/modules/video_filter/Makefile.am b/modules/video_filter/Makefile.am
> index ea0b72c..b2ff9cd 100644
> --- a/modules/video_filter/Makefile.am
> +++ b/modules/video_filter/Makefile.am
> @@ -3,6 +3,8 @@ video_filterdir = $(pluginsdir)/video_filter
>  noinst_HEADERS += video_filter/filter_picture.h
>
>  # video filters
> +libedgedetection_plugin_la_SOURCES = video_filter/edgedetection.c
> +libedgedetection_plugin_la_LIBADD = $(LIBM)
>  libadjust_plugin_la_SOURCES = video_filter/adjust.c video_filter/adjust_sat_hue.c video_filter/adjust_sat_hue.h
>  libadjust_plugin_la_LIBADD = $(LIBM)
>  libalphamask_plugin_la_SOURCES = video_filter/alphamask.c
> @@ -75,6 +77,7 @@ video_filter_LTLIBRARIES = \
>         libcanvas_plugin.la \
>         libcolorthres_plugin.la \
>         libcroppadd_plugin.la \
> +       libedgedetection_plugin.la \
>         liberase_plugin.la \
>         libextract_plugin.la \
>         libgradient_plugin.la \
> diff --git a/modules/video_filter/edgedetection.c b/modules/video_filter/edgedetection.c
> new file mode 100644
> index 0000000..c13070a
> --- /dev/null
> +++ b/modules/video_filter/edgedetection.c
> @@ -0,0 +1,231 @@
> +/******************************************************************************
> + * edgedetection.c : edge detection plugin for VLC
> +  *****************************************************************************
> + * Copyright (C) 2016 VLC authors and VideoLAN
> + * $Id$
> + *
> + * Authors: Odd-Arild Kristensen <oddarildkristensen at gmail.com>
> + *
> + * This program 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.
> + *
> + * This program 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 this program; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> + *****************************************************************************/
> +
> +/*****************************************************************************
> + * Preabmle

typo

> + *****************************************************************************/
> +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <assert.h>
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_filter.h>
> +#include <string.h>

You actually don't need assert.h or string.h

> +
> +/*****************************************************************************
> + * Module descriptor
> + *****************************************************************************/
> +#define EDGE_DETECTION_DESCRIPTION N_( "Edge detection video filter" )
> +#define EDGE_DETECTION_TEXT N_( "Edge detection" )
> +#define EDGE_DETECTION_LONGTEXT N_( \
> +    "Detects edges in the frame and highlights them in white." )
> +
> +/*****************************************************************************
> + * Local prototypes
> + *****************************************************************************/
> +static int Open( vlc_object_t * );
> +static int Close( vlc_object_t * );
> +static picture_t *new_frame( filter_t * );
> +static picture_t *Filter( filter_t *, picture_t * );
> +static int sobel( uint8_t const *, const int, const int, const int, const int);
> +
> +/* Kernel for X axis */
> +static const int pi_kernel_x[3][3] = {
> +    {-1, 0, 1},
> +    {-2, 0, 2},
> +    {-1, 0, 1}
> +};
> +
> +/* Kernel for Y axis */
> +static const int pi_kernel_y[3][3] = {
> +    {-1, -2, -1},
> +    {0, 0, 0},
> +    {1, 2, 1}
> +};
> +
> +vlc_module_begin ()
> +
> +    set_description( EDGE_DETECTION_DESCRIPTION )
> +    set_shortname( EDGE_DETECTION_TEXT )
> +    set_help( EDGE_DETECTION_LONGTEXT )
> +    set_category( CAT_VIDEO )
> +    set_subcategory( SUBCAT_VIDEO_VFILTER )
> +
> +    set_capability( "video filter2", 0 )
> +    set_callbacks( Open, Close )
> +
> +vlc_module_end ()
> +
> +/* Store the filter chain */
> +struct filter_sys_t
> +{
> +    filter_chain_t *p_chain;
> +};
> +
> +/*****************************************************************************
> + * Opens the filter.
> + * Allocates and initializes data needed by the filter. The image needs to
> + * be black-and-white in order to detect any edges. The Gaussian blur is
> + * needed so that the Sobel operator does not give a high response for noise,
> + * or small changes in the image.
> + *****************************************************************************/
> +static int Open( vlc_object_t *p_this )
> +{
> +    int ret;
> +    es_format_t fmt;
> +    filter_t *p_filter = (filter_t *)p_this;
> +    filter_sys_t *p_sys;
> +    filter_owner_t owner = {
> +        .sys = p_filter,
> +        .video = {
> +            .buffer_new = new_frame,
> +        },
> +    };
> +
> +    p_sys = malloc( sizeof( filter_sys_t ) );
> +    if( p_sys == NULL) {
> +        return VLC_ENOMEM;
> +    }
> +
> +    p_filter->p_sys = p_sys;
> +    p_filter->p_sys->p_chain = filter_chain_NewVideo( p_filter, true, &owner );
> +
> +    if ( p_sys->p_chain == NULL)
> +    {
> +        msg_Err( p_filter, "Could not allocate filter chain" );
> +        free( p_sys );
> +        return VLC_EGENERIC;
> +    }
> +    es_format_Copy( &fmt, &p_filter->fmt_in );
> +    /* Clear filter chain */
> +    filter_chain_Reset( p_sys->p_chain, &p_filter->fmt_in, &fmt);
> +    /* Add adjust filter to turn frame black-and-white */
> +    ret = filter_chain_AppendFromString( p_sys->p_chain, "adjust{saturation=0}" );
> +    if ( ret == -1 )
> +    {
> +        msg_Err( p_filter, "Could not append filter to filter chain" );

You're leaking p_chain here...

> +        free( p_sys );
> +        return VLC_EGENERIC;
> +    }
> +    /* Add gaussian blur to the frame so to remove noise from the frame */
> +    ret = filter_chain_AppendFromString( p_sys->p_chain, "gaussianblur{deviation=1}" );
> +    if ( ret == -1 )
> +    {
> +        msg_Err( p_filter, "Could not append filter to filter chain" );

...and here.

> +        free( p_sys );
> +        return VLC_EGENERIC;
> +    }
> +    /* Set callback function */
> +    p_filter->pf_video_filter = Filter;
> +    return VLC_SUCCESS;
> +}
> +
> +/******************************************************************************
> + * Closes the filter and cleans up all dynamically allocated data.
> + ******************************************************************************/
> +static int Close( vlc_object_t *p_this )
> +{
> +    filter_t *p_filter = (filter_t *)p_this;
> +    filter_chain_Delete( p_filter->p_sys->p_chain );
> +    free( p_filter->p_sys );
> +    return VLC_SUCCESS;
> +}
> +
> +/* *****************************************************************************
> + * Allocates a new buffer for the filter chain.
> + ******************************************************************************/
> +static picture_t *new_frame( filter_t *p_filter)
> +{
> +    return filter_NewPicture( p_filter->owner.sys );
> +}
> +
> +/******************************************************************************
> + * Callback function.
> + * This function implements the sobel operator which can be used to detect
> + * edges in a black-and-white image. The sobel operator is applied to
> + * every pixel in the frame for both X and Y axis.
> + ******************************************************************************/
> +static picture_t *Filter( filter_t *p_filter, picture_t *p_pic )
> +{
> +    picture_t *p_filtered_frame =
> +        filter_chain_VideoFilter( p_filter->p_sys->p_chain, p_pic );
> +    picture_t *p_out_frame = picture_NewFromFormat( &p_pic->format );
> +    if ( p_out_frame == NULL )
> +    {
> +        msg_Err( p_filter, "Could not allocate memory for new frame" );
> +        return NULL;
> +    }
> +    const int i_lines = p_filtered_frame->p[Y_PLANE].i_visible_lines;
> +    const int i_pitch = p_filtered_frame->p[Y_PLANE].i_pitch;
> +    /* Loop through every pixel in the frame */
> +    for ( int i_line = 1; i_line < i_lines - 1; i_line++ )
> +    {
> +        for ( int i_col = 1; i_col < i_pitch - 1; i_col++ )
> +        {
> +            /* Set the new value for the pixel */
> +            *( p_out_frame->p[Y_PLANE].p_pixels + ((i_pitch * i_line) + i_col) ) =
> +                (uint8_t) sobel( p_filtered_frame->p[Y_PLANE].p_pixels,
> +                       i_pitch, i_lines, i_col, i_line );

You probably want to clip these values to the range [0, 255] instead
of casting like this, here's the difference.
http://people.xiph.org/~tmatth/no-clipping.png
http://people.xiph.org/~tmatth/clipping.png

> +        }
> +    }
> +    picture_Release( p_filtered_frame );
> +    return p_out_frame;
> +}
> +
> +/******************************************************************************
> + * Sobel Operator.
> + * Calculates the gradients for both X and Y directions in a frame.
> + ******************************************************************************/
> +static int sobel( uint8_t const *p_pixels, const int i_pitch, const int i_lines,
> +                   const int i_col, const int i_line )
> +{
> +    int i_x_val = 0;
> +    int i_y_val = 0;
> +    /* Check for frame boundary */
> +    if ( i_line == 0 || i_line == i_lines )
> +    {
> +        i_y_val = 0;
> +    }
> +    else if( i_col == 0 || i_col == i_pitch )
> +    {
> +        i_x_val = 0;
> +    }

Since you're already skipping these cases in your Filter function, you
don't actually need this conditional, and besides, you've already
initialized these values so the if and else if branches have no
effect.
,
> +    /* General case where Sobel operator is applied */
> +    else
> +    {
> +        /* Apply the kernel to the pixel */
> +        for ( int i_x = 0; i_x < 3; i_x++ )
> +        {
> +            for ( int i_y = 0; i_y < 3; i_y++ )
> +            {
> +                i_x_val += pi_kernel_x[i_x][i_y] *
> +                    p_pixels[i_pitch * (i_line + i_y - 1) + (i_col + i_x - 1)];
> +                i_y_val += pi_kernel_y[i_x][i_y] *
> +                    p_pixels[i_pitch * (i_line + i_y + 1) + (i_col + i_x - 1)];
> +            }
> +        }
> +    }
> +    return abs(i_x_val) + abs(i_y_val);
> +}
> --