[vlc-devel] [PATCH] picture_pool: fix double mutex_unlock if pic_lock fails

[Thomas Guillem]

On Mon, Nov 3, 2014, at 12:12, Rémi Denis-Courmont wrote:
> Le 2014-11-03 13:52, Thomas Guillem a écrit :
> > On Mon, Nov 3, 2014, at 11:44, Rémi Denis-Courmont wrote:
> >> Le 2014-11-03 13:22, Thomas Guillem a écrit :
> >> > ---
> >> >  src/misc/picture_pool.c | 1 -
> >> >  1 file changed, 1 deletion(-)
> >> >
> >> > diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> >> > index 15d0791..dac7ef6 100644
> >> > --- a/src/misc/picture_pool.c
> >> > +++ b/src/misc/picture_pool.c
> >> > @@ -253,7 +253,6 @@ picture_t *picture_pool_Get(picture_pool_t 
> >> *pool)
> >> >              vlc_mutex_lock(&pool->lock);
> >> >              sys->in_use = false;
> >> >              pool->refs--;
> >> > -            vlc_mutex_unlock(&pool->lock);
> >> >              continue;
> >> >          }
> >>
> >> The patch is obviously correct.
> >>
> >> I suspect you are abusing the lock feature though. Locking a picture
> >> should only fail in exceptional circumstances such as OOM. To retain
> >> "busy" pictures, the video output must use
> >> picture_Hold()/picture_Release(), not lock/unlock.
> >
> > I didn't write the code that fails here.
> > But I had a look, and found where if fails:
> >
> >     if (info->w != aligned_width || info->h != sh ||
> >     sys->b_changed_crop) {
> >         // input size doesn't match the surface size -> request a 
> > resize
> >         jni_SetAndroidSurfaceSize(aligned_width, sh,
> >         sys->fmt.i_visible_width, sys->fmt.i_visible_height,
> >         sys->i_sar_num, sys->i_sar_den);
> >         // When using ANativeWindow, one should use
> >         ANativeWindow_setBuffersGeometry
> >         // to set the size and format. In our case, these are set via
> >         the SurfaceHolder
> >         // in Java, so we seem to manage without calling this
> >         ANativeWindow function.
> >         sys->s_unlockAndPost(surf);
> >         jni_UnlockAndroidSurface();
> >         sys->b_changed_crop = false;
> >         return VLC_EGENERIC;
> >     }
> >
> > I think, instead of returning return VLC_EGENERIC, we should try 
> > again
> > to lock a picture from android.
> 
> That code smells very fishy.
> 
> - Pictures in a pool cannot change size. The video format, including 
> size, is set when the pool is created. To change picture sizes, the 
> video output MUST emit an invalid pictures event. Then it can recreate a 
> new pool with the new video format when the pool() callback gets called 
> again.
> 
> - The unlock callback means that the picture content is ready. It does 
> NOT mean that the picture is to be rendered immediately. To render a 
> picture, there are two options:
>    - schedule it based on the picture_t.date in the render() video 
> output callback (like VDPAU does),
>   or
>    - render immediately in the display() video output callback (like 
> most plugins).
> 

Ok, I add the surface.c rewrite in my TODO list.

One other big issue is that jni_SetAndroidSurfaceSize is asynchronous,
so next android lock may return a picture with old size.
We need to call ANativeWindow_setBuffersGeometry that is syncrhonous
when we reconfigure the pool.

> -- 
> Rémi Denis-Courmont
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel