[vlc-devel] [PATCH] Fixed memory leaks, removed unused statements and improved error checking

[Rémi Denis-Courmont]

Commit message does not seem right.

On Wednesday 03 February 2016 10:16:52 Odd-Arild Kristensen wrote:
> ---
>  modules/video_filter/Makefile.am     |   3 +
>  modules/video_filter/edgedetection.c | 225
> +++++++++++++++++++++++++++++++++++ 2 files changed, 228 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..b6f950a
> --- /dev/null
> +++ b/modules/video_filter/edgedetection.c
> @@ -0,0 +1,225 @@
> +/**************************************************************************
> **** + * 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
> +
> ***************************************************************************
> **/ +#ifdef HAVE_CONFIG_H
> +# include "config.h"
> +#endif
> +
> +#include <vlc_common.h>
> +#include <vlc_plugin.h>
> +#include <vlc_filter.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." )
> +#define WHITE 255
> +
> +/**************************************************************************
> *** + * 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 uint8_t 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}
> +};

Could probably use signed char.

> +
> +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 i_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;
> +    }

Not needed since your whole state is a single pointer.

> +
> +    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 */
> +    i_ret = filter_chain_AppendFromString( p_sys->p_chain,
> "adjust{saturation=0}" ); +    if ( i_ret == -1 )
> +    {
> +        msg_Err( p_filter, "Could not append filter to filter chain" );
> +        filter_chain_Delete( p_filter->p_sys->p_chain );
> +        free( p_sys );
> +        return VLC_EGENERIC;
> +    }
> +    /* Add gaussian blur to the frame so to remove noise from the frame */
> +    i_ret = filter_chain_AppendFromString( p_sys->p_chain,
> "gaussianblur{deviation=1}" ); +    if ( i_ret == -1 )
> +    {
> +        msg_Err( p_filter, "Could not append filter to filter chain" );
> +        filter_chain_Delete( p_filter->p_sys->p_chain );
> +        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 )
> +    {
> +        picture_Release( p_filtered_frame );
> +        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 = 0; i_line < i_lines; i_line++ )
> +    {
> +        for ( int i_col = 0; i_col < i_pitch; i_col++ )
> +        {
> +            /* Set the new value for the pixel */
> +            *( p_out_frame->p[Y_PLANE].p_pixels + ((i_pitch * i_line) +
> i_col) ) = +                sobel( p_filtered_frame->p[Y_PLANE].p_pixels,
> +                       i_pitch, i_lines, i_col, i_line );
> +        }
> +    }
> +    picture_Release( p_filtered_frame );
> +    return p_out_frame;
> +}
> +
> +/**************************************************************************
> **** + * Sobel Operator.
> + * Calculates the gradients for both X and Y directions in a frame.
> +
> ***************************************************************************
> ***/ +static uint8_t sobel( uint8_t const *p_pixels, const int i_pitch,
> const int i_lines, +                   const int i_col, const int i_line )

Const is probably useless here.

> +{
> +    int i_x_val = 0;
> +    int i_y_val = 0;
> +    /* Check that we are not at the frame's boundary */
> +    if ( i_line != 0 || i_line != i_lines || i_col != 0 || i_col != i_pitch

Doesn´t this suffer off-by-one?

> ) +    {
> +        /* 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)]; +            }
> +        }
> +    }
> +    int i_ret = abs(i_x_val) + abs(i_y_val);
> +    return (i_ret > WHITE) ? WHITE : i_ret;
> +}

-- 
Rémi Denis-Courmont
http://www.remlab.net/