[vlc-devel] [PATCH 1/2] Anti-flicker filter initial version
Laurent Aimar
fenrir at elivagar.org
Sun May 1 13:25:42 CEST 2011
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
More information about the vlc-devel
mailing list