Hi Wan-Teh,
On Tue, Jul 11, 2017 at 4:53 PM, Wan-Teh Chang <w...@google.com> wrote:
> Hi Ronald,
>
> I have a question about this patch.
>
> On Mon, Apr 3, 2017 at 7:24 AM, Ronald S. Bultje <rsbul...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
