[FFmpeg-devel] [PATCH] pthread_frame: allow per-field ThreadFrame owners.

[Wan-Teh Chang]

Hi Ronald,

I have a question about this patch.

On Mon, Apr 3, 2017 at 7:24 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> This tries to handle cases where separate invocations of decode_frame()
> (each running in separate threads) write to respective fields in the
> same AVFrame->data[]. Having per-field owners makes interaction between
> readers (the referencing thread) and writers (the decoding thread)
> slightly more optimal if both accesses are field-based, since they will
> use the respective producer's thread objects (mutex/cond) instead of
> sharing the thread objects of the first field's producer.
>
> In practice, this fixes the following tsan-warning in fate-h264:
>
> WARNING: ThreadSanitizer: data race (pid=21615)
>   Read of size 4 at 0x7d640000d9fc by thread T2 (mutexes: write M1006):
>     #0 ff_thread_report_progress pthread_frame.c:569 (ffmpeg:x86_64+0x100f7cf54)
> [..]
>   Previous write of size 4 at 0x7d640000d9fc by main thread (mutexes: write M1004):
>     #0 update_context_from_user pthread_frame.c:335 (ffmpeg:x86_64+0x100f81abb)
> ---
>  libavcodec/h264_slice.c    |  8 +++++---
>  libavcodec/pthread_frame.c | 18 ++++++++++--------
>  libavcodec/thread.h        |  2 +-
>  libavcodec/utils.c         |  7 ++++---
>  4 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index fa1e9ae..d4d31cc 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1423,14 +1423,14 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl,
>       * We have to do that before the "dummy" in-between frame allocation,
>       * since that can modify h->cur_pic_ptr. */
>      if (h->first_field) {
> +        int last_field = last_pic_structure == PICT_BOTTOM_FIELD;
>          av_assert0(h->cur_pic_ptr);
>          av_assert0(h->cur_pic_ptr->f->buf[0]);
>          assert(h->cur_pic_ptr->reference != DELAYED_PIC_REF);
>
>          /* Mark old field/frame as completed */
> -        if (h->cur_pic_ptr->tf.owner == h->avctx) {
> -            ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX,
> -                                      last_pic_structure == PICT_BOTTOM_FIELD);
> +        if (h->cur_pic_ptr->tf.owner[last_field] == h->avctx) {
> +            ff_thread_report_progress(&h->cur_pic_ptr->tf, INT_MAX, last_field);
>          }
>
>          /* figure out if we have a complementary field pair */
> @@ -1568,7 +1568,9 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl,
>              return AVERROR_INVALIDDATA;
>          }
>      } else {
> +        int field = h->picture_structure == PICT_BOTTOM_FIELD;
>          release_unused_pictures(h, 0);
> +        h->cur_pic_ptr->tf.owner[field] = h->avctx;
>      }
>      /* Some macroblocks can be accessed before they're available in case
>      * of lost slices, MBAFF or threading. */

Note: I have to admit I don't understand the changes to
libavcodec/h264_slice.c. The changes to the other files are
straightforward, except for the one issue I ask about below.

> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 3e8677d..0c68836 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -3971,7 +3971,8 @@ int ff_thread_ref_frame(ThreadFrame *dst, ThreadFrame *src)
>  {
>      int ret;
>
> -    dst->owner = src->owner;
> +    dst->owner[0] = src->owner[0];
> +    dst->owner[1] = src->owner[1];
>
>      ret = av_frame_ref(dst->f, src->f);
>      if (ret < 0)
> @@ -3981,7 +3982,7 @@ int ff_thread_ref_frame(ThreadFrame *dst, ThreadFrame *src)
>
>      if (src->progress &&
>          !(dst->progress = av_buffer_ref(src->progress))) {
> -        ff_thread_release_buffer(dst->owner, dst);
> +        ff_thread_release_buffer(dst->owner[0], dst);
>          return AVERROR(ENOMEM);
>      }
>
[...]

I don't understand why we pass dst->owner[0], not dst->owner[1], to
the ff_thread_release_buffer() call. Does this assume dst->owner[0] ==
dst->owner[1]?

Although dst->owner[0] and dst->owner[1] are initialized to the same
value, the changes to libavcodec/h264_slice.c seem to imply
dst->owner[0] and dst->owner[1] may become different.

Thanks,
Wan-Teh Chang