[vlc-devel] [RFC PATCH 03/11] Android vout: check chroma before lib init

[Martin Storsjö]

On Tue, 24 Jun 2014, Thomas Guillem wrote:

> ---
> modules/video_output/android/surface.c |    9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/modules/video_output/android/surface.c b/modules/video_output/android/surface.c
> index 6aaa9ef..f6a183e 100644
> --- a/modules/video_output/android/surface.c
> +++ b/modules/video_output/android/surface.c
> @@ -173,6 +173,10 @@ static void *InitLibrary(vout_display_sys_t *sys)
> static int Open(vlc_object_t *p_this)
> {
>     vout_display_t *vd = (vout_display_t *)p_this;
> +    video_format_t fmt = vd->fmt;
> +
> +    if (fmt.i_chroma == VLC_CODEC_ANDROID_OPAQUE)
> +        return VLC_EGENERIC;
>
>     /* */
>     if (vlc_mutex_trylock(&single_instance) != 0) {
> @@ -200,11 +204,6 @@ static int Open(vlc_object_t *p_this)
>     }
>
>     /* Setup chroma */
> -    video_format_t fmt = vd->fmt;
> -
> -    if (fmt.i_chroma == VLC_CODEC_ANDROID_OPAQUE)
> -        return VLC_EGENERIC;
> -
>     char *psz_fcc = var_InheritString(vd, CFG_PREFIX "chroma");
>     if( psz_fcc ) {
>         fmt.i_chroma = vlc_fourcc_GetCodecFromString(VIDEO_ES, psz_fcc);
> -- 
> 1.7.10.4

A patch like this should always have an explanation on why you think this 
should be done. Is it only an optimization, to do less work in vain before 
aborting?

In this particular case, I see that the existing code has a bug and 
forgets to call vlc_mutex_unlock(&single_instance); in this case - so 
clearly you're fixing a bug. Then please mention this in the commit 
message; it's quite essential for anybody reading the commit log later.

The patch itself looks ok to me.

// Martin