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

[Rémi Denis-Courmont]

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).

-- 
Rémi Denis-Courmont