From patchwork Thu Aug 1 14:33:21 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anton Khirnov X-Patchwork-Id: 50859 Delivered-To: ffmpegpatchwork2@gmail.com Received: by 2002:a59:12d6:0:b0:489:2eb3:e4c4 with SMTP id 205csp1194548vqs; Thu, 1 Aug 2024 07:35:07 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCV3vZEMhEgph9gwk64XyoVI8kXTXN/6x+CQ4QSvd673Swje31dex60KbldpOK3nKp23xN71V3gAQZU4xxBSYGHyWhgp1YqB0SuJGA== X-Google-Smtp-Source: AGHT+IG2cX+dIOb+Fz2mobqK/d073CtkjlNhxq/QTSST8hY+bX6MVfobDGrrLXHwD0ymj3XJkn68 X-Received: by 2002:aa7:df12:0:b0:5a1:c4e3:a7a8 with SMTP id 4fb4d7f45d1cf-5b7f51293fdmr389296a12.30.1722522907335; Thu, 01 Aug 2024 07:35:07 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1722522907; cv=none; d=google.com; s=arc-20160816; b=Hw2hzOGeONKEZI5r87tNsg2FjCpWXxxVDoJdmMFZSw81iWsBvvUmvZDaTj8N0+Yftl 8gJzzGWmqL5aHSuxi6diIIS31HOJW4+9Di8Jj2TimxhDmHN676QiLjoJtS1q9woyJLjz jR1LVcv80/24R593TKdE/BVUTZvRWE5MFJAWsRXCgDcyKmV8UCufMiE1PDxirUKkSWjB STkDQKBonoE1TA2egKyHaYEvebfNf/Rt0NYCl/HAkcd2ZNW+rngwLtbV+XodQE+KQhDI Ck3XGRNq14ZrLN8CE5tpcf7yPmZ92VIPkJjkcoI28kh6EKp3dRKA5hyrQuA0tHHfE4jj giIw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:content-transfer-encoding:reply-to:list-subscribe :list-help:list-post:list-archive:list-unsubscribe:list-id :precedence:subject:mime-version:references:in-reply-to:message-id :date:to:from:dkim-signature:delivered-to; bh=EGyEiZr/SFIaPLuibrD+881yYcI3toFLn3IHcEq0Ut0=; fh=YOA8vD9MJZuwZ71F/05pj6KdCjf6jQRmzLS+CATXUQk=; b=bYapBCV2B6amQZD2yExS5Y6kiFNFYPvlVmjnePOXtNn9L2D2+ClMok3xzXKtdGl7Y1 em+X9qlxIEqyYCvYS6O8He5IpjyqJQqFpSvpgZGCGPr+4IkJkLmkt06KVUwPszcJhEdI fqmi9iXvY5SXArbBmy4mBUxlMhfwPbcJkRq6g++V7VGqeDBe4r/R/8AUuo7kA30uznzM sxs9I2Ao6hRo7nPQgLj91NFdhY0bHT0SXvanbCk86CNaDgxXxm8pCrBfb9mbYl0JMzml SwYs1ssdLQ1JdDLKL44Nahhamq6VkOBhZPS/YrUwzXULxoeeAd1j8HcEIjzXYG6G/PzE AlCw==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@khirnov.net header.s=mail header.b=EmM70F4J; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id 4fb4d7f45d1cf-5ac64faf8fasi10340408a12.307.2024.08.01.07.35.05; Thu, 01 Aug 2024 07:35:07 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@khirnov.net header.s=mail header.b=EmM70F4J; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 76F9B68D2F5; Thu, 1 Aug 2024 17:35:02 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail1.khirnov.net (quelana.khirnov.net [94.230.150.81]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0C45F68D2F5 for ; Thu, 1 Aug 2024 17:34:55 +0300 (EEST) Authentication-Results: mail1.khirnov.net; dkim=pass (2048-bit key; unprotected) header.d=khirnov.net header.i=@khirnov.net header.a=rsa-sha256 header.s=mail header.b=EmM70F4J; dkim-atps=neutral Received: from localhost (mail1.khirnov.net [IPv6:::1]) by mail1.khirnov.net (Postfix) with ESMTP id 4F2714DE1 for ; Thu, 1 Aug 2024 16:34:54 +0200 (CEST) Received: from mail1.khirnov.net ([IPv6:::1]) by localhost (mail1.khirnov.net [IPv6:::1]) (amavis, port 10024) with ESMTP id 6EV18314hkF8 for ; Thu, 1 Aug 2024 16:34:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=khirnov.net; s=mail; t=1722522893; bh=hPqhC/xG+S2PDvwSiAwz2qh67tK8p+vmVIcuXd+2n3k=; h=From:To:Subject:Date:In-Reply-To:References:From; b=EmM70F4J16JX0k2zwu2YVSkRE1DwPCE+i9jcWTwDhBlvWMYwHEhJt62ji0ium1M8j w04BqeikBzxVdtADIxbtfCPY7i19AYKoXCH2/VsyWjrt06t5kaJlXIn8gTU3jTCN38 k/r2reI+xi02Xye/V2yVgRqGVzvKJv/scUSS/3m4rE6WJVLVW6MEntwLr/AYPM6u71 ntFZ7EBbx13qZ5FOWGANHKTzJavGW4piORZ2jIFVJNTCcEvhZtAoXjdmjaNbAuyPHD Z6MN6yKfGd3X4mcQtsCK6ivdSUgJrz1atUKnH++KyrzO3fR121erV/0RXzIsXwOam8 SAcwlaIfW/L/Q== Received: from libav.khirnov.net (libav.khirnov.net [IPv6:2a00:c500:561:201::7]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "libav.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail1.khirnov.net (Postfix) with ESMTPS id 3825FEFE for ; Thu, 1 Aug 2024 16:34:53 +0200 (CEST) Received: from libav.khirnov.net (libav.khirnov.net [IPv6:::1]) by libav.khirnov.net (Postfix) with ESMTP id F2A153A02B4 for ; Thu, 01 Aug 2024 16:34:46 +0200 (CEST) From: Anton Khirnov To: ffmpeg-devel@ffmpeg.org Date: Thu, 1 Aug 2024 16:33:21 +0200 Message-ID: <20240801143430.4355-1-anton@khirnov.net> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240731125955.GV4991@pb2> References: <20240731125955.GV4991@pb2> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH] lavc/ffv1dec: fix races in accessing FFV1SliceContext.slice_damaged X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" X-TUID: 2y+c9FAr71pm That variable is shared between frame threads in the same defective way described in the previous commit. Fix it by adding a RefStruct-managed arrays of flags that is propagated across frame threads in the standard manner. Remove now-unused FFV1Context.fsrc --- This version keeps the ER code where it is, after all slices are decoded. It is a bit more complex though. libavcodec/ffv1.c | 2 ++ libavcodec/ffv1.h | 14 ++++++++++++- libavcodec/ffv1dec.c | 47 ++++++++++++++++++++++++++++---------------- 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c index 9c219b5ddb..333fb3d79b 100644 --- a/libavcodec/ffv1.c +++ b/libavcodec/ffv1.c @@ -214,6 +214,8 @@ av_cold int ff_ffv1_close(AVCodecContext *avctx) ff_refstruct_unref(&sc->plane); } + ff_refstruct_unref(&s->slice_damaged); + av_freep(&avctx->stats_out); for (j = 0; j < s->quant_table_count; j++) { av_freep(&s->initial_states[j]); diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h index edc3f6aef0..9aa0452922 100644 --- a/libavcodec/ffv1.h +++ b/libavcodec/ffv1.h @@ -118,7 +118,6 @@ typedef struct FFV1Context { int64_t picture_number; int key_frame; ProgressFrame picture, last_picture; - struct FFV1Context *fsrc; const AVFrame *cur_enc_frame; int plane_count; @@ -148,6 +147,19 @@ typedef struct FFV1Context { int num_h_slices; FFV1SliceContext *slices; + /* RefStruct object, per-slice damage flags shared between frame threads. + * + * After a frame thread marks some slice as finished with + * ff_progress_frame_report(), the corresponding array element must not be + * accessed by this thread anymore, as from then on it is owned by the next + * thread. + */ + uint8_t *slice_damaged; + /* Frame damage flag, used to delay announcing progress, since ER is + * applied after all the slices are decoded. + * NOT shared between frame threads. + */ + uint8_t frame_damaged; } FFV1Context; int ff_ffv1_common_init(AVCodecContext *avctx); diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c index 0b0ae956a6..415c66be63 100644 --- a/libavcodec/ffv1dec.c +++ b/libavcodec/ffv1dec.c @@ -254,6 +254,16 @@ static int decode_slice_header(const FFV1Context *f, return 0; } +static void slice_set_damaged(FFV1Context *f, FFV1SliceContext *sc) +{ + sc->slice_damaged = 1; + + // only set this for frame threading, as for slice threading its value is + // not used and setting it would be a race + if (f->avctx->active_thread_type & FF_THREAD_FRAME) + f->frame_damaged = 1; +} + static int decode_slice(AVCodecContext *c, void *arg) { FFV1Context *f = c->priv_data; @@ -264,15 +274,11 @@ static int decode_slice(AVCodecContext *c, void *arg) const int si = sc - f->slices; GetBitContext gb; - if (f->fsrc && !(p->flags & AV_FRAME_FLAG_KEY) && f->last_picture.f) + if (!(p->flags & AV_FRAME_FLAG_KEY) && f->last_picture.f) ff_progress_frame_await(&f->last_picture, si); - if (f->fsrc) { - const FFV1SliceContext *scsrc = &f->fsrc->slices[si]; - - if (!(p->flags & AV_FRAME_FLAG_KEY)) - sc->slice_damaged |= scsrc->slice_damaged; - } + if (f->slice_damaged[si]) + slice_set_damaged(f, sc); sc->slice_rct_by_coef = 1; sc->slice_rct_ry_coef = 1; @@ -282,7 +288,7 @@ static int decode_slice(AVCodecContext *c, void *arg) return AVERROR(ENOMEM); if (decode_slice_header(f, sc, p) < 0) { sc->slice_x = sc->slice_y = sc->slice_height = sc->slice_width = 0; - sc->slice_damaged = 1; + slice_set_damaged(f, sc); return AVERROR_INVALIDDATA; } } @@ -344,11 +350,12 @@ static int decode_slice(AVCodecContext *c, void *arg) v = sc->c.bytestream_end - sc->c.bytestream - 2 - 5*f->ec; if (v) { av_log(f->avctx, AV_LOG_ERROR, "bytestream end mismatching by %d\n", v); - sc->slice_damaged = 1; + slice_set_damaged(f, sc); } } - ff_progress_frame_report(&f->picture, si); + if ((c->active_thread_type & FF_THREAD_FRAME) && !f->frame_damaged) + ff_progress_frame_report(&f->picture, si); return 0; } @@ -768,11 +775,14 @@ static int read_header(FFV1Context *f) return AVERROR_INVALIDDATA; } + ff_refstruct_unref(&f->slice_damaged); + f->slice_damaged = ff_refstruct_allocz(f->slice_count * sizeof(*f->slice_damaged)); + if (!f->slice_damaged) + return AVERROR(ENOMEM); + for (int j = 0; j < f->slice_count; j++) { FFV1SliceContext *sc = &f->slices[j]; - sc->slice_damaged = 0; - if (f->version == 2) { int sx = get_symbol(c, state, 0); int sy = get_symbol(c, state, 0); @@ -857,6 +867,7 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe, f->avctx = avctx; + f->frame_damaged = 0; ff_init_range_decoder(c, buf, buf_size); ff_build_rac_states(c, 0.05 * (1LL << 32), 256 - 8); @@ -920,6 +931,8 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe, int trailer = 3 + 5*!!f->ec; int v; + sc->slice_damaged = 0; + if (i || f->version > 2) { if (trailer > buf_p - buf) v = INT_MAX; else v = AV_RB24(buf_p-trailer) + trailer; @@ -943,7 +956,7 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe, } else { av_log(f->avctx, AV_LOG_ERROR, "\n"); } - sc->slice_damaged = 1; + slice_set_damaged(f, sc); } if (avctx->debug & FF_DEBUG_PICT_INFO) { av_log(avctx, AV_LOG_DEBUG, "slice %d, CRC: 0x%08"PRIX32"\n", i, AV_RB32(buf_p + v - 4)); @@ -988,6 +1001,8 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe, avctx->pix_fmt, sc->slice_width, sc->slice_height); + + f->slice_damaged[i] = 1; } } ff_progress_frame_report(&f->picture, INT_MAX); @@ -1039,8 +1054,6 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) FFV1SliceContext *sc = &fdst->slices[i]; const FFV1SliceContext *sc0 = &fsrc->slices[i]; - sc->slice_damaged = sc0->slice_damaged; - ff_refstruct_replace(&sc->plane, sc0->plane); if (fsrc->version < 3) { @@ -1051,12 +1064,12 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) } } + ff_refstruct_replace(&fdst->slice_damaged, fsrc->slice_damaged); + av_assert1(fdst->max_slice_count == fsrc->max_slice_count); ff_progress_frame_replace(&fdst->picture, &fsrc->picture); - fdst->fsrc = fsrc; - return 0; } #endif