This fixes data races (which are UB) in the MPEG-4 and H.263+ encoder when predicting DC values; these encoders unconditionally read values from the line above the current line and only check lateron (via first_slice_line) whether said prediction can be used at all. It will also allow to remove said checks (by setting the entries to 1024 upon opening a new slice). The vsynth{1,2,3,_lena}-mpeg4-thread FATE tests were affected by this: https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-tsan-slices&time=20250613002615
Showing the changes to libavcodec (2 of 2 files). Show all
Unified · Split
| @@ -179,6 +179,7 @@ static void backup_duplicate_context(MpegEncContext *bak, MpegEncContext *src) · 178 unmodified lines | |||
| 179 | COPY(block); | 179 | COPY(block); |
| 180 | COPY(start_mb_y); | 180 | COPY(start_mb_y); |
| 181 | COPY(end_mb_y); | 181 | COPY(end_mb_y); |
| 182 | COPY(dc_val); | ||
| 182 | COPY(ac_val); | 183 | COPY(ac_val); |
| 183 | #undef COPY | 184 | #undef COPY |
| 184 | } | 185 | } |
| @@ -235,7 +236,7 @@ av_cold int ff_mpv_init_context_frame(MpegEncContext *s) · 50 unmodified lines | |||
| 235 | s->avctx->active_thread_type & FF_THREAD_SLICE) ? | 236 | s->avctx->active_thread_type & FF_THREAD_SLICE) ? |
| 236 | s->avctx->thread_count : 1; | 237 | s->avctx->thread_count : 1; |
| 237 | BufferPoolContext *const pools = &s->buffer_pools; | 238 | BufferPoolContext *const pools = &s->buffer_pools; |
| 238 | int y_size, c_size, yc_size, i, mb_array_size, mv_table_size, x, y; | 239 | int y_size, c_size, yc_size, mb_array_size, mv_table_size, x, y; |
| 239 | int mb_height; | 240 | int mb_height; |
| 240 | 241 | ||
| 241 | if (s->encoding && s->avctx->slices) | 242 | if (s->encoding && s->avctx->slices) |
| @@ -337,6 +338,9 @@ av_cold int ff_mpv_init_context_frame(MpegEncContext *s) · 95 unmodified lines | |||
| 337 | } | 338 | } |
| 338 | 339 | ||
| 339 | if (s->h263_pred || s->h263_aic || !s->encoding) { | 340 | if (s->h263_pred || s->h263_aic || !s->encoding) { |
| 341 | // When encoding, each slice (and therefore each thread) | ||
| 342 | // gets its own ac_val and dc_val buffers in order to avoid | ||
| 343 | // races. | ||
| 340 | size_t allslice_yc_size = yc_size * (s->encoding ? nb_slices : 1); | 344 | size_t allslice_yc_size = yc_size * (s->encoding ? nb_slices : 1); |
| 341 | if (s->out_format == FMT_H263) { | 345 | if (s->out_format == FMT_H263) { |
| 342 | /* ac values */ | 346 | /* ac values */ |
| @@ -349,10 +353,15 @@ av_cold int ff_mpv_init_context_frame(MpegEncContext *s) · 6 unmodified lines | |||
| 349 | // MN: we need these for error resilience of intra-frames | 353 | // MN: we need these for error resilience of intra-frames |
| 350 | // Allocating them unconditionally for decoders also means | 354 | // Allocating them unconditionally for decoders also means |
| 351 | // that we don't need to reinitialize when e.g. h263_aic changes. | 355 | // that we don't need to reinitialize when e.g. h263_aic changes. |
| 352 | if (!FF_ALLOC_TYPED_ARRAY(s->dc_val_base, yc_size)) | 356 | |
| 357 | // y_size and therefore yc_size is always odd; allocate one element | ||
| 358 | // more for each encoder slice in order to be able to align each slice's | ||
| 359 | // dc_val to four in order to use aligned stores when cleaning dc_val. | ||
| 360 | allslice_yc_size += s->encoding * nb_slices; | ||
| 361 | if (!FF_ALLOC_TYPED_ARRAY(s->dc_val_base, allslice_yc_size)) | ||
| 353 | return AVERROR(ENOMEM); | 362 | return AVERROR(ENOMEM); |
| 354 | s->dc_val = s->dc_val_base + s->b8_stride + 1; | 363 | s->dc_val = s->dc_val_base + s->b8_stride + 1; |
| 355 | for (i = 0; i < yc_size; i++) | 364 | for (size_t i = 0; i < allslice_yc_size; ++i) |
| 356 | s->dc_val_base[i] = 1024; | 365 | s->dc_val_base[i] = 1024; |
| 357 | } | 366 | } |
| 358 | 367 | ||
| @@ -545,6 +545,7 @@ static av_cold int init_slice_buffers(MPVMainEncContext *const m) · 544 unmodified lines | |||
| 545 | } | 545 | } |
| 546 | 546 | ||
| 547 | if (s2->c.ac_val) { | 547 | if (s2->c.ac_val) { |
| 548 | s2->c.dc_val += offset + i; | ||
| 548 | s2->c.ac_val += offset; | 549 | s2->c.ac_val += offset; |
| 549 | offset += yc_size; | 550 | offset += yc_size; |
| 550 | } | 551 | } |