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

[Anton Khirnov <anton@khirnov.net>]

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.

-- 
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".