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

[Sebastian Vater]

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.

>
>> +    /** 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.

I originally used a doubly-linked list for this.

>> +
>> +    /** 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.
>
>> +    /** 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.

>
>> +    /** 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?

>
>> +} 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?

-- 

Best regards,
                   :-) Basty/CDGS (-: