[vlc-devel] [PATCH 1/2] Anti-flicker filter initial version
dharani prabhu
dharani.prabhu.s at gmail.com
Sat May 7 21:37:41 CEST 2011
Hi,
Attached the patches addressing the review comments.
-Dharani
On Sun, May 1, 2011 at 4:55 PM, Laurent Aimar <fenrir at elivagar.org> wrote:
> Sorry for the late review:
>
> On Sat, Apr 16, 2011 at 01:53:02AM +0530, dharani prabhu wrote:
> > Attached the patches addressing the review comments. Had put the fixes
> into
> > original patch.
>
> > diff --git a/modules/video_filter/antiflicker.c
> b/modules/video_filter/antiflicker.c
> > new file mode 100755
> > index 0000000..9ba5b00
> > --- /dev/null
> > +++ b/modules/video_filter/antiflicker.c
> > @@ -0,0 +1,338 @@
> > +#ifdef HAVE_CONFIG_H
> > +# include "config.h"
> > +#endif
> > +
> > +#include <vlc_common.h>
> > +#include <vlc_plugin.h>
> > +#include <vlc_rand.h>
> Do you really need to include vlc_rand.h?
>
> >
> +/*****************************************************************************
> > + * Module descriptor
> > +
> *****************************************************************************/
> > +vlc_module_begin ()
> > + set_description( N_("antiflicker video filter") )
> > + set_shortname( N_( "antiflicker" ))
> > + set_capability( "video filter2", 0 )
> > + set_category( CAT_VIDEO )
> > + set_subcategory( SUBCAT_VIDEO_VFILTER )
> > +
> > + add_integer_with_range( FILTER_PREFIX "winsz", 10, 0, MAX_WINDOW_SZ,
> NULL,
> > + WINDOW_TEXT, WINDOW_LONGTEXT, false )
> > +
> > + add_integer_with_range( FILTER_PREFIX "sftn", 10, 0,
> MAX_SOFTENING_SZ, NULL,
> > + SFTN_TEXT, SFTN_LONGTEXT, false )
> For both: I really don't think that using abreviation for the
> configuration name
> is simpler/better.
> > +
> > + add_shortcut( "antiflicker" )
> > + set_callbacks( Create, Destroy )
> > +vlc_module_end ()
>
> >
> +/*****************************************************************************
> > + * Create: allocates Distort video thread output method
> > +
> *****************************************************************************
> > + * This function allocates and initializes a Distort vout method.
> > +
> *****************************************************************************/
> > +static int Create( vlc_object_t *p_this )
> > +{
> > + filter_t *p_filter = (filter_t *)p_this;
> > +
> > + /* Allocate structure */
> > + p_filter->p_sys = malloc( sizeof( filter_sys_t ) );
> sizeof(*p_filter->p_sys) is a bit safer.
>
> > + if( p_filter->p_sys == NULL )
> > + return VLC_ENOMEM;
> > +
> > + p_filter->pf_video_filter = Filter;
> > +
> > + /* Initialize the arguments */
> > + p_filter->p_sys->i_window_size = var_CreateGetIntegerCommand(
> p_filter,
> > + FILTER_PREFIX "winsz" );
> > + p_filter->p_sys->i_softening = var_CreateGetIntegerCommand(
> p_filter,
> > + FILTER_PREFIX "sftn" );
> > +
> > + memset( p_filter->p_sys->ia_luminance_data, 0,
> > + sizeof(p_filter->p_sys->ia_luminance_data) );
> > + p_filter->p_sys->p_old_data = NULL;
> > + p_filter->p_sys->ia_luminance_data[0] = 256;
> > +
> > + vlc_mutex_init( &p_filter->p_sys->lock );
> > + var_AddCallback(p_filter,FILTER_PREFIX "winsz",
> > + AntiFlickerCallback, p_filter->p_sys);
> > + var_AddCallback(p_filter,FILTER_PREFIX "sftn",
> > + AntiFlickerCallback, p_filter->p_sys);
> > +
> > + return VLC_SUCCESS;
> > +}
> > +
> >
> +/*****************************************************************************
> > + * GetLuminanceAvg : The funtion returns the luminance average for a
> picture
> > +
> *****************************************************************************/
> > +static int GetLuminanceAvg( picture_t * p_pic )
> > +{
> > + uint8_t *p_yplane_out = p_pic->p[Y_PLANE].p_pixels;
> > +
> > + int i_num_lines = p_pic->p[Y_PLANE].i_visible_lines;
> > + int i_num_cols = p_pic->p[Y_PLANE].i_visible_pitch;
> > + int i_in_pitch = p_pic->p[Y_PLANE].i_pitch;
> If you use i_visible_*, you must also use i_?_offset (which define the
> top-left of
> position the rectangle inside the video surface).
>
> > +
> > + uint32_t lum_sum = 0;
> > + uint8_t lum_avg = 0;
> > + for( int i_line = 0 ; i_line < i_num_lines ; i_line++ )
> > + {
> > + for( int i_col = 0 ; i_col < i_num_cols; ++i_col )
> > + {
> > + lum_sum += p_yplane_out[i_line*i_in_pitch+i_col];
> > + }
> > + }
> > + lum_avg = lum_sum / ( i_num_lines * i_num_cols );
> I am not 100% sure that the case where 'i_visible_lines or i_visible_pitch
> == 0' does
> not happens.
> > + return lum_avg;
> No need of the temporary lum_avg variable.:
> > +}
> > +
> >
> +/*****************************************************************************
> > + * Filter: adjust the luminance value and renders
> > +
> *****************************************************************************
> > + * The function uses moving average of past frames to adjust the
> luminance
> > + * of current frame also applies temporaral smoothening if enabled.
> > +
> *****************************************************************************/
> > +static picture_t *Filter( filter_t *p_filter, picture_t *p_pic )
> > +{
> > + if( !p_pic ) return NULL;
> > +
> > + picture_t *p_outpic = filter_NewPicture( p_filter );
> > + if( !p_outpic )
> > + {
> > + msg_Warn( p_filter, "can't get output picture" );
> No need to print this message, filter_NewPicture() already does it.
> > + picture_Release( p_pic );
> > + return NULL;
> > + }
> > +
> > + /****************** Get variables *************************/
> > +
> > + uint8_t i_window_size = 10;
> > + uint8_t i_softening = 0;
> Usesless initializations.
> > +
> > + vlc_mutex_lock( &p_filter->p_sys->lock );
> > + i_window_size = p_filter->p_sys->i_window_size;
> > + i_softening = p_filter->p_sys->i_softening;
> > + vlc_mutex_unlock( &p_filter->p_sys->lock );
> > +
> > + uint8_t *p_yplane_in = p_pic->p[Y_PLANE].p_pixels;
> > + uint8_t *p_yplane_out = p_outpic->p[Y_PLANE].p_pixels;
> > + bool scene_changed = false;
> > +
> > + int16_t i_num_lines = p_pic->p[Y_PLANE].i_visible_lines;
> > + int16_t i_num_cols = p_pic->p[Y_PLANE].i_visible_pitch;
> > + int16_t i_in_pitch = p_pic->p[Y_PLANE].i_pitch;
> > + int16_t i_out_pitch = p_outpic->p[Y_PLANE].i_pitch;
> > +
> > + /******** Get the luminance average for the current picture
> ********/
> > + uint8_t lum_avg = GetLuminanceAvg( p_pic );
> > +
> > + /******** Identify scene changes ************/
> > + if( abs((int)lum_avg - (int)p_filter->p_sys->
> > + ia_luminance_data[i_window_size - 1])
> > + > SCENE_CHANGE_THRESHOLD )
> Why not changing the return type of GetLuminanceAvg() and the type of
> ia_luminance_data to int? That would avoid the casts here.
>
> > + {
> > + scene_changed = true;
> > + }
>
> > +
> > + /******* Compute the adjustment factor using moving average
> ********/
> > + double scale = 1.0;
> > +
> > + if ( scene_changed )
> > + {
> > + //reset the luminance data
> > + for (int i = 0; i < i_window_size; ++i)
> > + p_filter->p_sys->ia_luminance_data[i] = lum_avg;
> > + }
> > + else
> > + {
> > + for (int i = 0; i < i_window_size-1 ; ++i)
> > + p_filter->p_sys->ia_luminance_data[i] =
> > + p_filter->p_sys->ia_luminance_data[i+1];
> > +
> > + p_filter->p_sys->ia_luminance_data[i_window_size - 1] = lum_avg;
> > +
> > + if (lum_avg > 0)
> > + {
> > + scale = 1.0f/lum_avg;
> > + double filt = 0;
> > + for (int i = 0; i < i_window_size; i++)
> > + filt += (float) p_filter->p_sys->ia_luminance_data[i]
> /
> > + i_window_size;
> You could divide by i_window_size outside the loop (directly in the line
> below).
> > + scale *= filt;
> > + }
> > + }
> > +
> > + /********* Apply the adjustment factor to each pixel on Y_PLANE
> *********/
> > + for( int16_t i_line = 0 ; i_line < i_num_lines ; i_line++ )
> > + {
> > + for( int16_t i_col = 0; i_col < i_num_cols ; i_col++ )
> > + {
> > + if( scene_changed )
> > + p_yplane_out[i_line*i_out_pitch+i_col] = lum_avg;
> I don't understand. SHouldn't you simply copy the input picture instead?
> (If so, picture_CopyPixels() would do). Also moving the test on
> scene_changed
> ouside the loop would speed up things.
> > + else
> > + {
> > + uint8_t pixel_data =
> p_yplane_in[i_line*i_in_pitch+i_col];
> > + float s = scale;
> > + if ( scale * pixel_data > 255)
> > + s = 255.0 / (float)pixel_data;
> Do not use float here, it will be slow as hell.
> For example, precompute an integer scale (ouside the loop) like:
> int scale_shift = 8;
> int scale_num = _MIN(scale, 255) * (1 << scale_shift);
>
> then in the loop, do:
> int value = (pixel_data * scale_num + (1 << (scale_shift-1))) >>
> scale_shift;
> and then:
> p_yplane_out[i_line*i_out_pitch+i_col] = value > 255 ? 255 : value;
> > + p_yplane_out[i_line*i_out_pitch+i_col] = s *
> (float)pixel_data;
> > + }
> > + }
> > + }
> > +
> > + /***************** Copy the UV plane as such
> *****************************/
> > + plane_CopyPixels( &p_outpic->p[U_PLANE], &p_pic->p[U_PLANE] );
> > + plane_CopyPixels( &p_outpic->p[V_PLANE], &p_pic->p[V_PLANE] );
> > +
> > + if (scene_changed || i_softening == 0)
> > + {
> > + return CopyInfoAndRelease( p_outpic, p_pic );
> > + }
> > +
> > + /******* Temporal softening phase. Adapted from code by Steven Don
> ******/
> > + uint8_t *src1, *src2;
> > + long diff, ofs, sum;
> > +
> > + if( !p_filter->p_sys->p_old_data )
> > + {
> > + p_filter->p_sys->p_old_data =
> > + calloc( (i_num_lines * i_out_pitch + i_num_cols) ,
> sizeof(uint8_t) );
> Becarefull, you cannot do that. The i_visible_pitch/lines can change per
> picture...
> It would be better to allocate using i_width and i_height (in the Open()
> function for example, filter->fmt_in.video.i_*).
> And then change the code below, to take care that the pitch are not equal.
>
> Also, you initialize to 0, but for the first image, I think it would be
> better
> to not modify the output picture. For the image after a scene_change, I
> also
> think it would look nicer if you do not modify the output picture.
>
> > + msg_Warn( p_filter, "can't get output picture" );
> > + return NULL;
> > + }
> > +
> > + ofs = (i_num_lines * i_out_pitch + i_num_cols);
> > + src1 = p_outpic->p[Y_PLANE].p_pixels;
> > + src2 = p_filter->p_sys->p_old_data;
> > +
> > + do
> > + {
> > + diff = abs(*src1 - *src2);
> > + if (diff < i_softening)
> > + {
> > + if (diff > (i_softening >> 1))
> > + {
> > + sum = *src1 + *src1 + *src2;
> > + *src2 = sum / 3;
> > + }
> > + }
> > + else
> > + {
> > + *src2 = *src1;
> > + }
> > + *src1 = *src2;
> > + src1++; src2++;
> > + } while (--ofs);
> > +
> > + return CopyInfoAndRelease( p_outpic, p_pic );
> > +}
>
> Regards,
>
> --
> fenrir
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110508/22d8e710/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Antiflicker-filter-initial-version.patch
Type: application/octet-stream
Size: 14611 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110508/22d8e710/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Antiflicker-filter-initial-version.patch
Type: application/octet-stream
Size: 4276 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110508/22d8e710/attachment-0001.obj>
More information about the vlc-devel
mailing list