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

[Ronald S. Bultje]

Hi Wan-Teh,

On Tue, Jul 11, 2017 at 4:53 PM, Wan-Teh Chang <wtc at google.com> wrote:

> 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]?
>

Neither is perfect, ideally (from an API programming PoV) you'd want to use
both. But that's not possible... In practice, I don't think it matters
much, since all it does is decide which release_buffer-queue it'll be
appended to (if we do delayed free()'ing). Are there problems with this?

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.


Right, if each slice is decoded in a separate (frame-)worker thread, then
they will be different.

Ronald