From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id BB446435D6 for ; Fri, 17 Jun 2022 23:41:37 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 4D45068B449; Sat, 18 Jun 2022 02:41:34 +0300 (EEST) Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 3CCB36801E0 for ; Sat, 18 Jun 2022 02:41:28 +0300 (EEST) Received: by mail-wr1-f52.google.com with SMTP id s1so7440940wra.9 for ; Fri, 17 Jun 2022 16:41:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to:user-agent; bh=rLP4pHh3u4yMbBG18YAgGKO99zRNlOG1BQ5K8xadw98=; b=OWTlEM7O0EcV791AT8dR3qb43oOyboDDh/IRanU88wYRENsnZqAWXcRskLYioT1Npg ZynrmlV/Q816Ok1zrHOOe5lyCcjZIs1e5DAgvW9G04vLpA0rMLGNtkSrtY8S2Rsydz93 PAzWQ9mc1P0WoM3H1EMeV6LJ+wsn0ArZoEPAnpTuTW5k+X9dmL+o8JQcYmouGBPx1d7J QV8lCYCzvco1xH7JiC5TTgqZVtq+neLnAdpTLzB713aGYzPyEMq+aB0j130VnpbI87dk QyWEXTY+2/LL6H49oQMwQb9s/kaoLpU1AH6UyqeTMgaLOh58/Thx2bNWlJ1alYQnKyi9 qM6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:subject:message-id:mail-followup-to :references:mime-version:content-disposition:in-reply-to:user-agent; bh=rLP4pHh3u4yMbBG18YAgGKO99zRNlOG1BQ5K8xadw98=; b=8PDOGTA1oJ9a8Lbsh7WVPvLQGzD29F27N4qniD88w1CTs/F28+Rb9NtPE7R3o9LGJ4 b9RzYEjCoA9TpXFdRYWAN7VIuM5WUDz8SpGI6iiktMoAH5cTzSUansvMfR82UIPE/D2U /2iH0pJYnlBm7ZAoFA0utM9NcE6UkhD8KEfIs9jdENZWiqda2WCChY++Bs/Jj06bze6o QGKR7m6ENu4Xkzad16/NlpLJ5F8CfqkPLJ2AFoUFxQXUQvSAQaXhqge16Kz+uT0PoCdq pJmAYo70JDFkRS12IU8tzte0AHKdhm3e10vSlddOvb9i9qnevlK+d8We9G7pDCJxNfVR +sbQ== X-Gm-Message-State: AJIora8NftkJrWvGCKFZKRQvghFYjNnpaH7nnxSk7iCdiAL4ribvibig 1hE7TfUjHraRV+u425FNyY/Gg9Fs3sWPmsghUi4= X-Google-Smtp-Source: AGRyM1u95ogDoNYgOWvRQ1hxDamye6IZ8UduekB8mhSmB6dJk4ELilxsZeTmsBkyt+QW/yuda6j5/A== X-Received: by 2002:a05:6000:a0b:b0:219:f87e:86b with SMTP id co11-20020a0560000a0b00b00219f87e086bmr11535476wrb.570.1655509286989; Fri, 17 Jun 2022 16:41:26 -0700 (PDT) Received: from mariano (dynamic-adsl-78-14-101-54.clienti.tiscali.it. [78.14.101.54]) by smtp.gmail.com with ESMTPSA id x24-20020a7bc218000000b003942a244ee7sm6772992wmi.44.2022.06.17.16.41.25 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Jun 2022 16:41:26 -0700 (PDT) Received: by mariano (Postfix, from userid 1000) id B42D6BFA36; Sat, 18 Jun 2022 01:41:24 +0200 (CEST) Date: Sat, 18 Jun 2022 01:41:24 +0200 From: Stefano Sabatini To: FFmpeg development discussions and patches Message-ID: <20220617234124.GA177180@mariano> Mail-Followup-To: FFmpeg development discussions and patches References: <9642852e9da11714c8643833562b6e86133ce1a1.camel@amazon.it> <0215838ca87728a7e2742ab19af7fe1d5c3f9abc.camel@amazon.it> <2d163f70-25e5-27d6-fc6a-a5a8084a03d7@rothenpieler.org> <31d55286-6118-0438-62d8-c04fcb6d183d@rothenpieler.org> <197e819b43e1c8d203fc035b6fd2ca7f46502ef6.camel@amazon.it> <4cce0209-f45c-fa9f-8672-91bb6da7c8f0@rothenpieler.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4cce0209-f45c-fa9f-8672-91bb6da7c8f0@rothenpieler.org> User-Agent: Mutt/1.13.2 (2019-12-18) Subject: Re: [FFmpeg-devel] [PATCH] Added support for MB_INFO 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On date Friday 2022-06-17 17:10:25 +0200, Timo Rothenpieler wrote: > On 17.06.2022 12:59, Carotti, Elias wrote: [...] > > Again, you don't have to pass the ownership, and in fact in my use case > > I do not pass it since I actually recycle and update the same buffer > > for subsequent frames. If you do intentionally pass the ownership you > > need to be aware of what you are doing. As I said, I see a leak only in > > case of a programming error. > > Maybe we could explicitly state it in the comment section in mb_info.h: > > if you set the deallocator you're relinquishing ownership of the > > buffer. > > I'm not sure if that's something you can sensibly do with side data? > What if it gets buffered, copied, and so on? This semantics is specific to libx264, but can be possibly extended to other encoders. AFAIU libx264 keeps the data in memory until it needs it, and it finally calls the free when it is finished with the data. I think the concerns are about: 1. How does the consumer (libx264 or another encoder) knows that the data is still valid? This is a problem of the application injecting the data, it might use reference counting, the deallocator in this case will unref the data on the user application. 2. How does the application knows that the data is still in use by the consumer (the encoder)? It doesn't have to, since this is handled by the consumer, when it is finished it will call the deallocator. > > Plus, there's one more flag (b_mb_info_update) in libx264 which allows > > to pass back information to the caller about which macroblocks among > > the flagged ones were actually encoded as P_SKIP. I did not add support > > for that though but in the future somebody may want to. > > Yes, it's very x264 specific. > But the side data is generic. If for some reason x264 does not process a > frame, or any other encoder ends up getting used, the data will leak if it > relied on x264 to free it. So, let's make up a use case for this. The application injects the data, it might be for example a filter computing the MB information and it might be unaware of what encoder is used down in the processing pipeline. For the simplest case the application knows that libx264 is the used encoder, and assumes that all data will be consumed by libx264. For the x264 API the problem does not exist, with generic side data the problem can raise since side data must be agnostic of its final destination. So I agree, for the generic use case we can have a leak. I see some possible ways to solve this: 1. We document the limitation and assume that when the MB metadata is injected down in the pipeline there is a single consumer, the application will leak otherwise. This works for the simple application -> libx264 pipeline, but it can't work for a more generic case. 2. We provide a mechanism *at the library level* to cleanup the unconsumed data. libavcodec sees that the encoder does not handle that kind of user data (it might be a capability flag) and automatically cleanups the data in that case. This would be still underkill though, since the AVFrame is not necessarily injested into an encoder (for example the MB info could be rendered or processed by an application or by a filter). This means that the AVFrame unref needs to call this side data deallocator (and that the consumer needs to "steal" the data from the AVFrame before it is deallocated by the AVFrame unref). But then what happens in case the AVFrame is shared by multiple endpoints (e.g. multiple encoders)? Then you need a ref count for the side data itself. I don't know if this is something which is feasible with the current side data API, and if it could be extended to support this kind of semantics. 3. The simpler approach, we copy the data into MB info and deallocate when we free it, but this might have a performance penalty because of the unneeded memcpy. > > In principle I could've put the buffer into the side information and > > not just pass a pointer to it but I thought that it would require > > adding more semantics which would imply a stronger dependency on > > libx264. > > Right now, mb_info is a vector of uint8_t flags and the only possible > > value - to date - is X264_MBINFO_CONSTANT. What if, say, one day > > libx264 decides the buffer has to be a vector of uint16_t or still > > uint8_t but the flags are packed? On the other hand, this, AFAIK, is > > only supported by libx264. Other codecs may want to choose a different > > semantic for a similar field and the only possibility to make it > > generic is letting the caller handling the low level details. > > I'm not aware of any other side data with a similar semantic. And I'm really > not sure if it's sane or even valid to do it like that. > Can't the data be entirely contained within the side data? We can assume the same semantics of libx264 (at the moment the only use case for this). _______________________________________________ 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".