[vlc-devel] [PATCH 1/4] vmem: fixed picture pool use

[sergey radionov]

2016-01-30 20:36 GMT+06:00 Rémi Denis-Courmont <remi at remlab.net>:

> Le 2016-01-30 10:27, Sergey Radionov a écrit :
>
>> ---
>>  modules/video_output/vmem.c | 36 +++++++++++++++++++++++-------------
>>  1 file changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/modules/video_output/vmem.c b/modules/video_output/vmem.c
>> index 5a6d869..ce9584d 100644
>> --- a/modules/video_output/vmem.c
>> +++ b/modules/video_output/vmem.c
>> @@ -3,6 +3,7 @@
>>
>>
>>
>> *****************************************************************************
>>   * Copyright (C) 2008 VLC authors and VideoLAN
>>   * Copyrgiht (C) 2010 Rémi Denis-Courmont
>> + * Copyrgiht (C) 2016 Sergey Radionov <rsatom at gmail.com>
>>   *
>>   * Authors: Sam Hocevar <sam at zoy.org>
>>   *
>> @@ -109,32 +110,37 @@ static void           Prepare(vout_display_t *,
>> picture_t *, subpicture_t *);
>>  static void           Display(vout_display_t *, picture_t *,
>> subpicture_t *);
>>  static int            Control(vout_display_t *, int, va_list);
>>
>> -static void Lock(void *data, picture_t *pic)
>> +static int Lock(picture_t *pic)
>>  {
>> -    vout_display_sys_t *sys = data;
>>      picture_sys_t *picsys = pic->p_sys;
>> +    vout_display_sys_t *sys = picsys->sys;
>>      void *planes[PICTURE_PLANE_MAX];
>> +    memset(planes, 0, sizeof(planes));
>>
>
> memset()ing pointers does not make much sense.
>

it's just to have ability check if it was changed inside callback.


>
>
>>      picsys->id = sys->lock(sys->opaque, planes);
>>
>> +    for (int i = 0; i < pic->i_planes; ++i) {
>> +        if (!planes[i])
>> +            return VLC_EGENERIC;
>>
>
> Wut? Why?


Sometimes memory buffer could be not available right on startup. But this
not mandatory, and could be removed. Should I?


>
> +    }
>> +
>>      for (int i = 0; i < pic->i_planes; i++)
>>          pic->p[i].p_pixels = planes[i];
>> +
>> +    return VLC_SUCCESS;
>>  }
>>
>> -static void Unlock(void *data, picture_t *pic)
>> +static void Unlock(picture_t *pic)
>>  {
>> -    vout_display_sys_t *sys = data;
>>      picture_sys_t *picsys = pic->p_sys;
>> +    vout_display_sys_t *sys = picsys->sys;
>>      void *planes[PICTURE_PLANE_MAX];
>>
>> -    assert(!picture_IsReferenced(pic));
>> -
>>      for (int i = 0; i < pic->i_planes; i++)
>>          planes[i] = pic->p[i].p_pixels;
>>
>>      if (sys->unlock != NULL)
>>          sys->unlock(sys->opaque, picsys->id, planes);
>> -
>>  }
>>
>>
>>
>>
>> /*****************************************************************************
>> @@ -269,7 +275,6 @@ static void Close(vlc_object_t *object)
>>
>>      if (sys->pool)
>>      {
>> -        picture_pool_Enum(sys->pool, Unlock, sys);
>>          picture_pool_Release(sys->pool);
>>      }
>>      free(sys);
>> @@ -315,23 +320,29 @@ static picture_pool_t *Pool(vout_display_t *vd,
>> unsigned count)
>>      }
>>
>>      /* */
>> -    sys->pool = picture_pool_New(count, pictures);
>> +    picture_pool_configuration_t cfg = {
>> +        .picture_count = count,
>> +        .picture = pictures,
>> +        .lock = Lock,
>> +        .unlock = Unlock
>>
>
> lock and unlock callbacks are quite broken, and mostly unfixable or not
> worth fixing. I would rather not use them.


Hm... but some modules use it:
https://github.com/videolan/vlc/blob/master/modules/codec/omxil/vout.c#L318
https://github.com/videolan/vlc/blob/master/modules/video_output/directfb.c#L233

Does it mean it broken too?

And does it mean you will not accept patch with lock/unlock callbacks?


>
> +    };
>> +
>> +    sys->pool = picture_pool_NewExtended(&cfg);
>>      if (!sys->pool) {
>>          for (unsigned i = 0; i < count; i++)
>>              picture_Release(pictures[i]);
>>      }
>>
>> -    picture_pool_Enum(sys->pool, Lock, sys);
>>      return sys->pool;
>>  }
>>
>>  static void Prepare(vout_display_t *vd, picture_t *pic, subpicture_t
>> *subpic)
>>  {
>> -    Unlock(vd->sys, pic);
>> +    VLC_UNUSED(vd);
>> +    VLC_UNUSED(pic);
>>      VLC_UNUSED(subpic);
>>  }
>>
>> -
>>  static void Display(vout_display_t *vd, picture_t *pic, subpicture_t
>> *subpic)
>>  {
>>      vout_display_sys_t *sys = vd->sys;
>> @@ -339,7 +350,6 @@ static void Display(vout_display_t *vd, picture_t
>> *pic, subpicture_t *subpic)
>>      if (sys->display != NULL)
>>          sys->display(sys->opaque, pic->p_sys->id);
>>
>> -    Lock(sys, pic);
>>      picture_Release(pic);
>>      VLC_UNUSED(subpic);
>>  }
>>
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20160130/bbcf9bd3/attachment.html>