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 1D4CB45EE7 for ; Mon, 17 Apr 2023 17:20:03 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id BA57D68BE45; Mon, 17 Apr 2023 20:20:00 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id D36DD689E1F for ; Mon, 17 Apr 2023 20:19:53 +0300 (EEST) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id 91ACF2404EE for ; Mon, 17 Apr 2023 19:19:53 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavisd-new, port 10024) with ESMTP id Wsx_pIvhofcJ for ; Mon, 17 Apr 2023 19:19:52 +0200 (CEST) Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (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 "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id C90332404EC for ; Mon, 17 Apr 2023 19:19:52 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id B79051601B2; Mon, 17 Apr 2023 19:19:52 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: References: <20230417143930.1186-1-jamrial@gmail.com> Mail-Followup-To: FFmpeg development discussions and patches Date: Mon, 17 Apr 2023 19:19:52 +0200 Message-ID: <168175199272.3843.8298383638303959732@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH] avutil/frame: use bitfields for some boolean and enum fields 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: Quoting Lynne (2023-04-17 17:00:27) > Apr 17, 2023, 16:40 by jamrial@gmail.com: > > > Should reduce the size of AVFrame in the next major bump without changing the API. > > > > Suggested-by: Anton Khirnov > > Signed-off-by: James Almer > > --- > > This supersedes "avutil/frame: add new interlaced and top_field_first flags" > > and "avutil/frame: add a keyframe flag to AVFrame". > > > > libavutil/frame.h | 56 +++++++++++++++++++++++++++++++++++++++++++++ > > libavutil/version.h | 1 + > > 2 files changed, 57 insertions(+) > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index f85d630c5c..c26067f383 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -416,6 +416,7 @@ typedef struct AVFrame { > > */ > > int format; > > > > +#if FF_API_BITFIELDS > > /** > > * 1 -> keyframe, 0-> not > > */ > > @@ -425,6 +426,57 @@ typedef struct AVFrame { > > * Picture type of the frame. > > */ > > enum AVPictureType pict_type; > > +#else > > + /** > > + * 1 -> keyframe, 0-> not > > + */ > > + unsigned int key_frame: 1; > > + > > + /** > > + * The content of the picture is interlaced. > > + */ > > + unsigned int interlaced_frame: 1; > > + > > + /** > > + * If the content is interlaced, is top field displayed first. > > + */ > > + unsigned int top_field_first: 1; > > + > > + /** > > + * Tell user application that palette has changed from previous frame. > > + */ > > + unsigned int palette_has_changed: 1; > > + > > + /** > > + * Reserved. Must not be touched. > > + */ > > + unsigned int reserved_bitfield: (sizeof(unsigned int) * 8) - 9; > > + > > + /** > > + * MPEG vs JPEG YUV range. > > + * - encoding: Set by user > > + * - decoding: Set by libavcodec > > + */ > > + enum AVColorRange color_range: 2; > > + > > + enum AVChromaLocation chroma_location: 3; > > > > Definitely disagree on all non-8bit field limits. > The reserved_bitfield is especially ugly. > A few wasted bits wouldn't affect much, we don't even support building on 6502s. > Use bools, or limit them to 8bits so we can use bools when we bump? > The rest can be limited to 8bits. I'm not seeing any arguments for what exactly is improved by wasting almost 90% of bits in those fields. "Ugly" is not an argument, this is not a beauty pageant. The patch looks generally good, except for a few remarks: * why not merge the color bitfields with the interlacing ones * weren't we going to deprecate palette_has_changed? there's no use case for it. * only one free value for AVColorRange and AVChromaLocation, yet pict_type is 8 bits out of which most are unused and that doesn't seem likely to change; I'd leave space to double the currently defined range for each of those -- Anton Khirnov _______________________________________________ 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".