[FFmpeg-soc] [soc] libavsequencer [PATCH 01/08] Music module public API header file.
Vitor Sessak
vitor1001 at gmail.com
Sat Jul 10 20:38:02 CEST 2010
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
More information about the FFmpeg-soc
mailing list