[FFmpeg-soc] [soc] libavsequencer [PATCH 01/08] Music module public API header file.

[Vitor Sessak]

Hi,

Some comments (in this file and in a few others)...

On 07/07/2010 10:46 PM, Sebastian Vater wrote:
> diff --git a/libavsequencer/module.h b/libavsequencer/module.h
> new file mode 100755
> index 0000000..af8fa89

[...]

> +#include "libavsequencer/avsequencer.h"
> +#include "libavsequencer/player.h"
> +#include "libavutil/tree.h"
> +
> +/**
> + * Sequencer module structure.
> + * New fields can be added to the end with minor version bumps.
> + * Removal, reordering and changes to existing fields require a major
> + * version bump.
> + */
> +typedef struct AVSequencerModule {
> +    /** Metadata information: Original module file name, module name,
> +     *  module message, artist, genre, album, begin and finish date of
> +     * composition and comment.  */
> +    AVSequencerMetadata *metadata;

I don't think you need any struct for the metadata: AVFormatContext has 
one already. Note that our metadata API allows you to add any name for 
metadata by adding for ex.

av_metadata_set2(&s->metadata, "module message", module_message, 
AV_METADATA_DONT_STRDUP_VAL);

> +    /** AVSequencerPlayerChannel pointer to virtual channel data.  */
> +    AVSequencerPlayerChannel *channel_data;
> +
> +    /** Integer indexed tree root of sub-songs available in
> +       this module with AVTreeNode->elem being a AVSequencerSong.  */
> +    AVTreeNode *song_list;

Why don't you change the numbering of the sub-songs so they are 
sequential and then you can use a plain array instead of a tree?

Same for the other trees.

> +    /** Integer indexed tree root of instruments available for
> +       the whole module (shared between all sub-songs) with
> +       AVTreeNode->elem being a AVSequencerInstrument.  */
> +    AVTreeNode *instrument_list;
> +
> +    /** Integer indexed tree root of envelopes used by module
> +       with AVTreeNode->elem being a AVSequencerEnvelope.
> +       There can be vibrato, tremolo, panbrello, spenolo,
> +       volume, panning, pitch, envelopes, a resonance filter and
> +       finally the auto vibrato, tremolo and panbrello envelopes.  */
> +    AVTreeNode *envelopes_list;
> +
> +    /** Integer indexed tree root of keyboard definitions
> +       with AVTreeNode->elem being a AVSequencerKeyboard.
> +       A keyboard definition maps an instrument octave/note-pair
> +       to the sample number being played.  */
> +    AVTreeNode *keyboard_list;
> +
> +    /** Integer indexed tree root of arpeggio envelopes
> +       with AVTreeNode->elem being a AVSequencerArpeggioEnvelope.
> +       Arpeggio envelopes allow to fully customize the arpeggio
> +       command by playing the envelope instead of only a
> +       repetive set of 3 different notes.  */
> +    AVTreeNode *arp_env_list;
> +
> +    /** Duration of the module, in AV_TIME_BASE fractional
> +       seconds. This is the total sum of all sub-song durations
> +       this module contains.  */
> +    uint64_t duration;
> +
> +    /** Maximum number of virtual channels, including NNA (New Note
> +       Action) background channels to be allocated and processed by
> +       the mixing engine (defaults to 64).  */
> +    uint16_t channels;
> +#define AVSEQ_MODULE_CHANNELS   64
> +
> +    /** Compatibility flags for playback. There are rare cases
> +       where effect handling can not be mapped into internal
> +       playback engine and have to be handled specially. For
> +       each module which needs this, this will define new
> +       flags which tag the player to handle it to that special
> +       way.  */
> +    int8_t compat_flags;
> +
> +    /** Module playback flags.  */
> +    int8_t flags;

The comment can be removed, it don't add any information that is not 
either in the struct name or the var name.

> +    /** 64-bit integer indexed unique key tree root of unknown data
> +       fields for input file reading with AVTreeNode->elem being
> +       unsigned 8-bit integer data. Some formats are chunk based
> +       and can store information, which can't be handled by some
> +       other, in case of a transition the unknown data is kept as
> +       is. Some programs write editor settings for module in those
> +       chunks, which won't get lost then. The first 8 bytes of this
> +       data is an unsigned 64-bit integer length in bytes of
> +       the unknown data.  */
> +    AVTreeNode *unknown_data;

Might make sense storing it somewhere, but why not a plain buffer?

> +    /** This is just a data field where the user solely
> +       decides, what the usage (if any) will be.  */
> +    uint8_t *user_data;

This struct is not supposed to be writable by the client, so I don't 
think this field make sense. This is also inconsistent with the rest of 
libav*.

> +} AVSequencerModule;
> +
> +/**
> + * Registers a module to the AVSequencer.
> + *
> + * @param module the AVSequencerModule to be registered
> + * @return >= 0 on success, error code otherwise
> + *
> + * @NOTE This is part of the new sequencer API which is still under construction.
> + *       Thus do not use this yet. It may change at any time, do not expect
> + *       ABI compatibility yet!
> + */
> +int avseq_module_register(AVSequencerModule *module);

?
Should this be created only when the file is opened? Normally, we have 
xxx_register() only for things that are statically initialized before 
opening any file (codecs, demuxers, etc).

-Vitor