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]?
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
