* [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside
@ 2023-09-12 11:40 Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE Thomas Guillem via ffmpeg-devel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Thomas Guillem via ffmpeg-devel @ 2023-09-12 11:40 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Thomas Guillem
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(&s->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(&s->cur_pic, 0, sizeof(ERPicture));
memset(&s->last_pic, 0, sizeof(ERPicture));
memset(&s->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(&s->er);
+ if (!s->studio_profile) {
+ if (ff_er_frame_end(&s->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(&h->er.next_pic, sl->ref_list[1][0].parent);
- ff_er_frame_end(&h->er);
+ if (ff_er_frame_end(&h->er) > 0)
+ h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
if (use_last_pic)
memset(&sl->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(&s->er);
+ if (ff_er_frame_end(&s->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(&s->er);
+ if (ff_er_frame_end(&s->er) > 0)
+ f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+
} else {
av_log(v->s.avctx, AV_LOG_WARNING,
"disabling error correction due to block count mismatch %dx%d != %dx%d\n",
@@ -431,8 +435,6 @@ static int decode_wmv9(AVCodecContext *avctx, const uint8_t *buf, int buf_size,
ff_mpv_frame_end(s);
- f = s->current_picture.f;
-
if (v->respic == 3) {
ctx->dsp.upsample_plane(f->data[0], f->linesize[0], w, h);
ctx->dsp.upsample_plane(f->data[1], f->linesize[1], w+1 >> 1, h+1 >> 1);
diff --git a/libavcodec/rv10.c b/libavcodec/rv10.c
index 5edd934f82..6d7ae8f903 100644
--- a/libavcodec/rv10.c
+++ b/libavcodec/rv10.c
@@ -440,6 +440,12 @@ static av_cold int rv10_decode_end(AVCodecContext *avctx)
return 0;
}
+static void rv10_er_frame_end(MpegEncContext *s)
+{
+ if (ff_er_frame_end(&s->er) > 0)
+ s->current_picture_ptr->f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+}
+
static int rv10_decode_packet(AVCodecContext *avctx, const uint8_t *buf,
int buf_size, int buf_size2, int whole_size)
{
@@ -477,7 +483,7 @@ static int rv10_decode_packet(AVCodecContext *avctx, const uint8_t *buf,
if ((s->mb_x == 0 && s->mb_y == 0) || !s->current_picture_ptr) {
// FIXME write parser so we always have complete frames?
if (s->current_picture_ptr) {
- ff_er_frame_end(&s->er);
+ rv10_er_frame_end(s);
ff_mpv_frame_end(s);
s->mb_x = s->mb_y = s->resync_mb_x = s->resync_mb_y = 0;
}
@@ -649,7 +655,7 @@ static int rv10_decode_frame(AVCodecContext *avctx, AVFrame *pict,
}
if (s->current_picture_ptr && s->mb_y >= s->mb_height) {
- ff_er_frame_end(&s->er);
+ rv10_er_frame_end(s);
ff_mpv_frame_end(s);
if (s->pict_type == AV_PICTURE_TYPE_B || s->low_delay) {
diff --git a/libavcodec/rv34.c b/libavcodec/rv34.c
index af4d6a3400..cae6e9d81b 100644
--- a/libavcodec/rv34.c
+++ b/libavcodec/rv34.c
@@ -1554,13 +1554,19 @@ static int get_slice_offset(AVCodecContext *avctx, const uint8_t *buf, int n, in
return buf_size;
}
+static void rv34_er_frame_end(MpegEncContext *s)
+{
+ if (ff_er_frame_end(&s->er) > 0)
+ s->current_picture_ptr->f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+}
+
static int finish_frame(AVCodecContext *avctx, AVFrame *pict)
{
RV34DecContext *r = avctx->priv_data;
MpegEncContext *s = &r->s;
int got_picture = 0, ret;
- ff_er_frame_end(&s->er);
+ rv34_er_frame_end(s);
ff_mpv_frame_end(s);
s->mb_num_left = 0;
@@ -1655,7 +1661,7 @@ int ff_rv34_decode_frame(AVCodecContext *avctx, AVFrame *pict,
av_log(avctx, AV_LOG_ERROR, "New frame but still %d MB left.\n",
s->mb_num_left);
if (!s->context_reinit)
- ff_er_frame_end(&s->er);
+ rv34_er_frame_end(s);
ff_mpv_frame_end(s);
}
@@ -1790,7 +1796,7 @@ int ff_rv34_decode_frame(AVCodecContext *avctx, AVFrame *pict,
av_log(avctx, AV_LOG_INFO, "marking unfished frame as finished\n");
/* always mark the current frame as finished, frame-mt supports
* only complete frames */
- ff_er_frame_end(&s->er);
+ rv34_er_frame_end(s);
ff_mpv_frame_end(s);
s->mb_num_left = 0;
ff_thread_report_progress(&s->current_picture_ptr->tf, INT_MAX, 0);
diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
index b8663aaf98..7113abd0a9 100644
--- a/libavcodec/vc1dec.c
+++ b/libavcodec/vc1dec.c
@@ -1347,8 +1347,10 @@ static int vc1_decode_frame(AVCodecContext *avctx, AVFrame *pict,
}
if ( !v->field_mode
&& avctx->codec_id != AV_CODEC_ID_WMV3IMAGE
- && avctx->codec_id != AV_CODEC_ID_VC1IMAGE)
- ff_er_frame_end(&s->er);
+ && avctx->codec_id != AV_CODEC_ID_VC1IMAGE) {
+ if (ff_er_frame_end(&s->er) > 0)
+ s->current_picture.f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
+ }
}
ff_mpv_frame_end(s);
--
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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* [FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE
2023-09-12 11:40 [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Thomas Guillem via ffmpeg-devel
@ 2023-09-12 11:40 ` Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES Thomas Guillem via ffmpeg-devel
2023-09-12 19:05 ` [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Michael Niedermayer
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Guillem via ffmpeg-devel @ 2023-09-12 11:40 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Thomas Guillem
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 0x7b5000007f78 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 0x7b5000007f78 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(&h->er.next_pic, sl->ref_list[1][0].parent);
if (ff_er_frame_end(&h->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(&sl->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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES
2023-09-12 11:40 [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE Thomas Guillem via ffmpeg-devel
@ 2023-09-12 11:40 ` Thomas Guillem via ffmpeg-devel
2023-09-12 13:11 ` Andreas Rheinhardt
2023-09-12 19:05 ` [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Michael Niedermayer
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Guillem via ffmpeg-devel @ 2023-09-12 11:40 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Thomas Guillem
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 0x7b5000009378 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 0x7b5000009378 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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES Thomas Guillem via ffmpeg-devel
@ 2023-09-12 13:11 ` Andreas Rheinhardt
2023-09-13 6:46 ` Thomas Guillem via ffmpeg-devel
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Rheinhardt @ 2023-09-12 13:11 UTC (permalink / raw)
To: ffmpeg-devel
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 0x7b5000009378 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 0x7b5000009378 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.
- 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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside
2023-09-12 11:40 [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES Thomas Guillem via ffmpeg-devel
@ 2023-09-12 19:05 ` Michael Niedermayer
2 siblings, 0 replies; 6+ messages in thread
From: Michael Niedermayer @ 2023-09-12 19:05 UTC (permalink / raw)
To: FFmpeg development discussions and patches
[-- Attachment #1.1: Type: text/plain, Size: 2570 bytes --]
On Tue, Sep 12, 2023 at 01:40:13PM +0200, Thomas Guillem via ffmpeg-devel wrote:
> 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.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);
The return code needs to be documented
> 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(&s->er);
> + if (!s->studio_profile) {
> + if (ff_er_frame_end(&s->er) > 0)
> + s->current_picture.f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
> + }
[...]
> +static void rv10_er_frame_end(MpegEncContext *s)
> +{
> + if (ff_er_frame_end(&s->er) > 0)
> + s->current_picture_ptr->f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
> +}
[...]
> +static void rv34_er_frame_end(MpegEncContext *s)
> +{
> + if (ff_er_frame_end(&s->er) > 0)
> + s->current_picture_ptr->f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
> +}
[...]
> + if (ff_er_frame_end(&s->er) > 0)
> + s->current_picture.f->decode_error_flags |= FF_DECODE_ERROR_CONCEALMENT_ACTIVE;
> + }
> }
This looks like duplicated code
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The worst form of inequality is to try to make unequal things equal.
-- Aristotle
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 251 bytes --]
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES
2023-09-12 13:11 ` Andreas Rheinhardt
@ 2023-09-13 6:46 ` Thomas Guillem via ffmpeg-devel
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Guillem via ffmpeg-devel @ 2023-09-13 6:46 UTC (permalink / raw)
To: Andreas Håkon via ffmpeg-devel; +Cc: Thomas Guillem
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 0x7b5000009378 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 0x7b5000009378 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-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-13 6:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-12 11:40 [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 2/3] h264: fix data-race with FF_DECODE_ERROR_CONCEALMENT_ACTIVE Thomas Guillem via ffmpeg-devel
2023-09-12 11:40 ` [FFmpeg-devel] [PATCH 3/3] h264: fix data-race with FF_DECODE_ERROR_DECODE_SLICES Thomas Guillem via ffmpeg-devel
2023-09-12 13:11 ` Andreas Rheinhardt
2023-09-13 6:46 ` Thomas Guillem via ffmpeg-devel
2023-09-12 19:05 ` [FFmpeg-devel] [PATCH 1/3] error_resilience: set the decode_error_flags outside Michael Niedermayer
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git