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