From patchwork Tue Sep 12 19:33:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 43718 Delivered-To: ffmpegpatchwork2@gmail.com Received: by 2002:a05:6a20:4e27:b0:149:dfde:5c0a with SMTP id gk39csp3016850pzb; Tue, 12 Sep 2023 12:32:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHcwF1v4HKzZnB516kZQWGabKZ0TyEzQEiBCvhUhe4jcavtmnNLwdT/4yMgyJpv/ZTwKUo+ X-Received: by 2002:a05:6402:882:b0:523:3e23:15ab with SMTP id e2-20020a056402088200b005233e2315abmr466635edy.26.1694547144267; Tue, 12 Sep 2023 12:32:24 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id b8-20020aa7cd08000000b00525667da231si9438409edw.82.2023.09.12.12.32.23; Tue, 12 Sep 2023 12:32:24 -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=@outlook.com header.s=selector1 header.b=amono4Ei; arc=fail (body hash mismatch); 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; dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=outlook.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8682E68C9CF; Tue, 12 Sep 2023 22:32:21 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from EUR04-HE1-obe.outbound.protection.outlook.com (mail-he1eur04olkn2104.outbound.protection.outlook.com [40.92.73.104]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 5D33468C6DE for ; Tue, 12 Sep 2023 22:32:14 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TaYuBAgpuYKakR2nBW1vKsQAE7GCd3ie+E5DpLSMfMR2n7uusjlELZXromr8StocXNn2ZxypjPLmxtK9D7WOAjpxfcwvciP342h9Jvfzejd3B8z+Ry3VxsGuxt8PExhDTS0TZ80YA4f7ach8VeSeMVFixhAuykz292w//HDMwvm0Jiam062+NgG/6ZuszV8LUOGOsrsr4ugKkRQlbu6oUe2C1zvkoTE/E2CXW8/dZNDhwO7CsE2dHb4rRx62LVyJwZRTzyN2Pxn8kfnDmmKLs9Hchh6zwz9Jwy0u4K9jZ0PFSKE4NyK3+hS9GZ7AoO7nURGNfM2d0C8bDKKLYqwRuw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=XvUANFow4rpajvpefebXWf5c7C/RXOnHQFWwPqHq8j8=; b=M7nJROSLcGxlofoOZkXCnSDIRZM8PZbUmCPEbiJG9BlI3DYsyrX3LETIP5OIqU7cr7dLn8la6vJyZ7Kf9L1eW95YjmtAwYodwVkdGV8sDoAd+J3bjy96QkqP5oP21mtGTR9qeb+inqXvnWGqZd2NqVasDTT2OkRfsJ19o0vIC3t7avmz3felzDxBOPzL+KnxKV7cl1Awb8DVF9pd3lwcvyoSqBLLVnbvYCdpWTgeGnQVmQZAeT0LKKVdPoci9KxZF0vkOj22n4sZP5FrZ5Z1TbvXh+I6XBxHY+RJctfkXM5DQedY7ncxutpMjcntvsVAHdFynC+K8uNjg/I36yfyhQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=XvUANFow4rpajvpefebXWf5c7C/RXOnHQFWwPqHq8j8=; b=amono4Ei2TItduz1LDIBC00qvAV4DI/h07sUTXaNB/+fyQlFzWekcUoK+A7UkAqpNBc5qJENE/zHg6qTs+5tbGwcloXCi7EA2vPAAtFyd/eQ9c0prBkksoZwMkGH8JZHmTHxRXedgwYR+t/xUpqhIjJE4XVkWq4VGbZu6lDYvtUFgMYN7/h9e4dVXNlhQXYRUKv9MXqmassKjkaBIQsLzB7vIyEUicOltm5Dk9OmKo0cKJSIgjWG2udS7HbrKJMf9MwQbX6mdEz43/Yp/CBa6I67b5/3DvTl7uXkQxGOTOFxXYo3rTsKBeslFN64WnE+aS6m+eZQfSJUVdvC5zM1iQ== Received: from GV1P250MB0737.EURP250.PROD.OUTLOOK.COM (2603:10a6:150:8e::17) by AS8P250MB0251.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:37e::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6768.33; Tue, 12 Sep 2023 19:32:12 +0000 Received: from GV1P250MB0737.EURP250.PROD.OUTLOOK.COM ([fe80::3fff:eb7b:b8e2:4dba]) by GV1P250MB0737.EURP250.PROD.OUTLOOK.COM ([fe80::3fff:eb7b:b8e2:4dba%4]) with mapi id 15.20.6768.036; Tue, 12 Sep 2023 19:32:12 +0000 From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Tue, 12 Sep 2023 21:33:23 +0200 Message-ID: X-Mailer: git-send-email 2.34.1 In-Reply-To: References: X-TMN: [97hvC5e3Uq6LvnMJOGZheyD5BTNN32Rv] X-ClientProxiedBy: ZR0P278CA0107.CHEP278.PROD.OUTLOOK.COM (2603:10a6:910:23::22) To GV1P250MB0737.EURP250.PROD.OUTLOOK.COM (2603:10a6:150:8e::17) X-Microsoft-Original-Message-ID: <20230912193323.2996282-1-andreas.rheinhardt@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: GV1P250MB0737:EE_|AS8P250MB0251:EE_ X-MS-Office365-Filtering-Correlation-Id: 6b047956-dba0-4484-7775-08dbb3c6f675 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: EEGuxZhHc2KL4jMsmbhxdNrNOZ2vjS2WuwYStQyuB6M1V6FEcvszjX9GUoMdlnChhGZEMxgnw1pWlhN47hPQRmmiGGS66jEuC1G0HsrxQsB5JyYfvEYhysxuuSqe7kKrgCLNt8COZVg5jrbNEKrbXCXiIGTXSlJviY9zgxsaVAhmzQ8jp97BuSCv/OtcR/CGTmt0i1f05J45m7GIGE4yAFqTxME/T8cdoDkhp7ap0h9jSkMscR5M14zF7u2oEtldnY51FQArJydDCpXKla/lEH61v+pzwRe+cBwVR3JUOQzsVUqWfvwH3hFqtxW17cjOaYi2NuNnQQiifu+FQEVgvE4HCYV+w9wiZGwUw8RMq4eA6pUjCnaZ7uHzxr5vre1WkoAJikZaDHBJ+qEBaHbGmO4CzWAxP5s8rVNOvuzVx/FUfNixi4sBzwOGzM+CUg1mxJvymOGOFCU0BzL05YZsflGtA809yQBOKr+KTmubMsQYzqIO2DEmaxjqHqn0G6e9WBsQRt5UYM73EkChlLaKDiuDFb3jet3gZxCRxtayK7G7u4I7nAXF2E1UkCgQ52Non27VW9FHNMd2DSzQomtmUroDdwQrd2likhXcdWM846S/upU2jXkiw8dZQQRuaiXz X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: jE9sVtTIxzhmA3guQ6zPN4tiZQFTlXQsyehtTSlRb/P5kCTVeuoMBYimL5tW9k1xVjqBjsZ+KC0s/u9afJ13UItBqa+oCXB0gYv2wEWTwX/t8PWCDoglPJhZYYmvZcXM1yc+nUhQBBecUgNwobdTaAz0jlUMS6DCIFu0k71YDub6weeJtbxSNqqdFqo1dQtxetheJ6IfNplxYlvHlpx/qGQpqiceq3ggqJfNqg9SQzkwFO9scZ5sBWTmGFGJGfHO2X/3v8iKBnlyOzF05/AGXR8WaUiCkBqQiMKxucM3FuVdPPRm8zNkZa83CTIoJd5OnOBKNrqQaNUuBkC0CCk4BwVsLfaxNI3rou/gaKovBVqMPML1V1e0VhGrOisFosc2M8fDLaL1MyxuwWD1fOmNmS3RDwa6eIxlWlQY3mU9cqJuwYY8R2OVDacCzqc4laXBal057WWy1DjA+v16FVN1p/nI+rYEb89zJwatVei4wuXMNPji3wmuQpLrKY/sEvWcfX4JXpyZBVcjK2LgLo/qbAdLdcoL+XP5FVstF/1iVoFO+O19aukZ+8cZwy5aGsh9i49pTd87OF3sDNAas9CYyT1I7V4o4IIoS9jYPflKu0FFYrdJQbpyUpRnsR3NSKWVlbizJ8JpkBxD9yD96sL5Rz+XNe2NLGq6xEAPxTIy8Er/eMPltJKZvyct6QK7gq+8XWrkKXJ5oZDA4+SHEdsOhWV5QbmMSysYoQK0Udaxzp0R3n/sTUuUk5kKSzgQ4i/+jVc9ols4YQ5I/M1/wuaMG9Td5nHbQaYZ8KcAgdW+5wE6ZIg99u3aXhlepZUME1By9QwT8djWzQ7V9oivpITJC0KFsXSo0ObBwMKGH9uKphLtZsZJA9bhMnu9/Pf3xEHM8vC4V653QLfy/mpGr+6UTcgNg4w4/5id14lWsJkz2xKBDSi2NEY6waANcbAyOuUV5yv9WgAIqo9DB9oj5CpYb94EvQYDwxNmsd2W2sB8xdz2hlfCavaub6O9xxHNtVXkyxpDqWfR3PlyKr7Wj24xhogLElq4+uAi9HonhKQN+Vsy/aoWCjnXYe2BSJYQtGhtoO1unOTkdknt4VuFWUu+sq3vD8d1kyDlkHALL2DJYXmIgE/f+kLIinNe9W1VdBE81WZk5CU+Dfw1AwiZS9oUCdUWfUT5w1kDS0gN+PhPIZXEvpQ9BVr/ur1fXFTkVaQeFO9fzpYInMfeUgPsDVH9dp3cW7XL2k/3IeYEmkTyw5RpHpxN4pgoi1jTsK4lhsZI X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 6b047956-dba0-4484-7775-08dbb3c6f675 X-MS-Exchange-CrossTenant-AuthSource: GV1P250MB0737.EURP250.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2023 19:32:12.5712 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8P250MB0251 Subject: [FFmpeg-devel] [PATCH 2/2] avcodec/h264dec: Fix data race when updating decode_error_flags 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 Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" X-TUID: FoEVeMRZrpWl When using multi-threaded decoding, every decoding thread has its own DBP consisting of H264Pictures and each of these points to its own AVFrames. They are synced during update_thread_context via av_frame_ref() and therefore the threads actually decoding (as well as all the others) must not modify any field that is copied by av_frame_ref() after ff_thread_finish_setup(). Yet this is exactly what happens when an error occurs during decoding and the AVFrame's decode_error_flags are updated. Given that these errors only become apparent during decoding, this can't be set before ff_thread_finish_setup() without defeating the point of frame-threading; in practice, this meant that the decoder did not set these flags correctly in case frame-threading was in use. (This means that e.g. the ffmpeg cli tool fails to output its "corrupt decoded frame" message in a nondeterministic fashion.) This commit fixes this by adding a new H264Picture field that is actually propagated across threads; the field is an AVBufferRef* whose data is an atomic_int; it is atomic in order to allow multiple threads to update it concurrently and not to provide synchronization between the threads setting the field and the thread ultimately returning the AVFrame. This unfortunately has the overhead of one allocation per H264Picture (both the original one as well as creating a reference to an existing one), even in case of no errors. In order to mitigate this, an AVBufferPool has been used and only if frame-threading is actually in use. This expense will be removed as soon as a proper API for refcounted objects (not based upon AVBuffer) is in place. Signed-off-by: Andreas Rheinhardt --- libavcodec/h264_picture.c | 9 +++++++++ libavcodec/h264_slice.c | 7 +++++++ libavcodec/h264dec.c | 38 ++++++++++++++++++++++++++++++++++++-- libavcodec/h264dec.h | 4 ++++ 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/libavcodec/h264_picture.c b/libavcodec/h264_picture.c index bd31f700cb..31b5e231c2 100644 --- a/libavcodec/h264_picture.c +++ b/libavcodec/h264_picture.c @@ -54,6 +54,7 @@ void ff_h264_unref_picture(H264Context *h, H264Picture *pic) av_buffer_unref(&pic->motion_val_buf[i]); av_buffer_unref(&pic->ref_index_buf[i]); } + av_buffer_unref(&pic->decode_error_flags); memset((uint8_t*)pic + off, 0, sizeof(*pic) - off); } @@ -136,6 +137,10 @@ int ff_h264_ref_picture(H264Context *h, H264Picture *dst, H264Picture *src) dst->hwaccel_picture_private = dst->hwaccel_priv_buf->data; } + ret = av_buffer_replace(&dst->decode_error_flags, src->decode_error_flags); + if (ret < 0) + goto fail; + h264_copy_picture_params(dst, src); return 0; @@ -186,6 +191,10 @@ int ff_h264_replace_picture(H264Context *h, H264Picture *dst, const H264Picture dst->hwaccel_picture_private = src->hwaccel_picture_private; + ret = av_buffer_replace(&dst->decode_error_flags, src->decode_error_flags); + if (ret < 0) + goto fail; + h264_copy_picture_params(dst, src); return 0; diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index 6cd7bb8fe7..71a878b80b 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -210,6 +210,13 @@ static int alloc_picture(H264Context *h, H264Picture *pic) if (ret < 0) goto fail; + if (h->decode_error_flags_pool) { + pic->decode_error_flags = av_buffer_pool_get(h->decode_error_flags_pool); + if (!pic->decode_error_flags) + goto fail; + atomic_init((atomic_int*)pic->decode_error_flags->data, 0); + } + if (CONFIG_GRAY && !h->avctx->hwaccel && h->flags & AV_CODEC_FLAG_GRAY && pic->f->data[2]) { int h_chroma_shift, v_chroma_shift; av_pix_fmt_get_chroma_sub_sample(pic->f->format, diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 8e90678125..796f80be8d 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -307,6 +307,12 @@ static int h264_init_context(AVCodecContext *avctx, H264Context *h) ff_h264_sei_uninit(&h->sei); + if (avctx->active_thread_type & FF_THREAD_FRAME) { + h->decode_error_flags_pool = av_buffer_pool_init(sizeof(atomic_int), NULL); + if (!h->decode_error_flags_pool) + return AVERROR(ENOMEM); + } + h->nb_slice_ctx = (avctx->active_thread_type & FF_THREAD_SLICE) ? avctx->thread_count : 1; h->slice_ctx = av_calloc(h->nb_slice_ctx, sizeof(*h->slice_ctx)); if (!h->slice_ctx) { @@ -353,6 +359,8 @@ static av_cold int h264_decode_end(AVCodecContext *avctx) h->cur_pic_ptr = NULL; + av_buffer_pool_uninit(&h->decode_error_flags_pool); + av_freep(&h->slice_ctx); h->nb_slice_ctx = 0; @@ -739,7 +747,16 @@ 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; + if (h->cur_pic_ptr->decode_error_flags) { + /* Frame-threading in use */ + atomic_int *decode_error = (atomic_int*)h->cur_pic_ptr->decode_error_flags->data; + /* Using atomics here is not supposed to provide syncronisation; + * they are merely used to allow to set decode_error from both + * decoding threads in case of coded slices. */ + atomic_fetch_or_explicit(decode_error, FF_DECODE_ERROR_DECODE_SLICES, + memory_order_relaxed); + } else + h->cur_pic_ptr->f->decode_error_flags |= FF_DECODE_ERROR_DECODE_SLICES; } ret = 0; @@ -762,6 +779,7 @@ end: H264SliceContext *sl = h->slice_ctx; int use_last_pic = h->last_pic_for_ec.f->buf[0] && !sl->ref_count[0]; + int decode_error_flags = 0; ff_h264_set_erpic(&h->er.cur_pic, h->cur_pic_ptr); @@ -779,7 +797,15 @@ 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, NULL); + ff_er_frame_end(&h->er, &decode_error_flags); + if (decode_error_flags) { + if (h->cur_pic_ptr->decode_error_flags) { + atomic_int *decode_error = (atomic_int*)h->cur_pic_ptr->decode_error_flags->data; + atomic_fetch_or_explicit(decode_error, decode_error_flags, + memory_order_relaxed); + } else + h->cur_pic_ptr->f->decode_error_flags |= decode_error_flags; + } if (use_last_pic) memset(&sl->ref_list[0][0], 0, sizeof(sl->ref_list[0][0])); } @@ -851,6 +877,14 @@ static int output_frame(H264Context *h, AVFrame *dst, H264Picture *srcp) if (srcp->needs_fg && (ret = av_frame_copy_props(dst, srcp->f)) < 0) return ret; + if (srcp->decode_error_flags) { + atomic_int *decode_error = (atomic_int*)srcp->decode_error_flags->data; + /* The following is not supposed to provide synchronisation at all: + * given that srcp has already finished decoding, decode_error + * has already been set to its final value. */ + dst->decode_error_flags |= atomic_load_explicit(decode_error, memory_order_relaxed); + } + av_dict_set(&dst->metadata, "stereo_mode", ff_h264_sei_stereo_mode(&h->sei.common.frame_packing), 0); if (srcp->sei_recovery_frame_cnt == 0) diff --git a/libavcodec/h264dec.h b/libavcodec/h264dec.h index beaab3902c..322c06a19c 100644 --- a/libavcodec/h264dec.h +++ b/libavcodec/h264dec.h @@ -152,6 +152,9 @@ typedef struct H264Picture { int mb_width, mb_height; int mb_stride; + + /* data points to an atomic_int */ + AVBufferRef *decode_error_flags; } H264Picture; typedef struct H264Ref { @@ -549,6 +552,7 @@ typedef struct H264Context { AVBufferPool *mb_type_pool; AVBufferPool *motion_val_pool; AVBufferPool *ref_index_pool; + AVBufferPool *decode_error_flags_pool; int ref2frm[MAX_SLICES][2][64]; ///< reference to frame number lists, used in the loop filter, the first 2 are for -2,-1 } H264Context;