[FFmpeg-devel] [PATCH 1/2] lavc/threadframe: allow decoders to attach buffers to ThreadFrame

[Andreas Rheinhardt]

Anton Khirnov:
> This may be useful for synchronizing side data that only becomes
> available after ff_thread_finish_setup() is called.
> ---
>  libavcodec/pthread_frame.c | 1 +
>  libavcodec/threadframe.h   | 3 +++
>  libavcodec/utils.c         | 7 +++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 33b5a2e628..4da3832942 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -1138,6 +1138,7 @@ fail:
>  void ff_thread_release_ext_buffer(AVCodecContext *avctx, ThreadFrame *f)
>  {
>      av_buffer_unref(&f->progress);
> +    av_buffer_unref(&f->priv_buf);
>      f->owner[0] = f->owner[1] = NULL;
>      ff_thread_release_buffer(avctx, f->f);
>  }
> diff --git a/libavcodec/threadframe.h b/libavcodec/threadframe.h
> index dea4dadc6d..c2ddc2969f 100644
> --- a/libavcodec/threadframe.h
> +++ b/libavcodec/threadframe.h
> @@ -30,6 +30,9 @@ typedef struct ThreadFrame {
>      // progress->data is an array of 2 ints holding progress for top/bottom
>      // fields
>      AVBufferRef *progress;
> +
> +    /* arbitrary user data propagated along with the frame */
> +    AVBufferRef *priv_buf;
>  } ThreadFrame;
>  
>  /**
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 066da76e16..cc2c2715b3 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -881,6 +881,12 @@ int ff_thread_ref_frame(ThreadFrame *dst, const ThreadFrame *src)
>          return AVERROR(ENOMEM);
>      }
>  
> +    if (src->priv_buf) {
> +        dst->priv_buf = av_buffer_ref(src->priv_buf);
> +        if (!dst->priv_buf)
> +            return AVERROR(ENOMEM);
> +    }
> +
>      return 0;
>  }
>  
> @@ -913,6 +919,7 @@ void ff_thread_release_ext_buffer(AVCodecContext *avctx, ThreadFrame *f)
>      f->owner[0] = f->owner[1] = NULL;
>      if (f->f)
>          av_frame_unref(f->f);
> +    av_buffer_unref(&f->priv_buf);
>  }
>  
>  void ff_thread_finish_setup(AVCodecContext *avctx)

This approach has the downside that you have to add the priv_buf before
ff_thread_finish_setup(). So in case it is not apparent initially
whether one needs this one is forced to add it (even if it turns out not
to be needed); it will also necessitate two av_buffer_ref() in. A better
approach would be to replace the progress array (of two atomic ints)
with a struct containing these two atomic ints and whatever data needs
to be shared. The owner should logically also be part of this struct,
yet I could not figure out if this is compatible with current h264dec
last time I looked at this (when I wrote the patchset containing
02220b88fc38ef9dd4f2d519f5d3e4151258b60c); the current way of doing
things allows different threads to have different opinions about the
ownership of the frames.

(My actual aim with this patchset was to move the AVFrame into the
aforementioned structure like so:

struct {
    atomic_int progress[2];
    AVFrame *f;
};

This would avoid the need for the av_frame_ref() in
ff_thread_ref_frame(); therefore all the frame's properties could be set
directly on the AVFrame by its owner as long as the frame is not
finished. The non-owners could read the data subject to the current
limitations (i.e. they have to wait for progress) and could read
anything after the frame is finished (progress == INT_MAX; codecs could
of course use their own semantics here if they wished).
There were two reasons why I didn't finish this approach: 1. How to
synchronize in case of two owners? (Happens only for h264dec.) 2. This
would add an av_frame_alloc() for every frame, even when not using frame
threading. The latter can be easily avoided, but avoiding this with
frame-threading would require a smarter pool-implementation. And I hate
to use/extend the AVBuffer-API for something for which it is simply the
wrong tool and use something that does not require an allocation for
every ref. (It would nevertheless have been advantageous
allocation-wise, because one saves the allocations implicit in
av_frame_ref().))

- Andreas