[soc] libavsequencer [PATCH 01/08] Music module public API header file.
-- Best regards, :-) Basty/CDGS (-:
On date Wednesday 2010-07-07 22:46:08 +0200, Sebastian Vater encoded:
--
Best regards, :-) Basty/CDGS (-:
diff --git a/libavsequencer/module.h b/libavsequencer/module.h new file mode 100755 index 0000000..af8fa89 --- /dev/null +++ b/libavsequencer/module.h @@ -0,0 +1,124 @@ +/* + * AVSequencer music module management + * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVSEQUENCER_MODULE_H +#define AVSEQUENCER_MODULE_H + +#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. + */
Please describe the general architecture for libavsequencer. It looks really complex, and we need to understand how the various components iteract. In particular we need to understand how a mod decoder can be built and integrated into FFmpeg. In particular we need to understand which will be the role of the player, which will likely be the first component to be reviewed.
+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;
AVSequencerMetadata is not shown in your patches, also I wonder, won't be this a repetition of libavformat AVMetaData?
+ + /** AVSequencerPlayerChannel pointer to virtual channel data. */ + AVSequencerPlayerChannel *channel_data;
Same I cannot see definition for this.
+ + /** Integer indexed tree root of sub-songs available in + this module with AVTreeNode->elem being a AVSequencerSong. */ + AVTreeNode *song_list; + + /** 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; + + /** 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; + + /** This is just a data field where the user solely + decides, what the usage (if any) will be. */ + uint8_t *user_data; + +} AVSequencerModule; +
+/** + * Registers a module to the AVSequencer.
Register, as per the new style.
+ * + * @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.
@note (case is meaningful).
+ * Thus do not use this yet. It may change at any time, do not expect + * ABI compatibility yet! + */ +int avseq_module_register(AVSequencerModule *module);
+ +#endif /* AVSEQUENCER_MODULE_H */
Regards.
Stefano Sabatini a écrit :
+ +/** + * 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. + */
Please describe the general architecture for libavsequencer. It looks really complex, and we need to understand how the various components iteract. In particular we need to understand how a mod decoder can be built and integrated into FFmpeg.
avsequencer.h will follow the next days. ;) This file is just about the actual module. Not the generic architecture which will all go into lavs/avsequencer.h
In particular we need to understand which will be the role of the player, which will likely be the first component to be reviewed.
All what is solely belonging to the player will go into lavs/player.h lavs/(module/song/order/track/instr/sample/synth).h will be both for editors and players.
+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;
AVSequencerMetadata is not shown in your patches, also I wonder, won't be this a repetition of libavformat AVMetaData?
In most terms, yes, but some data is not available in AVMetadata (like module file name and start and finish date of composition). I was looking at it, before I decided to use AVSequencerMetadata or AVMetadata. But maybe it's really a better solution to add these fields to AVMetadata directly?
+ + /** AVSequencerPlayerChannel pointer to virtual channel data. */ + AVSequencerPlayerChannel *channel_data;
Same I cannot see definition for this.
See lavs/player.h (PATCH 08/08), all typedef struct's starting with AVSequencerPlayer* are in player.h ;-)
+ /** Integer indexed tree root of sub-songs available in + this module with AVTreeNode->elem being a AVSequencerSong. */ + AVTreeNode *song_list; + + /** 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; + + /** 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; + + /** This is just a data field where the user solely + decides, what the usage (if any) will be. */ + uint8_t *user_data; + +} AVSequencerModule; +
+/** + * Registers a module to the AVSequencer.
Register, as per the new style.
Fixed locally.
+ * + * @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.
@note (case is meaningful).
Oooops...fixed locally! Please note I can't commit fixes to github (see my eMail) anymore since git rebase -i! Please see my mail. So for now, fixed only locally. -- Best regards, :-) Basty/CDGS (-:
Sebastian Vater a écrit :
Stefano Sabatini a écrit :
+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;
AVSequencerMetadata is not shown in your patches, also I wonder, won't be this a repetition of libavformat AVMetaData?
Fixed by globally replacing AVSequencerMetadata to AVMetadata, I decided it's way better to implement the new tags in AVMetadata itself than duplicating all that stuff. 10l to me...
Fixed locally.
Fixed now in github also.
+ * + * @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.
@note (case is meaningful).
Oooops...fixed locally!
Fixed now in github also.
Please note I can't commit fixes to github (see my eMail) anymore since git rebase -i! Please see my mail. So for now, fixed only locally.
Fixed. ;-) Please update your repository pointing to http://github.com/BastyCDGS/ffmpeg-soc One question though, should I mail such small patches here also, or is it enough to mention that they are fixed? -- Best regards, :-) Basty/CDGS (-:
On date Saturday 2010-07-10 15:11:03 +0200, Sebastian Vater encoded:
Sebastian Vater a écrit : [...] Please update your repository pointing to http://github.com/BastyCDGS/ffmpeg-soc
One question though, should I mail such small patches here also, or is it enough to mention that they are fixed?
Yes please post the patches here, we are lazy-busy so having less steps to do will help to make review faster, also posting the patch allows in-line comments. Regards.
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
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 (-:
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
Vitor Sessak a écrit :
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?
You surely meant AVSequencerModule->metadata not synth, right?
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.
I agree totally with that! O(1) for access is way more important than inserting or deleting. What do you think about: AVSequencerInstrument **instrument_list? So we have just a pointer list to move around instead the huge structures?
If it is currently unused, I think the variable should be removed altogether. We can add it later when needed.
Fixed locally right now (will commit soon) by removing both compat_flags and flags, since compat_flags is also unused in the module right now.
+ /** 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?
You mean like: uint8_t **unknown_data?
Why not then a "void *"?
void ** or uint8_t ** would be better I think...each chunk is a pointer then. That way single chunks can be changed without recalculating everything else.
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.
user_data is removed though, since rethinking this took me to the conclusion that it's better to let the application developer decide this and if he needs extra data for this, he can still do: typedef struct MyTrackerModule { AVSequencerModule module; [...] custom stuff follows here with exact declaration } MyTrackerModule;
+} 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.
Fixed locally, will commit that either tomorrow or before going to bed. -- Best regards, :-) Basty/CDGS (-:
On 07/10/2010 11:31 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
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?
You surely meant AVSequencerModule->metadata not synth, right?
Oops, there is a file metadata inside the synth data? I hope some demuxer expert can tell how to set metadata in these cases...
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.
I agree totally with that! O(1) for access is way more important than inserting or deleting. What do you think about: AVSequencerInstrument **instrument_list?
So we have just a pointer list to move around instead the huge structures?
I don't know what typical values are, but is the folowing doable? #define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS]; Everything access and insertion O(1) and both done in one line of code...
If it is currently unused, I think the variable should be removed altogether. We can add it later when needed.
Fixed locally right now (will commit soon) by removing both compat_flags and flags, since compat_flags is also unused in the module right now.
Nice.
+ /** 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?
You mean like: uint8_t **unknown_data?
Why not then a "void *"?
void ** or uint8_t ** would be better I think...each chunk is a pointer then. That way single chunks can be changed without recalculating everything else.
Why do you think in chunks? They might be organized in a tree inside the file or a circularly linked list, or etc. void * do not exclude int8_t **...
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.
user_data is removed though, since rethinking this took me to the conclusion that it's better to let the application developer decide this and if he needs extra data for this, he can still do: typedef struct MyTrackerModule { AVSequencerModule module; [...] custom stuff follows here with exact declaration } MyTrackerModule;
Nice. -Vitor
Vitor Sessak a écrit :
On 07/10/2010 11:31 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
It is still not good in github. Why duplicating the AVFormatContext->metadata as AVSenquencerSynth->metadata?
You surely meant AVSequencerModule->metadata not synth, right?
Oops, there is a file metadata inside the synth data? I hope some demuxer expert can tell how to set metadata in these cases...
Yes, consider the following, you have a sample which contains a synth sound structure and you want to describe it someway, since there is also custom synth sound code possible, it might be a good idea, setting an artist (i.e. author of the code), some comments, etc.
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case. But I have another idea, we have a CONFIG_SMALL option right? We could do my idea, if CONFIG_SMALL is defined and when it's undefined we use your MAX_INSTRUMENTS approach?
void ** or uint8_t ** would be better I think...each chunk is a pointer then. That way single chunks can be changed without recalculating everything else.
Why do you think in chunks? They might be organized in a tree inside the file or a circularly linked list, or etc. void * do not exclude int8_t **...
Remember that the unknown data is only unknown to TuComposer/FFmpeg, the GUI editor / whatever calling it might know (partially or fully) the data, but to find these, the data has to be organized some way. My thoughts on this were, that the applications search this list, match their identifier and then evaluate just their data. -- Best regards, :-) Basty/CDGS (-:
On 07/11/2010 12:58 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/10/2010 11:31 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
It is still not good in github. Why duplicating the AVFormatContext->metadata as AVSenquencerSynth->metadata?
You surely meant AVSequencerModule->metadata not synth, right?
Oops, there is a file metadata inside the synth data? I hope some demuxer expert can tell how to set metadata in these cases...
Yes, consider the following, you have a sample which contains a synth sound structure and you want to describe it someway, since there is also custom synth sound code possible, it might be a good idea, setting an artist (i.e. author of the code), some comments, etc.
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case.
I don't mean the theoretical limit, I mean the biggest value ever used in any actual file.
But I have another idea, we have a CONFIG_SMALL option right?
We could do my idea, if CONFIG_SMALL is defined and when it's undefined we use your MAX_INSTRUMENTS approach?
No, the whole point of a MAX_INSTRUMENTS is to make things simpler and this would ruin it.
void ** or uint8_t ** would be better I think...each chunk is a pointer then. That way single chunks can be changed without recalculating everything else.
Why do you think in chunks? They might be organized in a tree inside the file or a circularly linked list, or etc. void * do not exclude int8_t **...
Remember that the unknown data is only unknown to TuComposer/FFmpeg, the GUI editor / whatever calling it might know (partially or fully) the data, but to find these, the data has to be organized some way. My thoughts on this were, that the applications search this list, match their identifier and then evaluate just their data.
I don't really like to incentive the behavior of adding application-specific data to the files... -Vitor
Vitor Sessak a écrit :
On 07/11/2010 12:58 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case.
I don't mean the theoretical limit, I mean the biggest value ever used in any actual file.
Hmm, and how to determine that? Including all the future files that will ever be created? Another possibility would be simply stick to 2^n size, with a start let's say of 1, and if we add a new instrument which makes the list larger or equal to (2^n) + 1, we increase the size to 2^(n+1). Anyway, when the limit is 64k, we have at worst O(65535) which should even not a problem for 20 year old CPUs. And even if the user has to wait half a second here, we can accept this (in that case, the user would simply add 16 instruments at once to avoid the delay).
But I have another idea, we have a CONFIG_SMALL option right?
We could do my idea, if CONFIG_SMALL is defined and when it's undefined we use your MAX_INSTRUMENTS approach?
No, the whole point of a MAX_INSTRUMENTS is to make things simpler and this would ruin it.
Ok idea discarded. ;-)
Remember that the unknown data is only unknown to TuComposer/FFmpeg, the GUI editor / whatever calling it might know (partially or fully) the data, but to find these, the data has to be organized some way. My thoughts on this were, that the applications search this list, match their identifier and then evaluate just their data.
I don't really like to incentive the behavior of adding application-specific data to the files...
Unfortunately, some do this, and chunk based files like RIFF/IFF are especially designed for that. Expect to find this, even the IFF-ILBM most popular paint program (DPaint) did this. I'm not 100% sure about this, but MIDI files edited by professional sequencers like CuBase probably also store extra data in these, sometimes even important for final song output (i.e. adding custom samples this way). -- Best regards, :-) Basty/CDGS (-:
On 07/11/2010 01:17 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 12:58 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case.
I don't mean the theoretical limit, I mean the biggest value ever used in any actual file.
Hmm, and how to determine that? Including all the future files that will ever be created?
What was the biggest value you ever encountered? If it's 20, setting it to 100 might be safe. Those who want to try weird experiments with crazy files are free to increase it and recompile ;)
Another possibility would be simply stick to 2^n size, with a start let's say of 1, and if we add a new instrument which makes the list larger or equal to (2^n) + 1, we increase the size to 2^(n+1).
Anyway, when the limit is 64k, we have at worst O(65535) which should even not a problem for 20 year old CPUs. And even if the user has to wait half a second here, we can accept this (in that case, the user would simply add 16 instruments at once to avoid the delay).
That is not important now. If we can not set a maximum value for it we let to whatever code that is going to create this struct to alloc it using the best method for its particular problem...
But I have another idea, we have a CONFIG_SMALL option right?
We could do my idea, if CONFIG_SMALL is defined and when it's undefined we use your MAX_INSTRUMENTS approach?
No, the whole point of a MAX_INSTRUMENTS is to make things simpler and this would ruin it.
Ok idea discarded. ;-)
Remember that the unknown data is only unknown to TuComposer/FFmpeg, the GUI editor / whatever calling it might know (partially or fully) the data, but to find these, the data has to be organized some way. My thoughts on this were, that the applications search this list, match their identifier and then evaluate just their data.
I don't really like to incentive the behavior of adding application-specific data to the files...
Unfortunately, some do this, and chunk based files like RIFF/IFF are especially designed for that. Expect to find this, even the IFF-ILBM most popular paint program (DPaint) did this.
If all the formats you know that support this has this software-specific data in chunks, uint8_t ** is ok. -Vitor
On Sun, Jul 11, 2010 at 01:27:20AM +0200, Vitor Sessak wrote:
On 07/11/2010 01:17 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 12:58 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case.
I don't mean the theoretical limit, I mean the biggest value ever used in any actual file.
Hmm, and how to determine that? Including all the future files that will ever be created?
What was the biggest value you ever encountered? If it's 20, setting it to 100 might be safe. Those who want to try weird experiments with crazy files are free to increase it and recompile ;)
Another possibility would be simply stick to 2^n size, with a start let's say of 1, and if we add a new instrument which makes the list larger or equal to (2^n) + 1, we increase the size to 2^(n+1).
Anyway, when the limit is 64k, we have at worst O(65535) which should even not a problem for 20 year old CPUs. And even if the user has to wait half a second here, we can accept this (in that case, the user would simply add 16 instruments at once to avoid the delay).
That is not important now. If we can not set a maximum value for it we let to whatever code that is going to create this struct to alloc it using the best method for its particular problem...
iam not sure if the MAX_INSTRUMENTS idea is all that great, considering how we are being hit by MAX_STREAMS currently. But i think an array of pointers thats allocated somewhere is a big improvent over the tree in terms of simplicity [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 3 "Rare item" - "Common item with rare defect or maybe just a lie" "Professional" - "'Toy' made in china, not functional except as doorstop" "Experts will know" - "The seller hopes you are not an expert"
Michael Niedermayer a écrit :
On Sun, Jul 11, 2010 at 01:27:20AM +0200, Vitor Sessak wrote:
On 07/11/2010 01:17 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 12:58 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
I don't know what typical values are, but is the folowing doable?
#define MAX_INSTRUMENTS [... some value here ...] AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];
Everything access and insertion O(1) and both done in one line of code...
The limit almost always 64k, which would be quite large in that case.
I don't mean the theoretical limit, I mean the biggest value ever used in any actual file.
Hmm, and how to determine that? Including all the future files that will ever be created?
What was the biggest value you ever encountered? If it's 20, setting it to 100 might be safe. Those who want to try weird experiments with crazy files are free to increase it and recompile ;)
Another possibility would be simply stick to 2^n size, with a start let's say of 1, and if we add a new instrument which makes the list larger or equal to (2^n) + 1, we increase the size to 2^(n+1).
Anyway, when the limit is 64k, we have at worst O(65535) which should even not a problem for 20 year old CPUs. And even if the user has to wait half a second here, we can accept this (in that case, the user would simply add 16 instruments at once to avoid the delay).
That is not important now. If we can not set a maximum value for it we let to whatever code that is going to create this struct to alloc it using the best method for its particular problem...
iam not sure if the MAX_INSTRUMENTS idea is all that great, considering how we are being hit by MAX_STREAMS currently. But i think an array of pointers thats allocated somewhere is a big improvent over the tree in terms of simplicity [...]
Updated patch attached. -- Best regards, :-) Basty/CDGS (-:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
-- Best regards, :-) Basty/CDGS (-:
module.h.patch
/* * AVSequencer music module management * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com> * * This file is part of FFmpeg. * * FFmpeg is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * * FFmpeg is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * * You should have received a copy of the GNU Lesser General Public * License along with FFmpeg; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */
#ifndef AVSEQUENCER_MODULE_H #define AVSEQUENCER_MODULE_H
#include "libavformat/avformat.h" #include "libavsequencer/avsequencer.h" #include "libavsequencer/instr.h" #include "libavsequencer/player.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. */ AVMetadata *metadata;
/** AVSequencerPlayerChannel pointer to virtual channel data. */ AVSequencerPlayerChannel *channel_data;
Is there just a virtual channel data per file? And BTW, the "Player" part of the field bugs me. Does it really belongs to the BSS?
/** Array of pointers containing every sub-song for this module. */
"Array (of size songs) of pointers containing every sub-song for this module.", same for the others.
AVSequencerSong **song_list;
/** Array of pointers containing every instrument for this module. */ AVSequencerInstrument **instrument_list;
/** Array of pointers containing every evelope for this module. */ AVSequencerEnvelope **envelope_list;
/** Array of pointers containing every keyboard definitionb list
typo
for this module. */ AVSequencerKeyboard **keyboard_list;
/** Array of pointers containing every arpeggio envelope definition list for this module. */ AVSequencerArpeggio **arpeggio_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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
/** Number of sub-songs attached to this module. */ uint16_t songs;
Cosmetics: I prefer, for readability, to move this closer to AVSequencerSong **song_list. Same for the following.
/** Number of instruments attached to this module. */ uint16_t instruments;
/** Number of envelopes attached to this module. */ uint16_t envelopes;
/** Number of keyboard definitions attached to this module. */ uint16_t keyboards;
/** Number of arpeggio definitions attached to this module. */ uint16_t arpeggios;
/** 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;
Again, is it read from the file or calculated by the player?
#define AVSEQ_MODULE_CHANNELS 64
/** Array of pointers containing every unknown data field where the last element is indicated by a NULL pointer reference. The first 64-bit of the unknown data contains an unique identifier for this chunk and the second 64-bit data is actual unsigned length of the following raw 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 then won't get lost in that case. */ uint8_t **unknown_data; } AVSequencerModule;
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return >= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters? -Vitor
Hi, On Tue, Jul 13, 2010 at 6:04 AM, Vitor Sessak <vitor1001@gmail.com> wrote:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** 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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
Either way this belongs in AVFormatContext and the demuxer. Ronald
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 6:04 AM, Vitor Sessak <vitor1001@gmail.com> wrote:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** 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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
Either way this belongs in AVFormatContext and the demuxer.
Don't forget that the composer can edit this value in case if ever needed. TuComposer has a synth-sound assember which should be a turing machine making it impossible under certain situations to calculate playback time correctly, so the user can edit and adjust that manually. Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field. -- Best regards, :-) Basty/CDGS (-:
Hi, On Tue, Jul 13, 2010 at 4:15 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field.
So who reads the value then? It seems to me that if the user wants to stop the song, he simply stops calling avcodec_decode_audio() and no more audio will come out. Who would need to know this user-set value to justify it being in BSS? Ronald
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 4:15 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field.
So who reads the value then? It seems to me that if the user wants to stop the song, he simply stops calling avcodec_decode_audio() and no more audio will come out. Who would need to know this user-set value to justify it being in BSS?
It's free to someone to read it and use it, e.g. players which are in play-once mode instead of play-repeat, they can simply break playback when this time is reached and continue with the next song in the playlist. I have encoutered this with some exotic amiga formats, which because such a feature is not known (DeliPlayer has a global config file to set a max. playback time in that case and skip ALL files after that simple defined time), others simply loop the module forever in that case and never skip on to the next, forcing the user to manually skip forward one song. This can be really nasty, I prefer to start a playlist of modules and suddenly after some files have been playing, one loops forever and I had to deal with that manually, so I invented: Upon importing them to TuComposer, you can set a manual playback time for each file and TuComposer stores it, the player knows the playback time in that case and at reaching this it will skip normally to next module. Just consider you start XMMS, having your favourite songs in it, and suddenly one loops forever because lack of song-end detection...you're midden in a work of sth. and have to deal with that. FFmpeg coming soon formats having this issue can also set a default value or sth. like that by the demuxer, just an idea. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 10:15 PM, Sebastian Vater wrote:
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 6:04 AM, Vitor Sessak<vitor1001-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** 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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
Either way this belongs in AVFormatContext and the demuxer.
Don't forget that the composer can edit this value in case if ever needed. TuComposer has a synth-sound assember which should be a turing machine making it impossible under certain situations to calculate playback time correctly, so the user can edit and adjust that manually.
Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field.
From your explanation this field has two, possible conflicting, meanings: a) The known duration of the file b) The time after which the song should be forced by the player to finish (a) should be in lavf, while (b) should have zero as default value, meaning never force the song to end. I think you should leave this field just for (b) (and document it accordingly) and set (a) in lavf. I'd even rename it forced_song_end_time. -Vitor
Vitor Sessak a écrit :
On 07/13/2010 10:15 PM, Sebastian Vater wrote:
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 6:04 AM, Vitor Sessak<vitor1001-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** 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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
Either way this belongs in AVFormatContext and the demuxer.
Don't forget that the composer can edit this value in case if ever needed. TuComposer has a synth-sound assember which should be a turing machine making it impossible under certain situations to calculate playback time correctly, so the user can edit and adjust that manually.
Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field.
From your explanation this field has two, possible conflicting, meanings:
a) The known duration of the file b) The time after which the song should be forced by the player to finish
(a) should be in lavf, while (b) should have zero as default value, meaning never force the song to end.
I think you should leave this field just for (b) (and document it accordingly) and set (a) in lavf. I'd even rename it forced_song_end_time.
I fully agree! forced_duration would be OK, though? Another possibility would to mention that in the comment to this field. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 11:05 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:15 PM, Sebastian Vater wrote:
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 6:04 AM, Vitor Sessak<vitor1001-Re5JQEeQqe8AvxtiuMwx3w-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org> wrote:
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** 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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
Either way this belongs in AVFormatContext and the demuxer.
Don't forget that the composer can edit this value in case if ever needed. TuComposer has a synth-sound assember which should be a turing machine making it impossible under certain situations to calculate playback time correctly, so the user can edit and adjust that manually.
Also some formats don't have even something close like a clear song-end detection. In that case, the user can add it manually. That's the purpose of this field.
From your explanation this field has two, possible conflicting, meanings:
a) The known duration of the file b) The time after which the song should be forced by the player to finish
(a) should be in lavf, while (b) should have zero as default value, meaning never force the song to end.
I think you should leave this field just for (b) (and document it accordingly) and set (a) in lavf. I'd even rename it forced_song_end_time.
I fully agree! forced_duration would be OK, though?
Sure! This is an important part of the description of the file.
Another possibility would to mention that in the comment to this field.
You should mention what forced_duration means in the comment and what does it means it being 0. -Vitor
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
/** * 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. */ AVMetadata *metadata;
/** AVSequencerPlayerChannel pointer to virtual channel data. */ AVSequencerPlayerChannel *channel_data;
Is there just a virtual channel data per file? And BTW, the "Player" part of the field bugs me. Does it really belongs to the BSS?
Yes, you are right, this can be moved to AVSequencerPlayerGlobals and will fix this (will require some changes to player.c, though).
/** Array of pointers containing every sub-song for this module. */
"Array (of size songs) of pointers containing every sub-song for this module.", same for the others.
Fixed.
AVSequencerSong **song_list;
/** Array of pointers containing every instrument for this module. */ AVSequencerInstrument **instrument_list;
/** Array of pointers containing every evelope for this module. */ AVSequencerEnvelope **envelope_list;
/** Array of pointers containing every keyboard definitionb list
typo
Fixed.
for this module. */ AVSequencerKeyboard **keyboard_list;
/** Array of pointers containing every arpeggio envelope definition list for this module. */ AVSequencerArpeggio **arpeggio_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;
Is this ever written in the files or is it calculated by the player? If the later, it does not belong to the BSS.
This is exactly for one module, since AVSequencerContext can contain multiple modules (if an editor supports editing multiple modules at once), I decided to move it here, instead. This can both be calculated by the player but also be preset by the demuxer. Sometimes for very complex modules, automatic calculation is almost impossible, so the user can simply playback, look at the total time and enter it. This field is supposed to be editable by the composer.
/** Number of sub-songs attached to this module. */ uint16_t songs;
Cosmetics: I prefer, for readability, to move this closer to AVSequencerSong **song_list. Same for the following.
Fixed.
/** Number of instruments attached to this module. */ uint16_t instruments;
/** Number of envelopes attached to this module. */ uint16_t envelopes;
/** Number of keyboard definitions attached to this module. */ uint16_t keyboards;
/** Number of arpeggio definitions attached to this module. */ uint16_t arpeggios;
/** 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;
Again, is it read from the file or calculated by the player?
It can be set as maximum by the composer during editing. Most demuxers with non-NNA modes can set this automatically without problem, but for NNAs this is not possible, the user can set a maximum value (IT for example uses this). Not using that will not make the module sound as it originally was intended.
#define AVSEQ_MODULE_CHANNELS 64
/** Array of pointers containing every unknown data field where the last element is indicated by a NULL pointer reference. The first 64-bit of the unknown data contains an unique identifier for this chunk and the second 64-bit data is actual unsigned length of the following raw 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 then won't get lost in that case. */ uint8_t **unknown_data; } AVSequencerModule;
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return >= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx). The loading, of course, is not necessary in FFmpeg, as it will be done by the demuxer itself, but the register process. Also this function can be called from the user if he loads a module. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
The loading, of course, is not necessary in FFmpeg, as it will be done by the demuxer itself, but the register process. Also this function can be called from the user if he loads a module.
If the user wants to load a module, he should call lavf/ api, no? -Vitor
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time. So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself. To draw all this as a tree, I would do it the following way (AVSequencer is abbreviated simply lavs): lavsContext: 1. list of lavsModule -> 1.1. list of lavsSongs (sub-songs) 2. list of lavsInstruments (all instruments shared between sub-songs). 3. Same for envelopes, etc. lavsSongs of course descendends to order list and track data. lavsInstruments descend to samples, which descend to synths each. Maybe I should actually draw the dependency pattern with GIMP or sth. else? Like you did in the private mail?
The loading, of course, is not necessary in FFmpeg, as it will be done by the demuxer itself, but the register process. Also this function can be called from the user if he loads a module.
If the user wants to load a module, he should call lavf/ api, no?
Yes, that what I'm meaning, using the demuxer (which is in lavf/* right?) ;-) For tonight, I hope that's enough, I'm falling just to bed...hope didn't write any bullshit because if being tired. :-/ -- Best regards, :-) Basty/CDGS (-:
Hi, On Tue, Jul 13, 2010 at 5:55 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
To draw all this as a tree, I would do it the following way (AVSequencer is abbreviated simply lavs): lavsContext: 1. list of lavsModule -> 1.1. list of lavsSongs (sub-songs) 2. list of lavsInstruments (all instruments shared between sub-songs). 3. Same for envelopes, etc.
lavsSongs of course descendends to order list and track data. lavsInstruments descend to samples, which descend to synths each.
Why not open a new lavsContext per file? That's what libswscale does, for example. And which structure is BSS in this schematic? Ronald
Ronald S. Bultje a écrit :
Hi,
On Tue, Jul 13, 2010 at 5:55 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
To draw all this as a tree, I would do it the following way (AVSequencer is abbreviated simply lavs): lavsContext: 1. list of lavsModule -> 1.1. list of lavsSongs (sub-songs) 2. list of lavsInstruments (all instruments shared between sub-songs). 3. Same for envelopes, etc.
lavsSongs of course descendends to order list and track data. lavsInstruments descend to samples, which descend to synths each.
Why not open a new lavsContext per file? That's what libswscale does, for example. And which structure is BSS in this schematic?
Would be possible, too. But I did a small mistake in my tree, see my latest mail on this. Ok, after sending this one, the mail I meant is the 2nd latest. ;) Just thought that it would be more conclusive to have all the opened modules in one lavsContext. Sorry I'm probably still thinking a bit too much on TuComposer's terms. There lavsContext was the equivalent of TuComposer's base library which managed everything. -- Best regards, :-) Basty/CDGS (-:
Sebastian Vater a écrit :
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
To draw all this as a tree, I would do it the following way (AVSequencer is abbreviated simply lavs): lavsContext: 1. list of lavsModule -> 1.1. list of lavsSongs (sub-songs) 2. list of lavsInstruments (all instruments shared between sub-songs). 3. Same for envelopes, etc.
Sorry, 2. and 3. could of course go into 1.2. and 1.3... -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 11:55 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
And what does it change? Both lavf/ and lavc/ can handle several files at the same time (and I mean _really_ at the same time, it is fully thread-safe).
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
In the BSS, it does not make much sense an AVSequencerContext. Besides that, I agree with Ronald: why not a different player context for each file and let the application store them as it wish? -Vitor
Vitor Sessak a écrit :
On 07/13/2010 11:55 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
/** * Opens and registers module to the AVSequencer. * * @param avctx the AVSequencerContext to store the opened module into * @param module the AVSequencerModule which has been opened to be registered * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
And what does it change? Both lavf/ and lavc/ can handle several files at the same time (and I mean _really_ at the same time, it is fully thread-safe).
Great! Wasn't really aware of this! I have now to find a way to integrate the lavs base structs in lavs/module.h, as I understand it?
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
In the BSS, it does not make much sense an AVSequencerContext.
Besides that, I agree with Ronald: why not a different player context for each file and let the application store them as it wish?
This is clearly a until not-yet-known feature of FFmpeg which I didn't account to until now. Sorry for that! Now I have to guess the best way, to integrate lavs/avsequencer.h into lavs/module.h instead!? But let's file that tomorrow, otherwise you will read sth. like that the next mail: fjs9teuwJSDUHPuhfp Because falling on the keyboard for sake of tireness...;-) -- Best regards, :-) Basty/CDGS (-:
On 07/14/2010 12:16 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 11:55 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
> /** > * Opens and registers module to the AVSequencer. > * > * @param avctx the AVSequencerContext to store the opened module > into > * @param module the AVSequencerModule which has been opened to be > registered > * @return>= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule > *module);
Hmm, I think I don't really understand what this function do. Suppose you want to write a very short test program to play a MOD file using the libraries. Roughly, what functions it will call, in which order and with which parameters?
To be honest, I added this prototype only for now to see if the way I want to add them is compatible with FFmpeg style guide. Wanted just to avoid adding functions and all follow a wrong guideline. But the suppose is to load a module from disc and register it to the module list in avctx).
Guideline-wise it is fine, but I don't think functions should be added to the BSS structs without a very good reason.
That's clear! Just want to mention the point, that the user today expects the feature of having multiple (in this case modules) files opened at once and thus editing them. We're not anymore in the eighties were it was common just to have one file opened at the same time.
And what does it change? Both lavf/ and lavc/ can handle several files at the same time (and I mean _really_ at the same time, it is fully thread-safe).
Great! Wasn't really aware of this! I have now to find a way to integrate the lavs base structs in lavs/module.h, as I understand it?
So I thought of a way of allowing the user exactly that, that's why I plan to add multiple modules to AVSequencerContext. The programmer developing the GUI has to take care of this list though mostly by itself.
In the BSS, it does not make much sense an AVSequencerContext.
Besides that, I agree with Ronald: why not a different player context for each file and let the application store them as it wish?
This is clearly a until not-yet-known feature of FFmpeg which I didn't account to until now. Sorry for that! Now I have to guess the best way, to integrate lavs/avsequencer.h into lavs/module.h instead!?
No! avsequencer.h is part of the player and module.h is part of the BSS! They should be really well separated to have a clean architecture. Just do everything as if there were just a single file and, if you don't use global vars, everything will work well if you have two files with two different contexts. Changing a little bit the subject, I suggest that after you take into account all the feedback you had, you send a new patchset, this time containing just the headers for the BSS (and _only_ the BSS, without a single code line or define that belongs to the player!), so we can give a second round of reviewing.
But let's file that tomorrow, otherwise you will read sth. like that the next mail: fjs9teuwJSDUHPuhfp
Because falling on the keyboard for sake of tireness...;-)
Of course, take your time and have a good night! -Vitor
Vitor Sessak a écrit :
Changing a little bit the subject, I suggest that after you take into account all the feedback you had, you send a new patchset, this time containing just the headers for the BSS (and _only_ the BSS, without a single code line or define that belongs to the player!), so we can give a second round of reviewing.
So here is it, I merged them all into one to avoid flooding the ML. They're getting small enough now, to merge them all into one, I think! -- Best regards, :-) Basty/CDGS (-:
On 07/15/2010 12:18 AM, Sebastian Vater wrote:
Vitor Sessak a écrit :
Changing a little bit the subject, I suggest that after you take into account all the feedback you had, you send a new patchset, this time containing just the headers for the BSS (and _only_ the BSS, without a single code line or define that belongs to the player!), so we can give a second round of reviewing.
So here is it, I merged them all into one to avoid flooding the ML. They're getting small enough now, to merge them all into one, I think!
This is not a patch against latest SVN, this is a patch against your SoC tree and thus unreadable (your patch should contain all the lines of libavsequencer/* since none of this files exists now in main svn).
diff --git a/libavsequencer/track.h b/libavsequencer/track.h index c1370ed..33095d9 100755 --- a/libavsequencer/track.h +++ b/libavsequencer/track.h @@ -34,12 +34,18 @@ * version bump. */
[...]
/** 0x20 - Set volume:
Again, is saying "0x20" in the comment relevant? If this value for the set volume command is format-dependent (i.e., it is only 0x20 in MOD but 0x55 in XM), this is misleading.
Data word consists of two 8-bit pairs named xx and yy where xx is the volume level (ranging either from @@ -756,7 +729,8 @@ typedef struct AVSequencerTrackEffect { means that the tremolo envelope values will be inverted. */ #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F
Wasn't you going to change these (and many other) to a enum? I remember you said you would change it soon, but being afraid of breaking other files that uses this header, but what code exactly does replacing #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX 0x1D by enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, } breaks? -Vitor
Vitor Sessak a écrit :
On 07/15/2010 12:18 AM, Sebastian Vater wrote:
So here is it, I merged them all into one to avoid flooding the ML. They're getting small enough now, to merge them all into one, I think!
This is not a patch against latest SVN, this is a patch against your SoC tree and thus unreadable (your patch should contain all the lines of libavsequencer/* since none of this files exists now in main svn).
Fixed.
diff --git a/libavsequencer/track.h b/libavsequencer/track.h index c1370ed..33095d9 100755 --- a/libavsequencer/track.h +++ b/libavsequencer/track.h @@ -34,12 +34,18 @@ * version bump. */
[...]
/** 0x20 - Set volume:
Again, is saying "0x20" in the comment relevant? If this value for the set volume command is format-dependent (i.e., it is only 0x20 in MOD but 0x55 in XM), this is misleading.
The demuxer/decoder will convert this to effect number 0x20 anyway, in MOD the set volume command is C (internally = 0x02). The goal is to have a unique command set independent from the origin file.
Data word consists of two 8-bit pairs named xx and yy where xx is the volume level (ranging either from @@ -756,7 +729,8 @@ typedef struct AVSequencerTrackEffect { means that the tremolo envelope values will be inverted. */ #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F
Wasn't you going to change these (and many other) to a enum? I remember you said you would change it soon, but being afraid of breaking other files that uses this header, but what code exactly does replacing
Yes, will do this tonight.
#define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX 0x1D
by
enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, }
breaks?
I access them like: if (flags & AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. } Won't have change that to: if (flags & Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. } then? -- Best regards, :-) Basty/CDGS (-:
Hi, On Thu, Jul 15, 2010 at 8:28 AM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Vitor Sessak a écrit :
#define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX 0x1D
by
enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, }
breaks?
I access them like: if (flags & AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
Won't have change that to: if (flags & Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
then?
Only in C++. Not in C. :-). Ronald
Hi, On Thu, 15 Jul 2010, Ronald S. Bultje wrote:
On Thu, Jul 15, 2010 at 8:28 AM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
I access them like: if (flags & AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
Won't have change that to: if (flags & Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
then?
Only in C++. Not in C. :-).
FWIW, not in C++ either, you'd need to change it to Whatever::AV... only if it was defined within such a class or namespace, but the enum name itself is never needed. // Martin
Ronald S. Bultje a écrit :
Hi,
On Thu, Jul 15, 2010 at 8:28 AM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Vitor Sessak a écrit :
#define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX 0x1D
by
enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, }
breaks?
I access them like: if (flags & AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
Won't have change that to: if (flags & Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
then?
Only in C++. Not in C. :-).
I see, haven read the two replys from Martin and Vitor already. That's great and saves a lot of time. Just a question for understanding...what's the advantage of using enum's then? The only one I see straight away is that there is less space required (saving 4 characters by replacing '#define ' with ' '). What's also clear is that is enum is processed as a compile task after preprocessor stuff. Sorry, I know that really sounds like a stupid question, but I never worked with enum's, always did the #define stuff (you know, never change a running system and such things). I mean, if you want me to change this, there must actually be a good reason for this...which one this is? Just for informative tasks... -- Best regards, :-) Basty/CDGS (-:
Hi, On Thu, Jul 15, 2010 at 3:12 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Just a question for understanding...what's the advantage of using enum's then? The only one I see straight away is that there is less space required (saving 4 characters by replacing '#define ' with ' ').
Documentation tools (e.g. doxy) will group them together and treat them as a group of related properties, rather than separate entities. That's a big one for a huge project such as FFmpeg, where in the future many people will look at and modify your code, without necessarily understanding everything at the beginning. Ronald
On 07/15/2010 09:35 PM, Ronald S. Bultje wrote:
Hi,
On Thu, Jul 15, 2010 at 3:12 PM, Sebastian Vater <cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
Just a question for understanding...what's the advantage of using enum's then? The only one I see straight away is that there is less space required (saving 4 characters by replacing '#define ' with ' ').
Documentation tools (e.g. doxy) will group them together and treat them as a group of related properties, rather than separate entities. That's a big one for a huge project such as FFmpeg, where in the future many people will look at and modify your code, without necessarily understanding everything at the beginning.
Not to mention having variables of the type "enum Whatever" instead of a plain int, which tell immediately which kind of constants should be assigned to it. -Vitor
Vitor Sessak a écrit :
On 07/15/2010 09:35 PM, Ronald S. Bultje wrote:
Hi,
On Thu, Jul 15, 2010 at 3:12 PM, Sebastian Vater <cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
Just a question for understanding...what's the advantage of using enum's then? The only one I see straight away is that there is less space required (saving 4 characters by replacing '#define ' with ' ').
Documentation tools (e.g. doxy) will group them together and treat them as a group of related properties, rather than separate entities. That's a big one for a huge project such as FFmpeg, where in the future many people will look at and modify your code, without necessarily understanding everything at the beginning.
Not to mention having variables of the type "enum Whatever" instead of a plain int, which tell immediately which kind of constants should be assigned to it.
Hi I have excellent news! libavsequencer now flawlessly integrates into FFmpeg, just check out my latest git. Please do a git pull --rebase, Stefano had problems without using it. Here are the module.[ch] part of the BSS to review. This version compiles perfectly. -- Best regards, :-) Basty/CDGS (-:
On 08/07/2010 09:42 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/15/2010 09:35 PM, Ronald S. Bultje wrote:
Hi,
On Thu, Jul 15, 2010 at 3:12 PM, Sebastian Vater <cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
Just a question for understanding...what's the advantage of using enum's then? The only one I see straight away is that there is less space required (saving 4 characters by replacing '#define ' with ' ').
Documentation tools (e.g. doxy) will group them together and treat them as a group of related properties, rather than separate entities. That's a big one for a huge project such as FFmpeg, where in the future many people will look at and modify your code, without necessarily understanding everything at the beginning.
Not to mention having variables of the type "enum Whatever" instead of a plain int, which tell immediately which kind of constants should be assigned to it.
Hi I have excellent news!
libavsequencer now flawlessly integrates into FFmpeg, just check out my latest git. Please do a git pull --rebase, Stefano had problems without using it.
Here are the module.[ch] part of the BSS to review.
This version compiles perfectly.
diff --git a/libavsequencer/module.h b/libavsequencer/module.h new file mode 100755 index 0000000..abd9792 --- /dev/null +++ b/libavsequencer/module.h @@ -0,0 +1,108 @@ +/* + * AVSequencer music module management + * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVSEQUENCER_MODULE_H +#define AVSEQUENCER_MODULE_H + +#include "libavutil/log.h" +#include "libavformat/avformat.h" +#include "libavsequencer/song.h" +#include "libavsequencer/instr.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 { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original module file name, module name, + * module message, artist, genre, album, begin and finish date of + * composition and comment. */ + AVMetadata *metadata; + + /** Array (of size songs) of pointers containing every sub-song + for this module. */ + AVSequencerSong **song_list; + + /** Number of sub-songs attached to this module. */ + uint16_t songs; + + /** Array (of size instruments) of pointers containing every + instrument for this module. */ + AVSequencerInstrument **instrument_list; + + /** Number of instruments attached to this module. */ + uint16_t instruments; + + /** Array (of size envelopes) of pointers containing every + evelope for this module. */ + AVSequencerEnvelope **envelope_list; + + /** Number of envelopes attached to this module. */ + uint16_t envelopes; + + /** Array (of size keyboards) of pointers containing every + keyboard definition list for this module. */ + AVSequencerKeyboard **keyboard_list; + + /** Number of keyboard definitions attached to this module. */ + uint16_t keyboards; + + /** Array (of size arpeggios) of pointers containing every + arpeggio envelope definition list for this module. */ + AVSequencerArpeggio **arpeggio_list; + + /** Number of arpeggio definitions attached to this module. */ + uint16_t arpeggios;
+ /** Forced duration of the module, in AV_TIME_BASE fractional + seconds. This is the total sum of all sub-song durations + this module contains or zero if the duration is unknown and + also cannot be automatically determined. The composer then can + set manually a duration after which the player can skip over to + the next module if it is playing back in once mode. */ + uint64_t forced_duration;
This looks like a mix between what we discussed and what it was before. For me (and you seemed to agree), if the song finishes after the last note, this field should be set to zero, no matter if the duration can be automatically determined or not. If the duration _can_ be determined automatically, one should set the corresponding field of _AVCodecContext_ (obviously, when forced_duration != 0, we can always determine the song duration and it is equal to forced_duration). -Vitor
Vitor Sessak a écrit :
On 08/07/2010 09:42 PM, Sebastian Vater wrote:
+ /** Forced duration of the module, in AV_TIME_BASE fractional + seconds. This is the total sum of all sub-song durations + this module contains or zero if the duration is unknown and + also cannot be automatically determined. The composer then can + set manually a duration after which the player can skip over to + the next module if it is playing back in once mode. */ + uint64_t forced_duration;
This looks like a mix between what we discussed and what it was before. For me (and you seemed to agree), if the song finishes after the last note, this field should be set to zero, no matter if the duration can be automatically determined or not. If the duration _can_ be determined automatically, one should set the corresponding field of _AVCodecContext_ (obviously, when forced_duration != 0, we can always determine the song duration and it is equal to forced_duration).
Based on your description, we actually mean the same, maybe my documentation is misleading on this, do you have an idea to better write it? -- Best regards, :-) Basty/CDGS (-:
On 08/13/2010 10:45 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:42 PM, Sebastian Vater wrote:
+ /** Forced duration of the module, in AV_TIME_BASE fractional + seconds. This is the total sum of all sub-song durations + this module contains or zero if the duration is unknown and + also cannot be automatically determined. The composer then can + set manually a duration after which the player can skip over to + the next module if it is playing back in once mode. */ + uint64_t forced_duration;
This looks like a mix between what we discussed and what it was before. For me (and you seemed to agree), if the song finishes after the last note, this field should be set to zero, no matter if the duration can be automatically determined or not. If the duration _can_ be determined automatically, one should set the corresponding field of _AVCodecContext_ (obviously, when forced_duration != 0, we can always determine the song duration and it is equal to forced_duration).
Based on your description, we actually mean the same, maybe my documentation is misleading on this,
Yes, why do you mention "contains or zero if the duration is unknown and also cannot be automatically determined." if it should contain zero if the duration is known (or determined) but not to be enforced forcibly?
do you have an idea to better write it?
Sure: /** Forced duration of the module, in AV_TIME_BASE fractional seconds. If non-zero, the song should forcibly end after this duration, even if there are still notes to be played. */ uint64_t forced_duration; -Vitor
Vitor Sessak a écrit :
On 08/13/2010 10:45 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:42 PM, Sebastian Vater wrote:
+ /** Forced duration of the module, in AV_TIME_BASE fractional + seconds. This is the total sum of all sub-song durations + this module contains or zero if the duration is unknown and + also cannot be automatically determined. The composer then can + set manually a duration after which the player can skip over to + the next module if it is playing back in once mode. */ + uint64_t forced_duration;
This looks like a mix between what we discussed and what it was before. For me (and you seemed to agree), if the song finishes after the last note, this field should be set to zero, no matter if the duration can be automatically determined or not. If the duration _can_ be determined automatically, one should set the corresponding field of _AVCodecContext_ (obviously, when forced_duration != 0, we can always determine the song duration and it is equal to forced_duration).
Based on your description, we actually mean the same, maybe my documentation is misleading on this,
Yes, why do you mention "contains or zero if the duration is unknown and also cannot be automatically determined." if it should contain zero if the duration is known (or determined) but not to be enforced forcibly?
do you have an idea to better write it?
Sure:
/** Forced duration of the module, in AV_TIME_BASE fractional seconds. If non-zero, the song should forcibly end after this duration, even if there are still notes to be played. */ uint64_t forced_duration;
Fixed by your suggestions! -- Best regards, :-) Basty/CDGS (-:
Hi Sebastian, On Fri, Aug 13, 2010 at 8:06 PM, Sebastian Vater <cdgs.basty@googlemail.com> wrote:
Fixed by your suggestions!
For your next patch-set for BSS, can you send all BSS-related headers together, so that the one, single patch provides a set of headers that pass make checkheaders and can thus be reviewed easier? Note that it shouldn't contain any headers not part of BSS, it should only contain those headers part of BSS. Thanks! Ronald
On 07/15/2010 02:28 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/15/2010 12:18 AM, Sebastian Vater wrote:
So here is it, I merged them all into one to avoid flooding the ML. They're getting small enough now, to merge them all into one, I think!
This is not a patch against latest SVN, this is a patch against your SoC tree and thus unreadable (your patch should contain all the lines of libavsequencer/* since none of this files exists now in main svn).
Fixed.
diff --git a/libavsequencer/track.h b/libavsequencer/track.h index c1370ed..33095d9 100755 --- a/libavsequencer/track.h +++ b/libavsequencer/track.h @@ -34,12 +34,18 @@ * version bump. */
[...]
/** 0x20 - Set volume:
Again, is saying "0x20" in the comment relevant? If this value for the set volume command is format-dependent (i.e., it is only 0x20 in MOD but 0x55 in XM), this is misleading.
The demuxer/decoder will convert this to effect number 0x20 anyway, in MOD the set volume command is C (internally = 0x02). The goal is to have a unique command set independent from the origin file.
Data word consists of two 8-bit pairs named xx and yy where xx is the volume level (ranging either from @@ -756,7 +729,8 @@ typedef struct AVSequencerTrackEffect { means that the tremolo envelope values will be inverted. */ #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F
Wasn't you going to change these (and many other) to a enum? I remember you said you would change it soon, but being afraid of breaking other files that uses this header, but what code exactly does replacing
Yes, will do this tonight.
#define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE 0x2F #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX 0x1D
by
enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, }
breaks?
I access them like: if (flags& AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
Won't have change that to: if (flags& Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
then?
No, at least in my box the following compiles fine:
enum Whatever { AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F, AVSEQ_TRACK_EFFECT_CMD_STOP_FX = 0x1D, };
int f(int flags) { if (flags & AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE) return 1; else return 0; }
Finally, wasn't you going to remove all the defines for the default values by creating a function that init them? Can you send a new patch after applying these two major changes (ie, using enum instead of defines and avoiding the defines for the default values)? Ideally, you should send a new patch after all the feedback was already taken into account... -Vitor
Vitor Sessak a écrit :
Finally, wasn't you going to remove all the defines for the default values by creating a function that init them? Can you send a new patch after applying these two major changes (ie, using enum instead of defines and avoiding the defines for the default values)? Ideally, you should send a new patch after all the feedback was already taken into account...
Of course! I wanted to replace the default values when I actually wrote these functions, which I didn't had time to do yet. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 11:55 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:11 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:46 PM, Sebastian Vater wrote:
[...]
For tonight, I hope that's enough, I'm falling just to bed...hope didn't write any bullshit because if being tired. :-/
Sure, today saw a nice progress in reviewing! Have a very good night! -Vitor
Vitor Sessak a écrit :
On 07/13/2010 11:55 PM, Sebastian Vater wrote:
For tonight, I hope that's enough, I'm falling just to bed...hope didn't write any bullshit because if being tired. :-/
Sure, today saw a nice progress in reviewing! Have a very good night!
Thank you very much, I wish you all a wonderful night and dreams, too! Your comments really caused me brainstorming that, and I see the point, there's a lot of improvement to be done. ;-) -- Best regards, :-) Basty/CDGS (-:
participants (6)
-
Martin Storsjö -
Michael Niedermayer -
Ronald S. Bultje -
Sebastian Vater -
Stefano Sabatini -
Vitor Sessak