[vlc-devel] commit: video_filter_puzzle: remove unnedeed structure ( it does not change anything to the race conditions already present). ( Rémi Duraffort )

[Laurent Aimar]

On Tue, Oct 20, 2009, git version control wrote:
> vlc | branch: master | Rémi Duraffort <ivoire at videolan.org> | Tue Oct 20 13:05:31 2009 +0200| [a114dc512cec6aa89211f1d107acc58bfca4be26] | committer: Rémi Duraffort 
> 
> video_filter_puzzle: remove unnedeed structure (it does not change anything to the race conditions already present).
 Why?

> diff --git a/modules/video_filter/puzzle.c b/modules/video_filter/puzzle.c
> index 2505401..5ad755d 100644
> --- a/modules/video_filter/puzzle.c
> +++ b/modules/video_filter/puzzle.c
> @@ -97,12 +97,6 @@ struct filter_sys_t
>      /* */
>      vlc_mutex_t lock;
>      bool b_change;
> -    struct
> -    {
> -        int i_cols;
> -        int i_rows;
> -        bool b_blackslot;
> -    } change;
>  };
 Unless I am wrong, when change.* is accessed, the lock is always taken.

Like here (where a local copy is done to ensure coherency):
>      vlc_mutex_lock( &p_sys->lock );
>      if( p_sys->b_change )
>      {
> -        p_sys->i_rows      = p_sys->change.i_rows;
> -        p_sys->i_cols      = p_sys->change.i_cols;
> -        p_sys->b_blackslot = p_sys->change.b_blackslot;
>          p_sys->b_change = false;
> -
>          Shuffle( p_sys );
>      }
>      vlc_mutex_unlock( &p_sys->lock );

And here:
> @@ -374,15 +361,15 @@ static int PuzzleCallback( vlc_object_t *p_this, char const *psz_var,
>      vlc_mutex_lock( &p_sys->lock );
>      if( !strcmp( psz_var, CFG_PREFIX "rows" ) )
>      {
> -        p_sys->change.i_rows = __MAX( 1, newval.i_int );
> +        p_sys->i_rows = __MAX( 1, newval.i_int );
>      }
>      else if( !strcmp( psz_var, CFG_PREFIX "cols" ) )
>      {
> -        p_sys->change.i_cols = __MAX( 1, newval.i_int );
> +        p_sys->i_cols = __MAX( 1, newval.i_int );
>      }
>      else if( !strcmp( psz_var, CFG_PREFIX "black-slot" ) )
>      {
> -        p_sys->change.b_blackslot = newval.b_bool;
> +        p_sys->b_blackslot = newval.b_bool;
>      }
>      p_sys->b_change = true;
>      vlc_mutex_unlock( &p_sys->lock );

 Unless wrong, now there are problems because the cohereny is no more
ensured. The lock must now be held (and it is not) while the whole Filter()
function is executed whereas change.* prevented such need.
 I think it would be best to revert it ASAP.

-- 
fenrir