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
_______________________________________________
FFmpeg-soc mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
