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 88BF547EA2 for ; Thu, 30 Nov 2023 13:01:04 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6EEDA68D044; Thu, 30 Nov 2023 15:01:01 +0200 (EET) Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6F38168D02F for ; Thu, 30 Nov 2023 15:00:54 +0200 (EET) Received: by mail-pf1-f180.google.com with SMTP id d2e1a72fcca58-6cb55001124so1615922b3a.0 for ; Thu, 30 Nov 2023 05:00:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701349251; x=1701954051; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=k8ZXUMUPf3bsB1g2vmD6lTL4lqLHyS/qeoGmb7mzqtU=; b=dGitZ02PQXLelXzctd3v5n9xcsE89W/DndhEuHey9QIxMByRr+DM0LLLhBVGllNOy2 HhXsTzDkOl7XfmYX5j6iErcXyundwW9FBNNrHhgfj7EUSh3LRhCL6aRoELzkhDyU0e5j 5rwOUNxNH+hx+WxDeOedGRZaQ9BTRS3mdoJcaakGp1XRiC8sxXuvUjbETrqMNTfqw3tq uJGSCjZaVZ7nhZoZgiBvtnSLGGbmTFPOtTInA/13y8rF+zL6jNZcRD4hqoHs7Kr2LcZq lTDniD8mzcDyu0hA6HrADZv+tJnG/Ek/Lo8L2eK/ZYBX7eZ2gQRFKMf54CmLEKE17YvI SnXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701349251; x=1701954051; h=content-transfer-encoding:in-reply-to:autocrypt:from :content-language:references:to:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=k8ZXUMUPf3bsB1g2vmD6lTL4lqLHyS/qeoGmb7mzqtU=; b=VNwkaaeZH9jYL3gqiEFiEbNmyQkp7MkX79kPF5dJ3/yiCWmM7hBkZdpnMgBk5qj+kP nQTzWhv654daDvXhTKZY2aBdELg+l6xmcJBRxPLQ96xrQXRp7NqC3893XNNe8fPs3r27 /15OxaOolY5nsRfqd9Pxpp5kfnoGXjeY4JL/ZpPoeIDN0bM5L3ZKkI7fhqqthr6hH0CG Qikjjc28DkPd6lks6Pix5lpUA9W9zGohS7Q7P6QMuDiLjJPCxD03bk0WovJwkI1B9iji /zdCDiGI9Lr7ZMJzqY3NeFusSjPd31yge9eSgghpi11R2bl+cVbajm93KvB5jKbj4AeR BtVA== X-Gm-Message-State: AOJu0Yz/Enp0kKGXvFDlxlUTlb4HO/JUstRvasD/5JqEUJb//XJ2ud2c LFdhgkwkmy84VO/AHtJet5qJaGMf4Tw= X-Google-Smtp-Source: AGHT+IGZuvfe2Zjdd2dDTF0YUIE2BzAaV0bPW419tIsE3upaLbWK0nt+48ovmq3nYg9fuy3gb0GXiw== X-Received: by 2002:a05:6a21:6d8a:b0:187:bb9c:569 with SMTP id wl10-20020a056a216d8a00b00187bb9c0569mr31233709pzb.5.1701349251058; Thu, 30 Nov 2023 05:00:51 -0800 (PST) Received: from [192.168.0.14] (host197.190-225-105.telecom.net.ar. [190.225.105.197]) by smtp.gmail.com with ESMTPSA id e21-20020a62aa15000000b006c03f58b5b1sm1132867pff.59.2023.11.30.05.00.49 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 30 Nov 2023 05:00:50 -0800 (PST) Message-ID: Date: Thu, 30 Nov 2023 10:01:16 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20231126012858.40388-1-jamrial@gmail.com> <20231126012858.40388-5-jamrial@gmail.com> <170134209102.8914.7650187143643457061@lain.khirnov.net> Content-Language: en-US From: James Almer Autocrypt: addr=jamrial@gmail.com; keydata= xsBNBFjZtqABCADLW+vdEoZaJZDsIO6geYFTOcn1unsEHefj9zn+3oTHlDFFzO47mzHsSfbK 9JE2xpOJEVnC8FAF5Sayi/pVwV+mtQUV3n5dgVeVBYF9GUQwOGFCpK8X54RRqhkgknbunOEE 0CtgAJgmpFmmmHgq02GvEspx1h/rh4apqwQR6QX4Favb+x9+i9ytVpwVcBX94vo2toyP7h/K BWfadQmb8ltgE1kshfg+SQs/H5bTV5Z1DuEASf02ZL/1qYB/sdTgWPLv9XMUHHsRFmMY8TMx wJSkP+Af3AiYQPJYz1B1D4tt98T/NoiVdin10zATakPjV8hXaobuRmxgakkUASXudydDABEB AAHNH0phbWVzIEFsbWVyIDxqYW1yaWFsQGdtYWlsLmNvbT7CwJIEEwEIADwCGwMGCwkIBwMC BhUIAgkKCwQWAgMBAh4BAheAFiEEd1EujP2UoWlX5pp6FGMBrXN2WeAFAmJoLUUCGQEACgkQ FGMBrXN2WeAFVQf9GtGhniRs1PzNUOgJktCnv6j4BbLieaIPYPEFXKDHOgjqQE2zVMYXnoXl Jam928ii902a8OY06r9ywn/R8ApD1/3NY/v64O71CY9scz5XyH2au8wIZ6HwFy3/f7sqjdGD uctY8Qs7rjT7NkoC5lmgMu2v2k03dGtM9AAf5AK5gU+H0EUw7vmKKiXzUqt5kvBuf4CEwXvH AQT1SMJ52rIlDWB7FQFyZeUbOAK2IgY/KNedfK6nsgd/eQVnlofPd2XoddE7kP6iys7jJefw DD3g3rZyDTq7in5dyk5glaNpWZpbHGBs+9SCYLnfQ8XvWqPFOD+gj0plamKANgOvavKTxM7A TQRY2bagAQgA69YtILj8kYxmqPr/M8+MXT7wVoOWVW9lvSmPquCELaDy/NIS7D06VC5EuE/6 JlJXZMTn37NLlyWhzwOgXuXw5w2tyoQQBuvqGiXJijuXwXH7HKdzrc6rpYtAqt5w05hzNrFS KrS0izG64VpWrfproy3BsL+8TBm9brLhhNPynVRqVukbbGzlATTzNQGZ14TTi2/dL6DkMQnM qn4jX9UEe4GdGQBP50bUJSSmeiIkyNLWA+znuN2PZEz930ZwNrF9GtDVw7mzcmpCZ7spldE2 tutbpy9D1bIqxyqBrYDSezyzL2adR1qgHyOTMCHg2AYNkrIQHrSyJxKTpZ1/hqOp8wARAQAB wsBfBBgBAgAJBQJY2bagAhsMAAoJEBRjAa1zdlnghekH/0Yb0iYJ74oID2f/Fj+AJKS2ekQF P2xOr8lpGzgp/+yWUvPtqbX0A33anBJdYwxaAC0NataX3tfZ+oJkzXqfmqhIHMPYHdZesJA2 Bk9hU/33mDl5s5U66/z0uelWzwKVHoQ2O6or4+qF3HJFSJLCe9uvWJ3zXf9F342Ftj73sfx+ 3xkw/IXsN1RqbYqDlzpoEQ99SIEfY/8Jjwnd3sIPfqkuyeaYfe6GJDqKawdCEP1oRRlbXEAp TJgYz8r3nPhGv9cdHNDCk44ISbsqVuxIEnLqi4fTPZaGupiQhT+srl268TTAp2TQW7+6Ce/b NPQorMquzS/LZoyALpmsYi/miMc= In-Reply-To: <170134209102.8914.7650187143643457061@lain.khirnov.net> 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 11/30/2023 8:01 AM, Anton Khirnov wrote: > 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 I'm trying to keep everything namespaced. Audio Elements are not the only part of the spec to use "element". See Submixes. > >> +}; >> + >> +/** >> + * @defgroup lavf_iamf_params Parameter Definition >> + * @{ >> + * Parameters as defined in section 3.6.1 and 3.8 > > of what? Should i link https://aomediacodec.github.io/iamf/ somewhere? > >> +/** >> + * 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? Follow the names as in https://aomediacodec.github.io/iamf/#obu-parameterblock I can change it to Parameters or Params, or just remove it. > >> + 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. Yes, was keeping better documentation for last. > >> + */ >> +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. For these public fields i'm keeping the same name as they are in the spec. I use nb_foo for arrays of structs in the demuxer/muxer patches. But i can change it if you prefer. > >> +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? Setting AVOptions for the AVIAMFParamDefinition and each subblock, respectively. > >> + * >> + * 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. I'll shave a few bytes. > >> + /** >> + * 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 Yes, forgot the @ref. Good catch. > >> + * 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? It was a reminder for me of what could be set through AVOptions. I'll remove it. > >> + /** >> + * 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? lavf for demuxing and the user for muxing, as usual. Will mention it. > >> +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; > > ??? Ah, good catch. Need to remove this and adapt the muxer. > >> +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. Alright, will try to remove it from most of these. _______________________________________________ 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".