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 5C7F047E5C for ; Thu, 30 Nov 2023 11:01:40 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3E2A868D070; Thu, 30 Nov 2023 13:01:38 +0200 (EET) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 24A9D68CC6D for ; Thu, 30 Nov 2023 13:01:32 +0200 (EET) Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id DD25D2405ED for ; Thu, 30 Nov 2023 12:01:31 +0100 (CET) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavis, port 10024) with ESMTP id caL_PG3ldTHy for ; Thu, 30 Nov 2023 12:01:31 +0100 (CET) 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 34F6124043D for ; Thu, 30 Nov 2023 12:01:31 +0100 (CET) Received: by lain.khirnov.net (Postfix, from userid 1000) id 0D21C1601B9; Thu, 30 Nov 2023 12:01:31 +0100 (CET) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <20231126012858.40388-5-jamrial@gmail.com> References: <20231126012858.40388-1-jamrial@gmail.com> <20231126012858.40388-5-jamrial@gmail.com> Mail-Followup-To: FFmpeg development discussions and patches Date: Thu, 30 Nov 2023 12:01:31 +0100 Message-ID: <170134209102.8914.7650187143643457061@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 4/9] avutil: introduce an Immersive Audio Model and Formats API 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 James Almer (2023-11-26 02:28:53) > diff --git a/libavutil/iamf.h b/libavutil/iamf.h > new file mode 100644 > index 0000000000..1f4919efdb > --- /dev/null > +++ b/libavutil/iamf.h > +enum AVIAMFAudioElementType { > + AV_IAMF_AUDIO_ELEMENT_TYPE_CHANNEL, > + AV_IAMF_AUDIO_ELEMENT_TYPE_SCENE, 'audio' in the names is redundant and makes already long identifiers unnecessarily longer > +}; > + > +/** > + * @defgroup lavf_iamf_params Parameter Definition > + * @{ > + * Parameters as defined in section 3.6.1 and 3.8 of what? > +/** > + * Mix Gain Parameter Data as defined in section 3.8.1 > + * > + * Subblocks in AVIAMFParamDefinition use this struct when the value or > + * @ref AVIAMFParamDefinition.param_definition_type param_definition_type is > + * AV_IAMF_PARAMETER_DEFINITION_MIX_GAIN. > + */ > +typedef struct AVIAMFMixGainParameterData { Does 'ParameterData' at the end really serve any purpose? > + const AVClass *av_class; > + > + // AVOption enabled fields > + unsigned int subblock_duration; > + enum AVIAMFAnimationType animation_type; > + AVRational start_point_value; > + AVRational end_point_value; > + AVRational control_point_value; > + unsigned int control_point_relative_time; All these should really be documented. Also, some vertical alignment would improve readability. > +/** > + * Parameters as defined in section 3.6.1 This really REALLY needs more documentation. > + */ > +typedef struct AVIAMFParamDefinition { > + const AVClass *av_class; > + > + size_t subblocks_offset; > + size_t subblock_size; > + > + enum AVIAMFParamDefinitionType param_definition_type; > + unsigned int num_subblocks; We use nb_foo generally. > +AVIAMFParamDefinition *av_iamf_param_definition_alloc(enum AVIAMFParamDefinitionType param_definition_type, > + AVDictionary **options, > + unsigned int num_subblocks, AVDictionary **subblock_options, What are the dicts for? > + * > + * When audio_element_type is AV_IAMF_AUDIO_ELEMENT_TYPE_CHANNEL, this > + * corresponds to an Scalable Channel Layout layer as defined in section 3.6.2. > + * For AV_IAMF_AUDIO_ELEMENT_TYPE_SCENE, it is an Ambisonics channel > + * layout as defined in section 3.6.3 > + */ > +typedef struct AVIAMFLayer { > + const AVClass *av_class; > + > + // AVOption enabled fields > + AVChannelLayout ch_layout; > + > + unsigned int recon_gain_is_present; Every time you dedicate 4 bytes to storing one bit, God kills a kitten. > + /** > + * Output gain flags as defined in section 3.6.2 It would be really really nice if people could understand the struct contents without some external document. > + * This field is defined only if audio_element_type is presumably the parent's audio_element_type > + * AV_IAMF_AUDIO_ELEMENT_TYPE_CHANNEL, must be 0 otherwise. > + */ > + unsigned int output_gain_flags; > + /** > + * Output gain as defined in section 3.6.2 > + * > + * Must be 0 if @ref output_gain_flags is 0. > + */ > + AVRational output_gain; > + /** > + * Ambisonics mode as defined in section 3.6.3 > + * > + * This field is defined only if audio_element_type is > + * AV_IAMF_AUDIO_ELEMENT_TYPE_SCENE, must be 0 otherwise. > + * > + * If 0, channel_mapping is defined implicitly (Ambisonic Order) > + * or explicitly (Custom Order with ambi channels) in @ref ch_layout. > + * If 1, @ref demixing_matrix must be set. > + */ > + enum AVIAMFAmbisonicsMode ambisonics_mode; > + > + // End of AVOption enabled fields What purpose does this comment serve? > + /** > + * Demixing matrix as defined in section 3.6.3 > + * > + * Set only if @ref ambisonics_mode == 1, must be NULL otherwise. > + */ > + AVRational *demixing_matrix; Who sets this? > +typedef struct AVIAMFAudioElement { > + const AVClass *av_class; > + > + AVIAMFLayer **layers; > + /** > + * Number of layers, or channel groups, in the Audio Element. > + * For audio_element_type AV_IAMF_AUDIO_ELEMENT_TYPE_SCENE, there > + * may be exactly 1. > + * > + * Set by av_iamf_audio_element_add_layer(), must not be > + * modified by any other code. > + */ > + unsigned int num_layers; > + > + unsigned int codec_config_id; ??? > +int av_iamf_audio_element_add_layer(AVIAMFAudioElement *audio_element, AVDictionary **options); I would much prefer to have the caller call av_opt_set* manually rather than sprinkle AVDictionary function arguments everywhere. Do note that their usage in lavc and lavf APIs is out of necessity, not because it's very pretty. -- 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".