[FFmpeg-devel] [PATCH 4/9] avutil: introduce an Immersive Audio Model and Formats API

[James Almer]

On 11/30/2023 10:47 AM, Anton Khirnov wrote:
> Quoting James Almer (2023-11-30 14:01:16)
>>
>> Should i link https://aomediacodec.github.io/iamf/ somewhere?
> 
> Most definitely.
> 
>>>
>>>> +/**
>>>> + * 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.
> 
> I'd drop it. The names are long enough as they are.
> 
>>>> +    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.
> 
> I prefer to be consistent with ourselves in this rather than a spec.
> Specs come and go.
> 
>>>
>>>> + *
>>>> + * 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.
> 
> I don't see how that can be done easily due to struct alignment. I was
> thinking you could make it into a flags field instead.

But this is the only boolean field. Also, there can be at most six 
layers, so it's not exactly a huge waste either way.