[vlc-devel] [PATCH 2/7] picture_pool: add the video format requested when waiting for a picture

Alexandre Janniaux ajanni at videolabs.io
Fri Nov 6 17:02:10 CET 2020


Hi,

I'm not sure this is the correct level of abstraction
regarding how we discussed this originally. From what
I understood, we want to change the pool when the format
change, not adapt the previous pool. The picture_pool
even facilitates this by being reference-counted.

I guess your main issue with this is that previous pictures
would not be released until all pictures are released.
Maybe picture_pool_Release could implement some sort of
drain to release any current and future requeued picture
to handle this case smoothly instead? It would highly
simplify the API for handling this.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, Nov 06, 2020 at 03:42:09PM +0100, Steve Lhomme wrote:
> In push mode it's convenient to keep the same pool of buffers when only small
> changes to the video format occur.
>
> Passing NULL for the video format means no change to the original format is
> required.
> ---
>  include/vlc_picture_pool.h      |  2 +-
>  modules/hw/nvdec/hw_pool.c      |  4 ++--
>  modules/hw/nvdec/hw_pool.h      |  2 +-
>  modules/hw/nvdec/nvdec.c        |  2 +-
>  modules/hw/vaapi/chroma.c       |  2 +-
>  modules/hw/vaapi/filters.c      |  2 +-
>  src/input/decoder.c             |  2 +-
>  src/misc/picture.c              |  7 ++++---
>  src/misc/picture.h              |  2 +-
>  src/misc/picture_pool.c         | 13 +++++++------
>  src/test/picture_pool.c         |  2 +-
>  src/video_output/video_output.c |  2 +-
>  12 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/include/vlc_picture_pool.h b/include/vlc_picture_pool.h
> index aa5883ee521..8e325392d2c 100644
> --- a/include/vlc_picture_pool.h
> +++ b/include/vlc_picture_pool.h
> @@ -102,7 +102,7 @@ VLC_API picture_t * picture_pool_Get( picture_pool_t * ) VLC_USED;
>   *
>   * @note This function is thread-safe.
>   */
> -VLC_API picture_t *picture_pool_Wait(picture_pool_t *) VLC_USED;
> +VLC_API picture_t *picture_pool_Wait(picture_pool_t *, const video_format_t *) VLC_USED;
>
>  /**
>   * Cancel the picture pool.
> diff --git a/modules/hw/nvdec/hw_pool.c b/modules/hw/nvdec/hw_pool.c
> index 63c6f1911f3..3d117d66ca2 100644
> --- a/modules/hw/nvdec/hw_pool.c
> +++ b/modules/hw/nvdec/hw_pool.c
> @@ -103,9 +103,9 @@ error:
>      return NULL;
>  }
>
> -picture_t* nvdec_pool_Wait(nvdec_pool_t *pool)
> +picture_t* nvdec_pool_Wait(nvdec_pool_t *pool, const video_format_t *fmt)
>  {
> -    picture_t *pic = picture_pool_Wait(pool->picture_pool);
> +    picture_t *pic = picture_pool_Wait(pool->picture_pool, fmt);
>      if (!pic)
>          return NULL;
>
> diff --git a/modules/hw/nvdec/hw_pool.h b/modules/hw/nvdec/hw_pool.h
> index 59362116b3d..15bedfde868 100644
> --- a/modules/hw/nvdec/hw_pool.h
> +++ b/modules/hw/nvdec/hw_pool.h
> @@ -45,4 +45,4 @@ void nvdec_pool_Release(nvdec_pool_t *);
>   *
>   * The picture.p_sys is always NULL.
>   */
> -picture_t* nvdec_pool_Wait(nvdec_pool_t *);
> +picture_t* nvdec_pool_Wait(nvdec_pool_t *, const video_format_t *);
> diff --git a/modules/hw/nvdec/nvdec.c b/modules/hw/nvdec/nvdec.c
> index ebed90be51b..f16702f89ae 100644
> --- a/modules/hw/nvdec/nvdec.c
> +++ b/modules/hw/nvdec/nvdec.c
> @@ -419,7 +419,7 @@ static int CUDAAPI HandlePictureDisplay(void *p_opaque, CUVIDPARSERDISPINFO *p_d
>
>      if ( is_nvdec_opaque(p_dec->fmt_out.video.i_chroma) )
>      {
> -        p_pic = nvdec_pool_Wait(p_sys->out_pool);
> +        p_pic = nvdec_pool_Wait(p_sys->out_pool, &p_dec->fmt_out.video);
>          if (unlikely(p_pic == NULL))
>              return 0;
>          pic_context_nvdec_t *picctx = NVDEC_PICCONTEXT_FROM_PICCTX(p_pic->context);
> diff --git a/modules/hw/vaapi/chroma.c b/modules/hw/vaapi/chroma.c
> index f43e0dc1cd0..ff33c4a2f26 100644
> --- a/modules/hw/vaapi/chroma.c
> +++ b/modules/hw/vaapi/chroma.c
> @@ -242,7 +242,7 @@ UploadSurface(filter_t *filter, picture_t *src)
>      VADisplay const va_dpy = p_sys->dpy;
>      VAImage         dest_img;
>      void *          dest_buf;
> -    picture_t *     dest_pic = picture_pool_Wait(p_sys->dest_pics);
> +    picture_t *     dest_pic = picture_pool_Wait(p_sys->dest_pics, NULL);
>
>      if (!dest_pic)
>      {
> diff --git a/modules/hw/vaapi/filters.c b/modules/hw/vaapi/filters.c
> index bd933ac8c63..9174e21cd4d 100644
> --- a/modules/hw/vaapi/filters.c
> +++ b/modules/hw/vaapi/filters.c
> @@ -211,7 +211,7 @@ Filter(filter_t * filter, picture_t * src,
>  {
>      filter_sys_t *const filter_sys = filter->p_sys;
>      VABufferID          pipeline_buf = VA_INVALID_ID;
> -    picture_t *const    dest = picture_pool_Wait(filter_sys->dest_pics);
> +    picture_t *const    dest = picture_pool_Wait(filter_sys->dest_pics, NULL);
>      if (!dest)
>          return NULL;
>
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index 77ccd67e530..1610d300ae2 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -620,7 +620,7 @@ static picture_t *ModuleThread_NewVideoBuffer( decoder_t *p_dec )
>      assert( p_owner->p_vout );
>      assert( p_owner->out_pool );
>
> -    picture_t *pic = picture_pool_Wait( p_owner->out_pool );
> +    picture_t *pic = picture_pool_Wait( p_owner->out_pool, &p_dec->fmt_out.video );
>      if (pic)
>          picture_Reset( pic );
>      return pic;
> diff --git a/src/misc/picture.c b/src/misc/picture.c
> index e00b1303253..6902b9a8fcd 100644
> --- a/src/misc/picture.c
> +++ b/src/misc/picture.c
> @@ -423,7 +423,7 @@ static void picture_DestroyClone(picture_t *clone)
>      picture_Release(picture);
>  }
>
> -picture_t *picture_InternalClone(picture_t *picture,
> +picture_t *picture_InternalClone(picture_t *picture, const video_format_t *fmt,
>                                   void (*pf_destroy)(picture_t *), void *opaque)
>  {
>      picture_resource_t res = {
> @@ -437,7 +437,7 @@ picture_t *picture_InternalClone(picture_t *picture,
>          res.p[i].i_pitch = picture->p[i].i_pitch;
>      }
>
> -    picture_t *clone = picture_NewFromResource(&picture->format, &res);
> +    picture_t *clone = picture_NewFromResource(fmt ? fmt : &picture->format, &res);
>      if (likely(clone != NULL)) {
>          ((picture_priv_t *)clone)->gc.opaque = opaque;
>          picture_Hold(picture);
> @@ -447,7 +447,8 @@ picture_t *picture_InternalClone(picture_t *picture,
>
>  picture_t *picture_Clone(picture_t *picture)
>  {
> -    picture_t *clone = picture_InternalClone(picture, picture_DestroyClone, picture);
> +    picture_t *clone = picture_InternalClone(picture, NULL,
> +                                             picture_DestroyClone, picture);
>      if (likely(clone != NULL)) {
>          if (picture->context != NULL)
>              clone->context = picture->context->copy(picture->context);
> diff --git a/src/misc/picture.h b/src/misc/picture.h
> index 684eb2ada69..a815bb8a316 100644
> --- a/src/misc/picture.h
> +++ b/src/misc/picture.h
> @@ -36,4 +36,4 @@ typedef struct
>  void *picture_Allocate(int *, size_t);
>  void picture_Deallocate(int, void *, size_t);
>
> -picture_t * picture_InternalClone(picture_t *, void (*pf_destroy)(picture_t *), void *);
> +picture_t * picture_InternalClone(picture_t *,  const video_format_t *, void (*pf_destroy)(picture_t *), void *);
> diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> index d2df4bf2eb0..7438e5f1d6a 100644
> --- a/src/misc/picture_pool.c
> +++ b/src/misc/picture_pool.c
> @@ -85,13 +85,14 @@ static void picture_pool_ReleaseClone(picture_t *clone)
>  }
>
>  static picture_t *picture_pool_ClonePicture(picture_pool_t *pool,
> -                                            unsigned offset)
> +                                            unsigned offset,
> +                                            const video_format_t *fmt)
>  {
>      picture_t *picture = pool->picture[offset];
>      uintptr_t sys = ((uintptr_t)pool) + offset;
>
> -    picture_t *clone = picture_InternalClone(picture, picture_pool_ReleaseClone,
> -                                 (void*)sys);
> +    picture_t *clone = picture_InternalClone(picture, fmt,
> +                                             picture_pool_ReleaseClone, (void*)sys);
>      if (clone != NULL) {
>          assert(!picture_HasChainedPics(clone));
>          atomic_fetch_add_explicit(&pool->refs, 1, memory_order_relaxed);
> @@ -191,14 +192,14 @@ picture_t *picture_pool_Get(picture_pool_t *pool)
>          vlc_mutex_unlock(&pool->lock);
>          available &= ~(1ULL << i);
>
> -        return picture_pool_ClonePicture(pool, i);
> +        return picture_pool_ClonePicture(pool, i, NULL);
>      }
>
>      vlc_mutex_unlock(&pool->lock);
>      return NULL;
>  }
>
> -picture_t *picture_pool_Wait(picture_pool_t *pool)
> +picture_t *picture_pool_Wait(picture_pool_t *pool, const video_format_t *fmt)
>  {
>      vlc_mutex_lock(&pool->lock);
>      assert(pool->refs > 0);
> @@ -217,7 +218,7 @@ picture_t *picture_pool_Wait(picture_pool_t *pool)
>      pool->available &= ~(1ULL << i);
>      vlc_mutex_unlock(&pool->lock);
>
> -    return picture_pool_ClonePicture(pool, i);
> +    return picture_pool_ClonePicture(pool, i, fmt);
>  }
>
>  void picture_pool_Cancel(picture_pool_t *pool, bool canceled)
> diff --git a/src/test/picture_pool.c b/src/test/picture_pool.c
> index 9b43c25fdd4..e4b5491c241 100644
> --- a/src/test/picture_pool.c
> +++ b/src/test/picture_pool.c
> @@ -75,7 +75,7 @@ static void test(bool zombie)
>          picture_Release(pics[i]);
>
>      for (unsigned i = 0; i < PICTURES; i++) {
> -        pics[i] = picture_pool_Wait(pool);
> +        pics[i] = picture_pool_Wait(pool, NULL);
>          assert(pics[i] != NULL);
>      }
>
> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> index 20e569e173c..de515b4546e 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -468,7 +468,7 @@ picture_t *vout_GetPicture(vout_thread_t *vout)
>  {
>      vout_thread_sys_t *sys = VOUT_THREAD_TO_SYS(vout);
>      assert(!sys->dummy);
> -    picture_t *picture = picture_pool_Wait(sys->sys.private.display_pool);
> +    picture_t *picture = picture_pool_Wait(sys->sys.private.display_pool, NULL);
>      if (likely(picture != NULL)) {
>          picture_Reset(picture);
>          video_format_CopyCropAr(&picture->format, &sys->original);
> --
> 2.26.2
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list