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 E49EF435AB for ; Fri, 17 Jun 2022 15:30:31 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 8402B68B92E; Fri, 17 Jun 2022 18:30:28 +0300 (EEST) Received: from smtp-fw-6001.amazon.com (smtp-fw-6001.amazon.com [52.95.48.154]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 74A6768B863 for ; Fri, 17 Jun 2022 18:30:21 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.it; i=@amazon.it; q=dns/txt; s=amazon201209; t=1655479826; x=1687015826; h=from:to:date:message-id:references:in-reply-to: content-id:mime-version:content-transfer-encoding:subject; bh=Lcb022naWz9BnO6qRwcnZ7a9gK3Opb4DVAUp/zjcNwA=; b=JMO1wUyPK53Np0aPVusb6GHR8e0ZXqzW4Uch1AOLtbvx+72YIOiSa9ph 5FtAbntZXZjy9Wz5qfbroC1DwH02ItYP14jtZyX3u6Psrk+eyP82SetbQ x1sSNOQQLu3w1ftxNHYENs1FClVZQFpcK+ktE4BxPiLuXQK/EiYv5d8tk 8=; Thread-Topic: [FFmpeg-devel] [PATCH] Added support for MB_INFO Received: from iad12-co-svc-p1-lb1-vlan3.amazon.com (HELO email-inbound-relay-iad-1d-1c3c2014.us-east-1.amazon.com) ([10.43.8.6]) by smtp-border-fw-6001.iad6.amazon.com with ESMTP; 17 Jun 2022 15:30:08 +0000 Received: from EX13D25EUA004.ant.amazon.com (iad12-ws-svc-p26-lb9-vlan2.iad.amazon.com [10.40.163.34]) by email-inbound-relay-iad-1d-1c3c2014.us-east-1.amazon.com (Postfix) with ESMTPS id 1FFCFC6B49 for ; Fri, 17 Jun 2022 15:30:07 +0000 (UTC) Received: from EX13D25EUA003.ant.amazon.com (10.43.165.117) by EX13D25EUA004.ant.amazon.com (10.43.165.12) with Microsoft SMTP Server (TLS) id 15.0.1497.36; Fri, 17 Jun 2022 15:30:05 +0000 Received: from EX13D25EUA003.ant.amazon.com ([10.43.165.117]) by EX13D25EUA003.ant.amazon.com ([10.43.165.117]) with mapi id 15.00.1497.036; Fri, 17 Jun 2022 15:30:07 +0000 From: "Carotti, Elias" To: "ffmpeg-devel@ffmpeg.org" Thread-Index: AQHYfLJt5ZoAjIylI0i9kT+cl/Xc0K1TUq0AgAAQnoCAAAmcAIAABV6AgAAG4wCAAEYxgIAABX6A Date: Fri, 17 Jun 2022 15:30:06 +0000 Message-ID: <036af1d4b012762389e38f327a206d8800992cfd.camel@amazon.it> 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> In-Reply-To: <4cce0209-f45c-fa9f-8672-91bb6da7c8f0@rothenpieler.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.43.161.210] Content-ID: MIME-Version: 1.0 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 Fri, 2022-06-17 at 17:10 +0200, Timo Rothenpieler wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On 17.06.2022 12:59, Carotti, Elias wrote: > > > Yes, exactly. It relies on x264 to free it. > > > > Not really. It can rely on x264 if you explicitly want that > > behavior. > > If you do not set a deallocator, it remains the caller > > responsibility. > > > > > What happens if x264 is not involved, but some other encoder, > > > which > > > does > > > not check for that kind of side data? > > > > > > How does the caller know that the data was consumed, and the > > > ownership > > > passed on? > > > > 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? > > > 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. Honestly, I do not see how this could ever happen. If you don't use libx264 you're likely not be using this kind of side data either. If you use libx264 it's up to you to decide whether you want to reliquish control over it to libx264 or not. Otherwise, a solution perfectly compatible with what you suggest is to get rid of the deallocator field altogether which would make it clear it's the caller's responsibility to free the data. I just see it like the frame data or any other data you're passing to the encoder. You just pass the frame data to the library and if you don't free the memory it's not the library fault, right? > > > 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? > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico _______________________________________________ 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".