From patchwork Sun Feb 25 16:17:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Mark Thompson X-Patchwork-Id: 46520 Delivered-To: ffmpegpatchwork2@gmail.com Received: by 2002:a05:6a20:c51b:b0:19e:cdac:8cce with SMTP id gm27csp943204pzb; Sun, 25 Feb 2024 08:17:35 -0800 (PST) X-Forwarded-Encrypted: i=2; AJvYcCXP0XrvldBP0AhF1Usn08I6D683PJuMY+s1NiTU9RWCKORiqBauDbqiMgTgQ/+ejtlQkx8SNRBJuD6Ht2PDK4bxRpv9XWcB8JUtIw== X-Google-Smtp-Source: AGHT+IGkjrQYfuxH8yYz/GsFS3m/pitb+mZVF5Hh9zIyaqO52sR4aIn9QwBqmmcRPvD+EEtxO/X5 X-Received: by 2002:a17:907:770e:b0:a3e:ac92:b952 with SMTP id kw14-20020a170907770e00b00a3eac92b952mr3128767ejc.5.1708877854907; Sun, 25 Feb 2024 08:17:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1708877854; cv=none; d=google.com; s=arc-20160816; b=mi623xzoSymP9Dwxyv+S53jG102vPf5LFPnKFDLCkoMSMMi8epsjVii18pxPm48+Uz YXWcWfXwq2UaxAB/yJM6/td6h5rqqAlbYDdE57C0e4LPUw0NlBOgPzVv1FrKBobdrkrX qv1zKXbHIuD7P/c6o4gpm/GW23gnXooorTjkaXsGg7sfFoD3ERICBeQZCYyE1YFv60oD eZ8M3GhqiIyols+sC0xShc55r0bAPcVgZWb75dQc8rlUVYStoju7Q9/DNJ1Jwt9VkI7M 8WLlB0uY6K1rB795t8hyvOer69+G+Ygk675kIbIVe3evvwiMr3t71LPhMa2kLzNXscUr pEQA== 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:in-reply-to:from:references:to:content-language :user-agent:mime-version:date:message-id:dkim-signature:delivered-to; bh=KnRqry8PD11ZLwhNOlSDXSy3Zw0fFmCZg+8U20ojS3Q=; fh=YOA8vD9MJZuwZ71F/05pj6KdCjf6jQRmzLS+CATXUQk=; b=DuH9PVvPbPXNsh7HDFda7ujpXWh8Xt48akSRWm+NvVSCB9zO39XD7v3kAjYj7J0wtV HUPxiOQURIwASw76sdQCeynge5qetR9anAGiTH2Y9T39n+jVmV6lZ82YDKjp8o6Gck72 AqoqoSxU+83pijgGGm50+RA2yIsBgk1u307c+eZrloujFhiDuVZFaMI/zTrVMIdm+bPW 1CW6xaaAUm3nrfZ/C8KCXPqs8res8GtAI48nvGFD9IEp9B3DfdAE3TvExrjx1sJ/MzWx YYNEesqEHH5jnhLhtvfDM2gPkYThAVJGKqriMHL6rrtVnkxIp6Yo4Pv0XJqO2MwX1m4K skug==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@jkqxz-net.20230601.gappssmtp.com header.s=20230601 header.b=0NP4PTsF; 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 hk21-20020a170906c9d500b00a3e90feef5esi1401994ejb.559.2024.02.25.08.17.34; Sun, 25 Feb 2024 08:17:34 -0800 (PST) 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=@jkqxz-net.20230601.gappssmtp.com header.s=20230601 header.b=0NP4PTsF; 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 29E4A68C678; Sun, 25 Feb 2024 18:17:31 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 03F8E68C086 for ; Sun, 25 Feb 2024 18:17:23 +0200 (EET) Received: by mail-lj1-f176.google.com with SMTP id 38308e7fff4ca-2d21a68dd3bso31939791fa.1 for ; Sun, 25 Feb 2024 08:17:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20230601.gappssmtp.com; s=20230601; t=1708877843; x=1709482643; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=KNMiRDYJH7kwHkCgx38t4fv85t1YrVD7MspwFRafdEc=; b=0NP4PTsFWH/OEx4UXXd8gAV4vpHy7liC3MwzkcgYk1XH6kPC9DbSRPDPRhEuHQq+hS +HHz9xRbqRV61iRMlf6Xnxm7nY3mpEu8dvH48DURZhAUSpAo/5JBCIG9d/mjgpp2nnuH 22zkirqYCcjR5wEmEmRlN2Lx23ziZI53LxqfMYzevK7rPr0gKlGUdCgPEvrGXXhibYUX 2+ycdFhagAUCuX3M92cDrOl/XHKouLDR6SjlhVJAzF6ZlY+mOTxH1vHkuVVpsXzkUAez 432OqI7b2vWvuUA8N4xT7yhMvzD+4aB1uJyZ5upQVi7/icViG5zMdF7toyLGjGQ3HYk7 KY2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708877843; x=1709482643; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=KNMiRDYJH7kwHkCgx38t4fv85t1YrVD7MspwFRafdEc=; b=tZJDssOw5ehmoBfu9CP4iZNB39H7xCJiw4otQcZ6uDS3HVrPl8NIj2tpoJy48vKgK1 Fif5j+Mv8jcqo4rgVzVEYpNUaL4H7q2fsX9KPKsUD8OaFWg9dGbsLnvjPDaaKA3wJaQG a5kZJbmg5zDcogzTrUtLzgj8k7i3rjK9nnI+CBgkcuagYXSbyjZGq3BjMQJlvaTT0hIW u240iCi5ARW/O/6rEpA8zl/dvO7+6EAHh0mShtIWjCidBavVokYyzaUe0937Znsq/kfy Hr4n0+u8zL1TAZm+/Zi1DJgHJH7HiNckCY88ubgm390bfzn4ii0lmxRyxYIlw3VH2+M7 Q+OA== X-Gm-Message-State: AOJu0YxnT4Rl6A3jKJWxKWrl6HuprBbxD+ipG6ldG4oPvrgRGK6V3hpP J5/nK3h0pWgVLTOSZm6HWW0xVm7t3V+r4uidsiduNEKuArpSHyT55ioi9604vCyODmImYxe24cs 9 X-Received: by 2002:a2e:95c2:0:b0:2d2:5430:6063 with SMTP id y2-20020a2e95c2000000b002d254306063mr2899986ljh.29.1708877842108; Sun, 25 Feb 2024 08:17:22 -0800 (PST) Received: from [192.168.0.15] (cpc92302-cmbg19-2-0-cust1183.5-4.cable.virginm.net. [82.1.212.160]) by smtp.gmail.com with ESMTPSA id v6-20020a5d59c6000000b0033b47ee01f1sm5586434wry.49.2024.02.25.08.17.21 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 25 Feb 2024 08:17:21 -0800 (PST) Message-ID: <3082c4ec-e56b-4685-ac6b-712560b4ae8f@jkqxz.net> Date: Sun, 25 Feb 2024 16:17:50 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <3deacbbf-0087-477c-83b9-910bdba15b7d@jkqxz.net> <7c629c14-3bde-4433-8b70-c86180b6c976@jkqxz.net> <00642297-d9df-1c74-8a69-fdc8ca4633eb@passwd.hu> From: Mark Thompson In-Reply-To: <00642297-d9df-1c74-8a69-fdc8ca4633eb@passwd.hu> Subject: [FFmpeg-devel] [PATCH v2] ffmpeg: set extra_hw_frames to account for frames held in queues 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: 3W2ZPkG3OloT Since e0da916b8f5b079a4865eef7f64863f50785463d the ffmpeg utility has held multiple frames output by the decoder in internal queues without telling the decoder that it is going to do so. When the decoder has a fixed-size pool of frames (common in some hardware APIs where the output frames must be stored as an array texture) this could lead to the pool being exhausted and the decoder getting stuck. Fix this by telling the decoder to allocate additional frames according to the queue size. --- fftools/ffmpeg_dec.c | 13 +++++++++++++ fftools/ffmpeg_sched.c | 16 +++++++++++++++- fftools/ffmpeg_sched.h | 12 ++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) On 25/02/2024 16:02, Marton Balint wrote: > On Sun, 25 Feb 2024, Mark Thompson wrote: >> On 25/02/2024 15:01, Marton Balint wrote: >>>  On Sun, 25 Feb 2024, Mark Thompson wrote: >>> >>>>  Since e0da916b8f5b079a4865eef7f64863f50785463d the ffmpeg utility has >>>>  held multiple frames output by the decoder in internal queues without >>>>  telling the decoder that it is going to do so.  When the decoder has a >>>>  fixed-size pool of frames (common in some hardware APIs where the output >>>>  frames must be stored as an array texture) this could lead to the pool >>>>  being exhausted and the decoder getting stuck.  Fix this by telling the >>>>  decoder to allocate additional frames according to the queue size. >>> >>>  [...] >>> >>>>  diff --git a/fftools/ffmpeg_sched.h b/fftools/ffmpeg_sched.h >>>>  index 95f9c1d4db..315053ae42 100644 >>>>  --- a/fftools/ffmpeg_sched.h >>>>  +++ b/fftools/ffmpeg_sched.h >>>>  @@ -233,6 +233,13 @@ int sch_add_filtergraph(Scheduler *sch, unsigned >>>>  nb_inputs, unsigned nb_outputs, >>>>   */ >>>>   int sch_add_mux(Scheduler *sch, SchThreadFunc func, int (*init)(void *), >>>>                  void *ctx, int sdp_auto, unsigned thread_queue_size); >>>>  + >>>>  +/** >>>>  + * Default size of a thread queue, used if thread_queue_size is not set >>>>  on a >>>>  + * call to sch_add_mux(). >>> >>>  Not precisely, as this thread queue size is used for both frame queues and >>>  packet queues. >> >> Yes, it applies to both - hence the description I added not mentioning frames or packets. > > For some reason I assumed the description implies it is only used in sched_add_mux(). Ah, I didn't think of reading it like that. Hopefully clearer in this version. >>>  Historically the thread_queue_size option was introduced for packet queues >>>  for demuxed packets, and recently on the output for muxing packets. >>> >>>  If we want to make the frame queue size adjustable as well, I think it >>>  should be a separate option and maybe a separate constant should be added >>>  for its default value. >> >> This part is not changing anything about the queue sizes, it is just moving the existing magic number hidden in queue_alloc() to a named constant. >> >> I don't have any motivation to make the frame queue size adjustable; I added the assert so that if someone wants to do that in future they know that they need to take additional action to avoid breaking some decoders again. >> >>>>  + */ >>>>  +#define DEFAULT_THREAD_QUEUE_SIZE 8 >> >> Would you prefer that I make distinct DEFAULT_FRAME_THREAD_QUEUE_SIZE and DEFAULT_PACKET_THREAD_QUEUE_SIZE (both 8?) and replace the magic number in queue_alloc() with a selection between them based on the type?  I have no strong opinion on that, so I don't mind doing it if you would prefer it. > > I think its worth doing. Sure, here you go. Thanks, - Mark diff --git a/fftools/ffmpeg_dec.c b/fftools/ffmpeg_dec.c index 8c92b27cc1..0a6c3f46f1 100644 --- a/fftools/ffmpeg_dec.c +++ b/fftools/ffmpeg_dec.c @@ -1146,6 +1146,19 @@ int dec_open(Decoder **pdec, Scheduler *sch, goto fail; } + if (dp->dec_ctx->hw_device_ctx) { + // Update decoder extra_hw_frames option to account for the + // frames held in queues inside the ffmpeg utility. This is + // called after avcodec_open2() because the user-set value of + // extra_hw_frames becomes valid in there, and we need to add + // this on top of it. + int extra_frames = DEFAULT_FRAME_THREAD_QUEUE_SIZE; + if (dp->dec_ctx->extra_hw_frames >= 0) + dp->dec_ctx->extra_hw_frames += extra_frames; + else + dp->dec_ctx->extra_hw_frames = extra_frames; + } + ret = check_avoptions(*dec_opts); if (ret < 0) goto fail; diff --git a/fftools/ffmpeg_sched.c b/fftools/ffmpeg_sched.c index 1144fce958..ce10de682c 100644 --- a/fftools/ffmpeg_sched.c +++ b/fftools/ffmpeg_sched.c @@ -361,7 +361,21 @@ static int queue_alloc(ThreadQueue **ptq, unsigned nb_streams, unsigned queue_si ThreadQueue *tq; ObjPool *op; - queue_size = queue_size > 0 ? queue_size : 8; + if (queue_size <= 0) { + if (type == QUEUE_FRAMES) + queue_size = DEFAULT_FRAME_THREAD_QUEUE_SIZE; + else + queue_size = DEFAULT_PACKET_THREAD_QUEUE_SIZE; + } + + if (type == QUEUE_FRAMES) { + // This queue length is used in the decoder code to ensure that + // there are enough entries in fixed-size frame pools to account + // for frames held in queues inside the ffmpeg utility. If this + // can ever dynamically change then the corresponding decode + // code needs to be updated as well. + av_assert0(queue_size == DEFAULT_FRAME_THREAD_QUEUE_SIZE); + } op = (type == QUEUE_PACKETS) ? objpool_alloc_packets() : objpool_alloc_frames(); diff --git a/fftools/ffmpeg_sched.h b/fftools/ffmpeg_sched.h index 95f9c1d4db..eb3ef033e7 100644 --- a/fftools/ffmpeg_sched.h +++ b/fftools/ffmpeg_sched.h @@ -233,6 +233,18 @@ int sch_add_filtergraph(Scheduler *sch, unsigned nb_inputs, unsigned nb_outputs, */ int sch_add_mux(Scheduler *sch, SchThreadFunc func, int (*init)(void *), void *ctx, int sdp_auto, unsigned thread_queue_size); + +/** + * Default size of a packet thread queue. For muxing this can be overridden by + * the thread_queue_size option as passed to a call to sch_add_mux(). + */ +#define DEFAULT_PACKET_THREAD_QUEUE_SIZE 8 + +/** + * Default size of a frame thread queue. + */ +#define DEFAULT_FRAME_THREAD_QUEUE_SIZE 8 + /** * Add a muxed stream for a previously added muxer. *