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

[Vitor Sessak]

On 07/10/2010 07:57 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>>
>>> +#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);
>
> Already fixed in github.

It is still not good in github. Why duplicating the 
AVFormatContext->metadata as AVSenquencerSynth->metadata?

>>> +    /** 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.
>
> Sub-songs and instruments can also be added when tracker writers use
> FFmpeg as a base. The user e.g. can add a new instrument at any place.

Since I imagine that adding new instruments, songs, etc should be pretty 
rare and their number should be pretty small, I imagine that the extra 
simplicity of using an array is worth the O(n) cost to add an item.

>>> +
>>> +    /** 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.
>
> Fixed in github by changing the comment to:
>      /** Module playback flags. Currently, no flags are defined.  */
>
> Maybe that is better? Or do you still recommend to remove it completely.

If it is currently unused, I think the variable should be removed 
altogether. We can add it later when needed.

>>> +    /** 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?
>
> Thought it might be better to have a generic chunk format declared.

Why not then a "void *"?

>>> +    /** 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*.
>
> Fixed in github by removing user_data in all header files.
>
> However, these structures are all to be supposed to be writable by the
> client, how you otherwise could write a tracker using FFmpeg, when the
> stuff the user edits can't be applied to this structure?

I agree.

>>
>>> +} 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).
>
> Yes, when the file is opened and added to AVSequencerContext->modules
> list. I just see that there is a parameter missing, AVSequencerContext
> *avctx...should I change the same to avseq_module_open instead?

Yes.

-Vitor