Re: [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES
On Tue, Sep 12, 2023, at 15:11, Andreas Rheinhardt wrote:
> Thomas Guillem via ffmpeg-devel:
>> Same than the previous commit but with FF_DECODE_ERROR_DECODE_SLICES
>>
>> Fix the following data-race:
>>
>> WARNING: ThreadSanitizer: data race (pid=55935)
>> Write of size 4 at 0x7b509378 by thread T1 (mutexes: write M608):
>> #0 decode_nal_units src/libavcodec/h264dec.c:742 (ffmpeg+0xb19dd6)
>> #1 h264_decode_frame src/libavcodec/h264dec.c:1016 (ffmpeg+0xb19dd6)
>> #2 frame_worker_thread src/libavcodec/pthread_frame.c:228
>> (ffmpeg+0xdeea7e)
>>
>> Previous read of size 4 at 0x7b509378 by thread T14 (mutexes: write
>> M610):
>> #0 frame_copy_props src/libavutil/frame.c:321 (ffmpeg+0x1793759)
>> #1 av_frame_replace src/libavutil/frame.c:530 (ffmpeg+0x1794714)
>> #2 ff_thread_replace_frame src/libavcodec/utils.c:898 (ffmpeg+0xfb1d0f)
>> #3 ff_h264_replace_picture src/libavcodec/h264_picture.c:159
>> (ffmpeg+0x149cd3d)
>> #4 ff_h264_update_thread_context src/libavcodec/h264_slice.c:413
>> (ffmpeg+0x14abf04)
>> #5 update_context_from_thread src/libavcodec/pthread_frame.c:355
>> (ffmpeg+0xdec39c)
>> #6 submit_packet src/libavcodec/pthread_frame.c:494 (ffmpeg+0xdecee3)
>> #7 ff_thread_decode_frame src/libavcodec/pthread_frame.c:545
>> (ffmpeg+0xdecee3)
>> #8 decode_simple_internal src/libavcodec/decode.c:431 (ffmpeg+0x9e1e20)
>> #9 decode_simple_receive_frame src/libavcodec/decode.c:607
>> (ffmpeg+0x9e1e20)
>> #10 decode_receive_frame_internal src/libavcodec/decode.c:635
>> (ffmpeg+0x9e1e20)
>> #11 avcodec_send_packet src/libavcodec/decode.c:732 (ffmpeg+0x9e28fa)
>> #12 packet_decode src/fftools/ffmpeg_dec.c:555 (ffmpeg+0x229888)
>> #13 decoder_thread src/fftools/ffmpeg_dec.c:702 (ffmpeg+0x229888)
>> ---
>> libavcodec/h264dec.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
>> index 24e849fc5b..b82ca8f14f 100644
>> --- a/libavcodec/h264dec.c
>> +++ b/libavcodec/h264dec.c
>> @@ -739,7 +739,7 @@ static int decode_nal_units(H264Context *h, const
>> uint8_t *buf, int buf_size)
>>
>> // set decode_error_flags to allow users to detect concealed decoding
>> errors
>> if ((ret < 0 || h->er.error_occurred) && h->cur_pic_ptr) {
>> -h->cur_pic_ptr->f->decode_error_flags |=
>> FF_DECODE_ERROR_DECODE_SLICES;
>> +h->cur_pic_ptr->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
>> }
>>
>> ret = 0;
>
> IIRC this does not work: The thread that decodes a frame is typically
> not the same thread that outputs said frame. The H264Picture srcp in
> output_frame() points to one of the H264Picture in the H264Context.DBP
> of the outputting thread, not the decoding thread. The outputting
> threads decode_error_flags will therefore always be zero.
>
> My preferred way to fix this is to allocate the H264Pictures (or at
> least the stuff needed by later decoding threads) separately and make
> them shared between decoder threads (with the caveat that only the
> actual decoding threads may modify them; and they may only do so in a
> controlled manner (i.e. no changes after having signalled to finish the
> picture etc.). But this would be a major rewrite which will probably
> never happen.
>
> In the meantime i will send an alternative patch for this.
Thanks for your patches and your clear explanations !
>
> - Andreas
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE
Set the FF_DECODE_ERROR_CONCEALMENT_ACTIVE flags on the AVFrane before
outputing it. Store in in the H264Picture in the meantime, where it
won't be read/write by other threads.
Fix the following data-race:
WARNING: ThreadSanitizer: data race (pid=55134)
Write of size 4 at 0x7b507f78 by thread T1 (mutexes: write M58):
#0 decode_nal_units src/libavcodec/h264dec.c:783 (ffmpeg+0xb1a678)
#1 h264_decode_frame src/libavcodec/h264dec.c:1014 (ffmpeg+0xb1a678)
#2 frame_worker_thread src/libavcodec/pthread_frame.c:228 (ffmpeg+0xdeea6e)
Previous read of size 4 at 0x7b507f78 by thread T14 (mutexes: write M60):
#0 frame_copy_props src/libavutil/frame.c:321 (ffmpeg+0x1793739)
#1 av_frame_replace src/libavutil/frame.c:530 (ffmpeg+0x17946f4)
#2 ff_thread_replace_frame src/libavcodec/utils.c:898 (ffmpeg+0xfb1cff)
#3 ff_h264_replace_picture src/libavcodec/h264_picture.c:159
(ffmpeg+0x149cd2d)
#4 ff_h264_update_thread_context src/libavcodec/h264_slice.c:413
(ffmpeg+0x14abee4)
#5 update_context_from_thread src/libavcodec/pthread_frame.c:355
(ffmpeg+0xdec38c)
#6 submit_packet src/libavcodec/pthread_frame.c:494 (ffmpeg+0xdeced3)
#7 ff_thread_decode_frame src/libavcodec/pthread_frame.c:545
(ffmpeg+0xdeced3)
#8 decode_simple_internal src/libavcodec/decode.c:431 (ffmpeg+0x9e1e20)
#9 decode_simple_receive_frame src/libavcodec/decode.c:607 (ffmpeg+0x9e1e20)
#10 decode_receive_frame_internal src/libavcodec/decode.c:635
(ffmpeg+0x9e1e20)
#11 avcodec_send_packet src/libavcodec/decode.c:732 (ffmpeg+0x9e28fa)
#12 packet_decode src/fftools/ffmpeg_dec.c:555 (ffmpeg+0x229888)
#13 decoder_thread src/fftools/ffmpeg_dec.c:702 (ffmpeg+0x229888)
---
libavcodec/h264_slice.c | 1 +
libavcodec/h264dec.c| 4 +++-
libavcodec/h264dec.h| 2 ++
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6cd7bb8fe7..249c764d13 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -535,6 +535,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
pic->f->crop_right = h->crop_right;
pic->f->crop_top= h->crop_top;
pic->f->crop_bottom = h->crop_bottom;
+pic->decode_error_flags = 0;
pic->needs_fg = h->sei.common.film_grain_characteristics.present &&
!h->avctx->hwaccel &&
!(h->avctx->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN);
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 553f300c3d..24e849fc5b 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -780,7 +780,7 @@ end:
ff_h264_set_erpic(>er.next_pic, sl->ref_list[1][0].parent);
if (ff_er_frame_end(>er) > 0)
-h->cur_pic_ptr->f->decode_error_flags |=
FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+h->cur_pic_ptr->decode_error_flags |=
FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
if (use_last_pic)
memset(>ref_list[0][0], 0, sizeof(sl->ref_list[0][0]));
}
@@ -849,6 +849,8 @@ static int output_frame(H264Context *h, AVFrame *dst,
H264Picture *srcp)
if (ret < 0)
return ret;
+dst->decode_error_flags |= srcp->decode_error_flags;
+
if (srcp->needs_fg && (ret = av_frame_copy_props(dst, srcp->f)) < 0)
return ret;
diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h
index beaab3902c..cb3d5cef00 100644
--- a/libavcodec/h264dec.h
+++ b/libavcodec/h264dec.h
@@ -152,6 +152,8 @@ typedef struct H264Picture {
int mb_width, mb_height;
int mb_stride;
+
+int decode_error_flags;
} H264Picture;
typedef struct H264Ref {
--
2.39.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES
Same than the previous commit but with FF_DECODE_ERROR_DECODE_SLICES
Fix the following data-race:
WARNING: ThreadSanitizer: data race (pid=55935)
Write of size 4 at 0x7b509378 by thread T1 (mutexes: write M608):
#0 decode_nal_units src/libavcodec/h264dec.c:742 (ffmpeg+0xb19dd6)
#1 h264_decode_frame src/libavcodec/h264dec.c:1016 (ffmpeg+0xb19dd6)
#2 frame_worker_thread src/libavcodec/pthread_frame.c:228 (ffmpeg+0xdeea7e)
Previous read of size 4 at 0x7b509378 by thread T14 (mutexes: write M610):
#0 frame_copy_props src/libavutil/frame.c:321 (ffmpeg+0x1793759)
#1 av_frame_replace src/libavutil/frame.c:530 (ffmpeg+0x1794714)
#2 ff_thread_replace_frame src/libavcodec/utils.c:898 (ffmpeg+0xfb1d0f)
#3 ff_h264_replace_picture src/libavcodec/h264_picture.c:159
(ffmpeg+0x149cd3d)
#4 ff_h264_update_thread_context src/libavcodec/h264_slice.c:413
(ffmpeg+0x14abf04)
#5 update_context_from_thread src/libavcodec/pthread_frame.c:355
(ffmpeg+0xdec39c)
#6 submit_packet src/libavcodec/pthread_frame.c:494 (ffmpeg+0xdecee3)
#7 ff_thread_decode_frame src/libavcodec/pthread_frame.c:545
(ffmpeg+0xdecee3)
#8 decode_simple_internal src/libavcodec/decode.c:431 (ffmpeg+0x9e1e20)
#9 decode_simple_receive_frame src/libavcodec/decode.c:607 (ffmpeg+0x9e1e20)
#10 decode_receive_frame_internal src/libavcodec/decode.c:635
(ffmpeg+0x9e1e20)
#11 avcodec_send_packet src/libavcodec/decode.c:732 (ffmpeg+0x9e28fa)
#12 packet_decode src/fftools/ffmpeg_dec.c:555 (ffmpeg+0x229888)
#13 decoder_thread src/fftools/ffmpeg_dec.c:702 (ffmpeg+0x229888)
---
libavcodec/h264dec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 24e849fc5b..b82ca8f14f 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -739,7 +739,7 @@ static int decode_nal_units(H264Context *h, const uint8_t
*buf, int buf_size)
// set decode_error_flags to allow users to detect concealed decoding
errors
if ((ret < 0 || h->er.error_occurred) && h->cur_pic_ptr) {
-h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
+h->cur_pic_ptr->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES;
}
ret = 0;
--
2.39.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside
This will allow to fix data-races when ff_er_frame_end() is called after
ff_thread_finish_setup()
---
libavcodec/error_resilience.c | 12 ++--
libavcodec/error_resilience.h | 2 +-
libavcodec/h263dec.c | 6 --
libavcodec/h264dec.c | 3 ++-
libavcodec/mpeg12dec.c| 3 ++-
libavcodec/mss2.c | 8 +---
libavcodec/rv10.c | 10 --
libavcodec/rv34.c | 12 +---
libavcodec/vc1dec.c | 6 --
9 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/libavcodec/error_resilience.c b/libavcodec/error_resilience.c
index 68e20925e0..1f43b233ff 100644
--- a/libavcodec/error_resilience.c
+++ b/libavcodec/error_resilience.c
@@ -889,7 +889,7 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
}
}
-void ff_er_frame_end(ERContext *s)
+int ff_er_frame_end(ERContext *s)
{
int *linesize = NULL;
int i, mb_x, mb_y, error, error_type, dc_error, mv_error, ac_error;
@@ -906,7 +906,7 @@ void ff_er_frame_end(ERContext *s)
!er_supported(s) ||
atomic_load(>error_count) == 3 * s->mb_width *
(s->avctx->skip_top + s->avctx->skip_bottom)) {
-return;
+return 0;
}
linesize = s->cur_pic.f->linesize;
@@ -921,7 +921,7 @@ void ff_er_frame_end(ERContext *s)
if (mb_x == s->mb_width) {
av_log(s->avctx, AV_LOG_DEBUG, "ignoring last missing slice\n");
-return;
+return 0;
}
}
@@ -960,7 +960,7 @@ void ff_er_frame_end(ERContext *s)
s->cur_pic.ref_index[i] = NULL;
s->cur_pic.motion_val[i] = NULL;
}
-return;
+return 0;
}
}
@@ -1114,8 +1114,6 @@ void ff_er_frame_end(ERContext *s)
av_log(s->avctx, AV_LOG_INFO, "concealing %d DC, %d AC, %d MV errors in %c
frame\n",
dc_error, ac_error, mv_error,
av_get_picture_type_char(s->cur_pic.f->pict_type));
-s->cur_pic.f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
-
is_intra_likely = is_intra_more_likely(s);
/* set unknown mb-type to most likely */
@@ -1352,4 +1350,6 @@ void ff_er_frame_end(ERContext *s)
memset(>cur_pic, 0, sizeof(ERPicture));
memset(>last_pic, 0, sizeof(ERPicture));
memset(>next_pic, 0, sizeof(ERPicture));
+
+return 1;
}
diff --git a/libavcodec/error_resilience.h b/libavcodec/error_resilience.h
index 47cc8a4fc6..a8cf73c72e 100644
--- a/libavcodec/error_resilience.h
+++ b/libavcodec/error_resilience.h
@@ -90,7 +90,7 @@ typedef struct ERContext {
} ERContext;
void ff_er_frame_start(ERContext *s);
-void ff_er_frame_end(ERContext *s);
+int ff_er_frame_end(ERContext *s);
void ff_er_add_slice(ERContext *s, int startx, int starty, int endx, int endy,
int status);
diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c
index 52e51dd489..3e83d90586 100644
--- a/libavcodec/h263dec.c
+++ b/libavcodec/h263dec.c
@@ -621,8 +621,10 @@ retry:
av_assert1(s->bitstream_buffer_size == 0);
frame_end:
-if (!s->studio_profile)
-ff_er_frame_end(>er);
+if (!s->studio_profile) {
+if (ff_er_frame_end(>er) > 0)
+s->current_picture.f->decode_error_flags |=
FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+}
if (avctx->hwaccel) {
ret = FF_HW_SIMPLE_CALL(avctx, end_frame);
diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index f13b1081fc..553f300c3d 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -779,7 +779,8 @@ end:
if (sl->ref_count[1])
ff_h264_set_erpic(>er.next_pic, sl->ref_list[1][0].parent);
-ff_er_frame_end(>er);
+if (ff_er_frame_end(>er) > 0)
+h->cur_pic_ptr->f->decode_error_flags |=
FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
if (use_last_pic)
memset(>ref_list[0][0], 0, sizeof(sl->ref_list[0][0]));
}
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 1accd07e9e..b2cc78c3d3 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -2033,7 +2033,8 @@ static int slice_end(AVCodecContext *avctx, AVFrame *pict)
if (/* s->mb_y << field_pic == s->mb_height && */ !s->first_field &&
!s1->first_slice) {
/* end of image */
-ff_er_frame_end(>er);
+if (ff_er_frame_end(>er) > 0)
+s->current_picture_ptr->f->decode_error_flags |=
FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
ff_mpv_frame_end(s);
diff --git a/libavcodec/mss2.c b/libavcodec/mss2.c
index 70aa56cb84..29cb4be614 100644
--- a/libavcodec/mss2.c
+++ b/libavcodec/mss2.c
@@ -421,8 +421,12 @@ static int decode_wmv9(AVCodecContext *avctx, const
uint8_t *buf, int buf_size,
ff_vc1_decode_blocks(v);
+f = s->current_picture.f;
+
if (v->end_mb_x == s->mb_width && s->end_mb_y == s->mb_height) {
-ff_er_frame_end(>er);
+if
