[soc] libavsequencer [PATCH 03/08] Order list public API header file.
-- Best regards, :-) Basty/CDGS (-:
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
+ /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
It is weird all these fields have default values that are the lowest/highest possible value of the data type. A file can not have a volume higher than default? -Vitor
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume << 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
+ /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
It is weird all these fields have default values that are the lowest/highest possible value of the data type. A file can not have a volume higher than default?
As you recommended in song.h I will most likely remove this default stuff completely and use your recommended approach by simply declaring a fill with defaults function. -- Best regards, :-) Basty/CDGS (-:
On 07/10/2010 08:02 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume<< 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
Why don't you have just a single var for (volume << 8) + subvolume? -Vitor
Vitor Sessak a écrit :
On 07/10/2010 08:02 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume<< 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
Why don't you have just a single var for (volume << 8) + subvolume?
I already thought quite a long time about this, long before starting GSoC. But then I noticed that I mostly access only volume without sub-volume. Sub-volume is practically only used for the volume slide commands, etc. For the rest, I just access the 8-bit volume value. Maybe an union would be an alternative idea? But that probably looks more ugly can current implementation. Anyway there is no actual reason, why the sub-volume slides couldn't take in to account into actual output (it would make volume depth 16-bit and therefore much smoother like switching from 8-bit sample to 16-bit), since sub-slides are an unique feature of TuComposer so far which I invented because TuComposer uses 16-bit data for the effects instead of just 8-bit ones. But it would require a lot of changes (esp. in the playback engine), which I would prefer doing after GSoC when we have everything ready so that FFmpeg can actually play and convert mod files already. Changing the internal logic of the playback engine will require testing it, which is not possible at the current state, I just want to get it working at all, i.e. at least having the same audio output as TuComposer had. I could however, do the changes in TuComposer and try that out, but that would effectively double the amount of time required, and thus slow down the development in FFmpeg by the factor of 2, which I really don't want! Remember, I already have decided to drop the old TuComposer completely when the FFmpeg port works at least with the quality the original had. So changing code in original TuComposer is just plain silly. There is no reason anymore to have that as a separate project. The name TuComposer will still remain to honor history though when everything is in FFmpeg in the wxTuComposer project coming then, which will be the first GUI for it using wxWidgets, but using FFmpeg instead. ;-) This will also the point when I implement the remaining missing features of TuComposer. When all this is done, we can change such things without being worried to break anything without knowing what was wrong. Another point though, it seems that my documentation regarding volume / sub-volume is misunderstandable regarding this, so I have for now at least to change the documentation. Any good idea? -- Best regards, :-) Basty/CDGS (-:
On 07/10/2010 11:24 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/10/2010 08:02 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume<< 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
Why don't you have just a single var for (volume<< 8) + subvolume?
I already thought quite a long time about this, long before starting GSoC. But then I noticed that I mostly access only volume without sub-volume. Sub-volume is practically only used for the volume slide commands, etc.
For the rest, I just access the 8-bit volume value. Maybe an union would be an alternative idea? But that probably looks more ugly can current implementation.
Anyway there is no actual reason, why the sub-volume slides couldn't take in to account into actual output (it would make volume depth 16-bit and therefore much smoother like switching from 8-bit sample to 16-bit), since sub-slides are an unique feature of TuComposer so far which I invented because TuComposer uses 16-bit data for the effects instead of just 8-bit ones.
But it would require a lot of changes (esp. in the playback engine), which I would prefer doing after GSoC when we have everything ready so that FFmpeg can actually play and convert mod files already.
So subvolume is not actually needed to describe a song?
Changing the internal logic of the playback engine will require testing it, which is not possible at the current state, I just want to get it working at all, i.e. at least having the same audio output as TuComposer had.
I agree of letting it as-is for now. -Vitor
Vitor Sessak a écrit :
On 07/10/2010 11:24 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/10/2010 08:02 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote:
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..ac48db2 --- /dev/null +++ b/libavsequencer/order.h +typedef struct AVSequencerOrderList { + /** Integer indexed tree root of order list data used by this + channel with AVTreeNode->elem being an AVSequencerOrderData. */ + AVTreeNode *order_data; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; +#define AVSEQ_ORDER_LIST_VOLUME 255 + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume<< 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
Why don't you have just a single var for (volume<< 8) + subvolume?
I already thought quite a long time about this, long before starting GSoC. But then I noticed that I mostly access only volume without sub-volume. Sub-volume is practically only used for the volume slide commands, etc.
For the rest, I just access the 8-bit volume value. Maybe an union would be an alternative idea? But that probably looks more ugly can current implementation.
Anyway there is no actual reason, why the sub-volume slides couldn't take in to account into actual output (it would make volume depth 16-bit and therefore much smoother like switching from 8-bit sample to 16-bit), since sub-slides are an unique feature of TuComposer so far which I invented because TuComposer uses 16-bit data for the effects instead of just 8-bit ones.
But it would require a lot of changes (esp. in the playback engine), which I would prefer doing after GSoC when we have everything ready so that FFmpeg can actually play and convert mod files already.
So subvolume is not actually needed to describe a song?
Changing the internal logic of the playback engine will require testing it, which is not possible at the current state, I just want to get it working at all, i.e. at least having the same audio output as TuComposer had.
I agree of letting it as-is for now.
New patch attached. -- Best regards, :-) Basty/CDGS (-:
On 07/11/2010 10:05 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/10/2010 11:24 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/10/2010 08:02 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/07/2010 10:47 PM, Sebastian Vater wrote: > diff --git a/libavsequencer/order.h b/libavsequencer/order.h > new file mode 100644 > index 0000000..ac48db2 > --- /dev/null > +++ b/libavsequencer/order.h > +typedef struct AVSequencerOrderList { > + /** Integer indexed tree root of order list data used by this > + channel with AVTreeNode->elem being an > AVSequencerOrderData. */ > + AVTreeNode *order_data; > + > + /** Number of order list data entries to use for this > channel. */ > + uint16_t length; > + > + /** Repeat start order list data number for this channel. */ > + uint16_t rep_start; > + > + /** Volume level for this channel (defaults to 255). */ > + uint8_t volume; > +#define AVSEQ_ORDER_LIST_VOLUME 255 > + > + /** Sub-volume level for this channel. This is basically > channel > + volume divided by 256, but the sub-volume doesn't account > + into actual mixer output (defaults 0). */ > + uint8_t sub_volume; > +#define AVSEQ_ORDER_LIST_SUB_VOLUME 0
Dividing an uint_8 by 256? Does not give much information...
Dividing (volume<< 8) + sub_volume by 256. I mean with that the sub_volume is internally used for accuracy sliding but not outputted to the mixing engine.
Why don't you have just a single var for (volume<< 8) + subvolume?
I already thought quite a long time about this, long before starting GSoC. But then I noticed that I mostly access only volume without sub-volume. Sub-volume is practically only used for the volume slide commands, etc.
For the rest, I just access the 8-bit volume value. Maybe an union would be an alternative idea? But that probably looks more ugly can current implementation.
Anyway there is no actual reason, why the sub-volume slides couldn't take in to account into actual output (it would make volume depth 16-bit and therefore much smoother like switching from 8-bit sample to 16-bit), since sub-slides are an unique feature of TuComposer so far which I invented because TuComposer uses 16-bit data for the effects instead of just 8-bit ones.
But it would require a lot of changes (esp. in the playback engine), which I would prefer doing after GSoC when we have everything ready so that FFmpeg can actually play and convert mod files already.
So subvolume is not actually needed to describe a song?
Changing the internal logic of the playback engine will require testing it, which is not possible at the current state, I just want to get it working at all, i.e. at least having the same audio output as TuComposer had.
I agree of letting it as-is for now.
New patch attached.
/* * AVSequencer order list and data 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_ORDER_H #define AVSEQUENCER_ORDER_H
#include "libavsequencer/track.h"
/** * Song order list structure, This structure is actually for one channel * and therefore actually pointed as an array with size of number of * host channels.
Wouldn't it be better named AVSequencerChannelData?
* 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 AVSequencerOrderList { /** Array of pointers containing all order list data used by this channel. */ AVSequencerOrderData **order_data;
Please replace in the comment "order list data" for a brief description of what it means. And BTW, is this an "order _list_ data"?
/** Number of order list data used for this channel. */ uint16_t orders;
/** Number of order list data entries to use for this channel. */ uint16_t length;
/** Repeat start order list data number for this channel. */ uint16_t rep_start;
/** Volume level for this channel (defaults to 255). */ uint8_t volume; #define AVSEQ_ORDER_LIST_VOLUME 255
/** Sub-volume level for this channel. This is basically channel volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
/** Stereo track panning level for this channel (defaults to -128 = central stereo track panning). */ int8_t track_panning; #define AVSEQ_ORDER_LIST_TRACK_PAN -128
/** Stereo track sub-panning level for this channel. This is basically track panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t track_sub_panning; #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN -128
/** Stereo panning level for this channel (defaults to -128 = central stereo panning). */ int8_t channel_panning; #define AVSEQ_ORDER_LIST_PANNING -128
/** Stereo sub-panning level for this channel. This is basically channel panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t channel_sub_panning; #define AVSEQ_ORDER_LIST_SUB_PANNING 0
/** Compatibility flags for playback. There are rare cases where order handling can not be mapped into internal playback engine and have to be handled specially. For each order list which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags;
I suppose this is not unused ATM?
/** Order list playback flags. Some sequencers feature surround panning or allow initial muting. which has to be taken care specially in the internal playback engine. Also sequencers differ in how they handle slides. */ uint8_t flags; #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND 0x01 ///< Initial channel surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND 0x02 ///< Initial track surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_MUTED 0x04 ///< Initial muted channel
} AVSequencerOrderList;
/** * Song order list data structure, this contains actual order list data. * 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 AVSequencerOrderData { /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack *track;
/** Next order list data pointer if seeking forward one frame. */ AVSequencerOrderData *next_pos;
/** Previous order list data pointer if seeking backward one frame. */ AVSequencerOrderData *prev_pos;
These do not look to belong to the BSS.
/** Number of row to jump to if forward seeking one frame. */ uint16_t next_row;
/** Number of row to jump to if backward seeking one frame. */ uint16_t prev_row;
/** Beginning row for this track. If this is a track synchronization point, the high byte is interpreted as the first track number to be synchronized with and the low byte as the second track number or for all channels when all 4 tracks are 0. */ uint16_t first_row;
/** Last row for this track. If this is a track synchronization point, the high byte is interpreted as the third track number to be synchronized with and the low byte as the fourth track number or for all channels when all 4 tracks are 0. If last row is set to 65535 in non synchronization mode, the last row is always taken from AVSequencerTrack. */ uint16_t last_row;
/** Order list data playback flags. Some sequencers feature special end markers or even different playback routes for different playback modules (one-shot and repeat mode playback), mark synchronization points or temporary change volume), which has to be taken care specially in the internal playback engine. */ uint8_t flags; #define AVSEQ_ORDER_DATA_FLAG_END_ORDER 0x01 ///< Order data indicates end of order #define AVSEQ_ORDER_DATA_FLAG_END_SONG 0x02 ///< Order data indicates end of whole song #define AVSEQ_ORDER_DATA_FLAG_NOT_IN_ONCE 0x04 ///< Order data will be skipped if you're playing in one-time mode #define AVSEQ_ORDER_DATA_FLAG_NOT_IN_REPEAT 0x08 ///< Order data will be skipped if you're playing in repeat mode #define AVSEQ_ORDER_DATA_FLAG_TRACK_SYNC 0x10 ///< Order data is a track synchronization point. #define AVSEQ_ORDER_DATA_FLAG_SET_VOLUME 0x20 ///< Order data takes advantage of the order list volume set
/** Relative note transpose for full track. Allows playing several tracks some half-tones up/down. */ int8_t transpose;
/** Instrument transpose. All instruments will be relatively mapped to this if this is non-zero. */ int16_t instr_transpose;
/** Tempo change or zero to skip tempo change. */ uint16_t tempo;
Cryptic comment. This is the timestamp where the tempo change? An index to a list a tempo changing structures? The instrument number that has a different tempo?
/** Played nesting level (GoSub command maximum nesting depth). */ uint16_t played;
Again, BSS?
/** Track volume (this overrides settings in AVSequencerTrack). To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME must be set in flags. */ uint8_t volume;
This is ugly, why not making it a signed int and summing it to AVSequencerTrack.volume no matter what? -Vitor
Vitor Sessak a écrit :
On 07/11/2010 10:05 PM, Sebastian Vater wrote:
/* * AVSequencer order list and data 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_ORDER_H #define AVSEQUENCER_ORDER_H
#include "libavsequencer/track.h"
/** * Song order list structure, This structure is actually for one channel * and therefore actually pointed as an array with size of number of * host channels.
Wouldn't it be better named AVSequencerChannelData?
It is only channel based if you import from a tracker which separates channels for each track, but most trackers don't do this, they have just one entry for all the channels, since they only know synchronized channels.
* 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 AVSequencerOrderList { /** Array of pointers containing all order list data used by this channel. */ AVSequencerOrderData **order_data;
Please replace in the comment "order list data" for a brief description of what it means. And BTW, is this an "order _list_ data"?
Actually it is the data of the order list, order_data contains all order list entries like AVSequencerTrackData contains all the rows of AVSequencerTrack.
/** Number of order list data used for this channel. */ uint16_t orders;
/** Number of order list data entries to use for this channel. */ uint16_t length;
/** Repeat start order list data number for this channel. */ uint16_t rep_start;
/** Volume level for this channel (defaults to 255). */ uint8_t volume; #define AVSEQ_ORDER_LIST_VOLUME 255
/** Sub-volume level for this channel. This is basically channel volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
/** Stereo track panning level for this channel (defaults to -128 = central stereo track panning). */ int8_t track_panning; #define AVSEQ_ORDER_LIST_TRACK_PAN -128
/** Stereo track sub-panning level for this channel. This is basically track panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t track_sub_panning; #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN -128
/** Stereo panning level for this channel (defaults to -128 = central stereo panning). */ int8_t channel_panning; #define AVSEQ_ORDER_LIST_PANNING -128
/** Stereo sub-panning level for this channel. This is basically channel panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t channel_sub_panning; #define AVSEQ_ORDER_LIST_SUB_PANNING 0
/** Compatibility flags for playback. There are rare cases where order handling can not be mapped into internal playback engine and have to be handled specially. For each order list which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags;
I suppose this is not unused ATM?
Fixed.
/** Order list playback flags. Some sequencers feature surround panning or allow initial muting. which has to be taken care specially in the internal playback engine. Also sequencers differ in how they handle slides. */ uint8_t flags; #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND 0x01 ///< Initial channel surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND 0x02 ///< Initial track surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_MUTED 0x04 ///< Initial muted channel
} AVSequencerOrderList;
/** * Song order list data structure, this contains actual order list data. * 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 AVSequencerOrderData { /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack *track;
/** Next order list data pointer if seeking forward one frame. */ AVSequencerOrderData *next_pos;
/** Previous order list data pointer if seeking backward one frame. */ AVSequencerOrderData *prev_pos;
These do not look to belong to the BSS.
They are used to allow the composer to define alternative order sequences in case of forward and backward seeking. These alternatives can pre-initialize note data in that case. This is a very rarely used feature, but avoids the problem that instruments are missing if you skip one channel. Example: We have a track playing a new instrument at row 60. If the user skips this track before arriving row 60, it will not be played at all, thus causing silence. Now consider the normal next row, would change data here or even has a complete empty row (but expecting to play a long looped instrument), in that case it would confuse the actual output. These pointers allow the composer to initialize that in case.
/** Tempo change or zero to skip tempo change. */ uint16_t tempo;
Cryptic comment. This is the timestamp where the tempo change? An index to a list a tempo changing structures? The instrument number that has a different tempo?
Fixed.
/** Played nesting level (GoSub command maximum nesting depth). */ uint16_t played;
Again, BSS?
Would require duplicating each track information in the player and therefore add a lot of unnecessary complexity.
/** Track volume (this overrides settings in AVSequencerTrack). To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME must be set in flags. */ uint8_t volume;
This is ugly, why not making it a signed int and summing it to AVSequencerTrack.volume no matter what?
This again would seriously break compatibility with almost any tracker. Trackers either set volume by track data XOR by order data, never both. It is not a volume transpose but a set volume command. For relative volume commands, there is set track volume which sets relative to instrument / sample volume. Anyway, volumes are scaled by multiply and divide, not by adding. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 09:49 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:05 PM, Sebastian Vater wrote:
/* * AVSequencer order list and data management * Copyright (c) 2010 Sebastian Vater<cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> * * 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_ORDER_H #define AVSEQUENCER_ORDER_H
#include "libavsequencer/track.h"
/** * Song order list structure, This structure is actually for one channel * and therefore actually pointed as an array with size of number of * host channels.
Wouldn't it be better named AVSequencerChannelData?
It is only channel based if you import from a tracker which separates channels for each track, but most trackers don't do this, they have just one entry for all the channels, since they only know synchronized channels.
* 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 AVSequencerOrderList { /** Array of pointers containing all order list data used by this channel. */ AVSequencerOrderData **order_data;
Please replace in the comment "order list data" for a brief description of what it means. And BTW, is this an "order _list_ data"?
Actually it is the data of the order list, order_data contains all order list entries like AVSequencerTrackData contains all the rows of AVSequencerTrack.
Is it the list of the rows in chronological order? Is it the list of notes in alphabetical order? Also, if I understand well, it is an array of linked lists. Why?
/** Number of order list data used for this channel. */ uint16_t orders;
/** Number of order list data entries to use for this channel. */ uint16_t length;
/** Repeat start order list data number for this channel. */ uint16_t rep_start;
/** Volume level for this channel (defaults to 255). */ uint8_t volume; #define AVSEQ_ORDER_LIST_VOLUME 255
/** Sub-volume level for this channel. This is basically channel volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
/** Stereo track panning level for this channel (defaults to -128 = central stereo track panning). */ int8_t track_panning; #define AVSEQ_ORDER_LIST_TRACK_PAN -128
/** Stereo track sub-panning level for this channel. This is basically track panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t track_sub_panning; #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN -128
/** Stereo panning level for this channel (defaults to -128 = central stereo panning). */ int8_t channel_panning; #define AVSEQ_ORDER_LIST_PANNING -128
/** Stereo sub-panning level for this channel. This is basically channel panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t channel_sub_panning; #define AVSEQ_ORDER_LIST_SUB_PANNING 0
/** Compatibility flags for playback. There are rare cases where order handling can not be mapped into internal playback engine and have to be handled specially. For each order list which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags;
I suppose this is not unused ATM?
Fixed.
/** Order list playback flags. Some sequencers feature surround panning or allow initial muting. which has to be taken care specially in the internal playback engine. Also sequencers differ in how they handle slides. */ uint8_t flags; #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND 0x01 ///< Initial channel surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND 0x02 ///< Initial track surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_MUTED 0x04 ///< Initial muted channel
} AVSequencerOrderList;
/** * Song order list data structure, this contains actual order list data. * 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 AVSequencerOrderData { /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack *track;
/** Next order list data pointer if seeking forward one frame. */ AVSequencerOrderData *next_pos;
/** Previous order list data pointer if seeking backward one frame. */ AVSequencerOrderData *prev_pos;
These do not look to belong to the BSS.
They are used to allow the composer to define alternative order sequences in case of forward and backward seeking. These alternatives can pre-initialize note data in that case. This is a very rarely used feature, but avoids the problem that instruments are missing if you skip one channel.
Example: We have a track playing a new instrument at row 60. If the user skips this track before arriving row 60, it will not be played at all, thus causing silence. Now consider the normal next row, would change data here or even has a complete empty row (but expecting to play a long looped instrument), in that case it would confuse the actual output. These pointers allow the composer to initialize that in case.
Is this about an human user seeking the file he is playing or a tracker formats that specify seeking commands?
/** Tempo change or zero to skip tempo change. */ uint16_t tempo;
Cryptic comment. This is the timestamp where the tempo change? An index to a list a tempo changing structures? The instrument number that has a different tempo?
Fixed.
/** Played nesting level (GoSub command maximum nesting depth). */ uint16_t played;
Again, BSS?
Would require duplicating each track information in the player and therefore add a lot of unnecessary complexity.
So that means that the player _writes_ to this struct? This seems not optimal design in a modularity point of view. IMHO, everything that is feeded to the player should be read-only and the player should only write in his own context structure.
/** Track volume (this overrides settings in AVSequencerTrack). To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME must be set in flags. */ uint8_t volume;
This is ugly, why not making it a signed int and summing it to AVSequencerTrack.volume no matter what?
This again would seriously break compatibility with almost any tracker. Trackers either set volume by track data XOR by order data, never both. It is not a volume transpose but a set volume command.
What is the difference between a track volume and a order data volume? If they are the same, since only one or the other is specified per file, why not using a single struct field for it, without caring where in the file it is defined (and the module loader should know where to look)? -Vitor
Vitor Sessak a écrit :
On 07/13/2010 09:49 PM, Sebastian Vater wrote:
Is it the list of the rows in chronological order? Is it the list of notes in alphabetical order? Also, if I understand well, it is an array of linked lists. Why?
Because the user can insert new rows, delete old ones, rearrange them when editing.
/** Number of order list data used for this channel. */ uint16_t orders;
/** Number of order list data entries to use for this channel. */ uint16_t length;
/** Repeat start order list data number for this channel. */ uint16_t rep_start;
/** Volume level for this channel (defaults to 255). */ uint8_t volume; #define AVSEQ_ORDER_LIST_VOLUME 255
/** Sub-volume level for this channel. This is basically channel volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
/** Stereo track panning level for this channel (defaults to -128 = central stereo track panning). */ int8_t track_panning; #define AVSEQ_ORDER_LIST_TRACK_PAN -128
/** Stereo track sub-panning level for this channel. This is basically track panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t track_sub_panning; #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN -128
/** Stereo panning level for this channel (defaults to -128 = central stereo panning). */ int8_t channel_panning; #define AVSEQ_ORDER_LIST_PANNING -128
/** Stereo sub-panning level for this channel. This is basically channel panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t channel_sub_panning; #define AVSEQ_ORDER_LIST_SUB_PANNING 0
/** Compatibility flags for playback. There are rare cases where order handling can not be mapped into internal playback engine and have to be handled specially. For each order list which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags;
I suppose this is not unused ATM?
Fixed.
/** Order list playback flags. Some sequencers feature surround panning or allow initial muting. which has to be taken care specially in the internal playback engine. Also sequencers differ in how they handle slides. */ uint8_t flags; #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND 0x01 ///< Initial channel surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND 0x02 ///< Initial track surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_MUTED 0x04 ///< Initial muted channel
} AVSequencerOrderList;
/** * Song order list data structure, this contains actual order list data. * 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 AVSequencerOrderData { /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack *track;
/** Next order list data pointer if seeking forward one frame. */ AVSequencerOrderData *next_pos;
/** Previous order list data pointer if seeking backward one frame. */ AVSequencerOrderData *prev_pos;
These do not look to belong to the BSS.
They are used to allow the composer to define alternative order sequences in case of forward and backward seeking. These alternatives can pre-initialize note data in that case. This is a very rarely used feature, but avoids the problem that instruments are missing if you skip one channel.
Example: We have a track playing a new instrument at row 60. If the user skips this track before arriving row 60, it will not be played at all, thus causing silence. Now consider the normal next row, would change data here or even has a complete empty row (but expecting to play a long looped instrument), in that case it would confuse the actual output. These pointers allow the composer to initialize that in case.
Is this about an human user seeking the file he is playing or a tracker formats that specify seeking commands?
The composer can specifiy alternative seek paths if he wants...of course, if there are formats which also support this feature, it can be done here as well.
/** Tempo change or zero to skip tempo change. */ uint16_t tempo;
Cryptic comment. This is the timestamp where the tempo change? An index to a list a tempo changing structures? The instrument number that has a different tempo?
Fixed.
/** Played nesting level (GoSub command maximum nesting depth). */ uint16_t played;
Again, BSS?
Would require duplicating each track information in the player and therefore add a lot of unnecessary complexity.
So that means that the player _writes_ to this struct? This seems not optimal design in a modularity point of view. IMHO, everything that is feeded to the player should be read-only and the player should only write in his own context structure.
This would duplicate the amount of work required, i.e. duplicating the whole order list structure. Do you think, this is worth the additional overhead? -- Best regards, :-) Basty/CDGS (-:
Vitor Sessak a écrit :
On 07/13/2010 09:49 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:05 PM, Sebastian Vater wrote:
/* * AVSequencer order list and data management * Copyright (c) 2010 Sebastian Vater<cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> * * 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_ORDER_H #define AVSEQUENCER_ORDER_H
#include "libavsequencer/track.h"
/** * Song order list structure, This structure is actually for one channel * and therefore actually pointed as an array with size of number of * host channels.
Wouldn't it be better named AVSequencerChannelData?
It is only channel based if you import from a tracker which separates channels for each track, but most trackers don't do this, they have just one entry for all the channels, since they only know synchronized channels.
* 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 AVSequencerOrderList { /** Array of pointers containing all order list data used by this channel. */ AVSequencerOrderData **order_data;
Please replace in the comment "order list data" for a brief description of what it means. And BTW, is this an "order _list_ data"?
Actually it is the data of the order list, order_data contains all order list entries like AVSequencerTrackData contains all the rows of AVSequencerTrack.
Is it the list of the rows in chronological order? Is it the list of notes in alphabetical order? Also, if I understand well, it is an array of linked lists. Why?
/** Number of order list data used for this channel. */ uint16_t orders;
/** Number of order list data entries to use for this channel. */ uint16_t length;
/** Repeat start order list data number for this channel. */ uint16_t rep_start;
/** Volume level for this channel (defaults to 255). */ uint8_t volume; #define AVSEQ_ORDER_LIST_VOLUME 255
/** Sub-volume level for this channel. This is basically channel volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
/** Stereo track panning level for this channel (defaults to -128 = central stereo track panning). */ int8_t track_panning; #define AVSEQ_ORDER_LIST_TRACK_PAN -128
/** Stereo track sub-panning level for this channel. This is basically track panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t track_sub_panning; #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN -128
/** Stereo panning level for this channel (defaults to -128 = central stereo panning). */ int8_t channel_panning; #define AVSEQ_ORDER_LIST_PANNING -128
/** Stereo sub-panning level for this channel. This is basically channel panning divided by 256, but the sub-panning doesn't account into actual mixer output (defaults 0). */ uint8_t channel_sub_panning; #define AVSEQ_ORDER_LIST_SUB_PANNING 0
/** Compatibility flags for playback. There are rare cases where order handling can not be mapped into internal playback engine and have to be handled specially. For each order list which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags;
I suppose this is not unused ATM?
Fixed.
/** Order list playback flags. Some sequencers feature surround panning or allow initial muting. which has to be taken care specially in the internal playback engine. Also sequencers differ in how they handle slides. */ uint8_t flags; #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND 0x01 ///< Initial channel surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND 0x02 ///< Initial track surround instead of stereo panning #define AVSEQ_ORDER_LIST_FLAG_MUTED 0x04 ///< Initial muted channel
} AVSequencerOrderList;
/** * Song order list data structure, this contains actual order list data. * 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 AVSequencerOrderData { /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack *track;
/** Next order list data pointer if seeking forward one frame. */ AVSequencerOrderData *next_pos;
/** Previous order list data pointer if seeking backward one frame. */ AVSequencerOrderData *prev_pos;
These do not look to belong to the BSS.
They are used to allow the composer to define alternative order sequences in case of forward and backward seeking. These alternatives can pre-initialize note data in that case. This is a very rarely used feature, but avoids the problem that instruments are missing if you skip one channel.
Example: We have a track playing a new instrument at row 60. If the user skips this track before arriving row 60, it will not be played at all, thus causing silence. Now consider the normal next row, would change data here or even has a complete empty row (but expecting to play a long looped instrument), in that case it would confuse the actual output. These pointers allow the composer to initialize that in case.
Is this about an human user seeking the file he is playing or a tracker formats that specify seeking commands?
/** Tempo change or zero to skip tempo change. */ uint16_t tempo;
Cryptic comment. This is the timestamp where the tempo change? An index to a list a tempo changing structures? The instrument number that has a different tempo?
Fixed.
/** Played nesting level (GoSub command maximum nesting depth). */ uint16_t played;
Again, BSS?
Would require duplicating each track information in the player and therefore add a lot of unnecessary complexity.
So that means that the player _writes_ to this struct? This seems not optimal design in a modularity point of view. IMHO, everything that is feeded to the player should be read-only and the player should only write in his own context structure.
/** Track volume (this overrides settings in AVSequencerTrack). To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME must be set in flags. */ uint8_t volume;
This is ugly, why not making it a signed int and summing it to AVSequencerTrack.volume no matter what?
This again would seriously break compatibility with almost any tracker. Trackers either set volume by track data XOR by order data, never both. It is not a volume transpose but a set volume command.
What is the difference between a track volume and a order data volume? If they are the same, since only one or the other is specified per file, why not using a single struct field for it, without caring where in the file it is defined (and the module loader should know where to look)?
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 order.[ch] part of the BSS to review. This version compiles perfectly. -- Best regards, :-) Basty/CDGS (-:
On 08/07/2010 09:44 PM, Sebastian Vater wrote: [...]
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 order.[ch] part of the BSS to review.
This version compiles perfectly.
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..70bcf0b --- /dev/null +++ b/libavsequencer/order.h @@ -0,0 +1,193 @@
[...]
+typedef struct AVSequencerOrderData { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original order entry file name, order + * entry name, artist and comment. */ + AVMetadata *metadata;
+ /** AVSequencerTrack pointer to track which should be played. */ + AVSequencerTrack *track;
If there is one and only one AVSequencerTrack per AVSequencerOrderData, why not having it directly as part of the struct, i.e.: /** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack track; ?
+ /** Next order list data pointer if seeking forward one frame. */ + struct AVSequencerOrderData *next_pos; + + /** Previous order list data pointer if seeking backward one + frame. */ + struct AVSequencerOrderData *prev_pos; + + /** Number of row to jump to if forward seeking one frame. */ + uint16_t next_row; + + /** Number of row to jump to if backward seeking one frame. */ + uint16_t prev_row; + + /** Beginning row for this track. If this is a track synchronization + point, the high byte is interpreted as the first track number + to be synchronized with and the low byte as the second track + number or for all channels when all 4 tracks are 0. */ + uint16_t first_row; + + /** Last row for this track. If this is a track synchronization + point, the high byte is interpreted as the third track number + to be synchronized with and the low byte as the fourth track + number or for all channels when all 4 tracks are 0. + If last row is set to 65535 in non synchronization mode, + the last row is always taken from AVSequencerTrack. */ + uint16_t last_row;
+ /** Order list data playback flags. Some sequencers feature + special end markers or even different playback routes for + different playback modules (one-shot and repeat mode + playback), mark synchronization points or temporary + change volume), which has to be taken care specially + in the internal playback engine. */ + uint8_t flags;
enum...
+ /** Relative note transpose for full track. Allows playing several + tracks some half-tones up/down. */ + int8_t transpose;
Comment unclear. Is this a flag? What does it means if transpose == -23?
+ /** Instrument transpose. All instruments will be relatively + mapped to this if this is non-zero. */ + int16_t instr_transpose;
Why an int16_t for 0/1?
+ /** Tempo change or zero to skip tempo change. A tempo value of + zero would be zero, since that would mean literally execute + unlimited rows and tracks in just one tick. */ + uint16_t tempo;
+ /** Played nesting level (GoSub command maximum nesting depth). */ + uint16_t played; + + /** Track volume (this overrides settings in AVSequencerTrack). + To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME + must be set in flags. This allows have a basic default track + volume by still allowing to override the track volume in case + the track is used multiple times, e.g. for creating echoes. */ + uint8_t volume; + + /** Track sub-volume. This is basically track volume + divided by 256, but the sub-volume doesn't account + into actual mixer output (this overrides AVSequencerTrack). */ + uint8_t sub_volume; +} AVSequencerOrderData; + +/** AVSequencerOrderList->flags bitfield. */ +enum AVSequencerOrderListFlags {
+ AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND = 0x01, ///< Initial channel surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND = 0x02, ///< Initial track surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_MUTED = 0x04, ///< Initial muted channel +};
Did you mean "initially"?
+/** + * Song order list structure, This structure is actually for one channel + * and therefore actually pointed as an array with size of number of + * host channels. + * 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 AVSequencerOrderList { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original order list file name, order + * list name, artist and comment. */ + AVMetadata *metadata; + + /** Array (of size orders) of pointers containing all order list + data used by this channel. */ + AVSequencerOrderData **order_data; + + /** Number of order list data used for this channel. */ + uint16_t orders; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; + + /** Stereo track panning level for this channel (defaults to + -128 = central stereo track panning). */ + int8_t track_panning; + + /** Stereo track sub-panning level for this channel. This is + basically track panning divided by 256, but the sub-panning + doesn't account into actual mixer output (defaults 0). */ + uint8_t track_sub_panning; + + /** Stereo panning level for this channel (defaults to + -128 = central stereo panning). */ + int8_t channel_panning; + + /** Stereo sub-panning level for this channel. This is + basically channel panning divided by 256, but the sub-panning + doesn't account into actual mixer output (defaults 0). */ + uint8_t channel_sub_panning;
+ /** Order list playback flags. Some sequencers feature + surround panning or allow initial muting. which has to + be taken care specially in the internal playback engine. + Also sequencers differ in how they handle slides. */ + uint8_t flags;
enum....
+ +#endif /* AVSEQUENCER_ORDER_H */
Vitor Sessak a écrit :
On 08/07/2010 09:44 PM, Sebastian Vater wrote:
[...]
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 order.[ch] part of the BSS to review.
This version compiles perfectly.
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..70bcf0b --- /dev/null +++ b/libavsequencer/order.h @@ -0,0 +1,193 @@
[...]
+typedef struct AVSequencerOrderData { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original order entry file name, order + * entry name, artist and comment. */ + AVMetadata *metadata;
+ /** AVSequencerTrack pointer to track which should be played. */ + AVSequencerTrack *track;
If there is one and only one AVSequencerTrack per AVSequencerOrderData, why not having it directly as part of the struct, i.e.:
/** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack track;
?
Would be possible though, but remember, the track stuff is supposed to be editable and it's easier to just to replace the pointer instead of copying the data all over it. Also it requires way less memory usage, please note that number of order data has to be multiplied by number of channels, i.e. for a 64 channel sub-song we have 64 * (order_list->orders) entries, which can quite a lot (a lot of modules have order_list->orders beyond 100).
+ /** Next order list data pointer if seeking forward one frame. */ + struct AVSequencerOrderData *next_pos; + + /** Previous order list data pointer if seeking backward one + frame. */ + struct AVSequencerOrderData *prev_pos; + + /** Number of row to jump to if forward seeking one frame. */ + uint16_t next_row; + + /** Number of row to jump to if backward seeking one frame. */ + uint16_t prev_row; + + /** Beginning row for this track. If this is a track synchronization + point, the high byte is interpreted as the first track number + to be synchronized with and the low byte as the second track + number or for all channels when all 4 tracks are 0. */ + uint16_t first_row; + + /** Last row for this track. If this is a track synchronization + point, the high byte is interpreted as the third track number + to be synchronized with and the low byte as the fourth track + number or for all channels when all 4 tracks are 0. + If last row is set to 65535 in non synchronization mode, + the last row is always taken from AVSequencerTrack. */ + uint16_t last_row;
+ /** Order list data playback flags. Some sequencers feature + special end markers or even different playback routes for + different playback modules (one-shot and repeat mode + playback), mark synchronization points or temporary + change volume), which has to be taken care specially + in the internal playback engine. */ + uint8_t flags;
enum...
Hmm, what you mean with this, it is already enum, right?
+ /** Relative note transpose for full track. Allows playing several + tracks some half-tones up/down. */ + int8_t transpose;
Comment unclear. Is this a flag? What does it means if transpose == -23?
It means to adjust -23 halftones to each track data instrument / note pair encountered. Let's say you have in the pattern: C-5 01 .. ... Since -24 would be 2 octaves back (i.e. C-3), -23 would playback as: C#3 01 .. ... It's simply globally applied to the whole track which is used by this order entry. This is, e.g. a feature heavily used by FutureComposer.
+ /** Instrument transpose. All instruments will be relatively + mapped to this if this is non-zero. */ + int16_t instr_transpose;
Why an int16_t for 0/1?
This is not a 0/1, this means adding instr_transpose to the instrument value. Considering the example above: C-5 01 .. ... and instr_transpose = 4 then it will play: C-5 05 .. ... i.e. instrument number 5 instead of 1. Again, this is heavily used by FutureComposer.
+ /** Tempo change or zero to skip tempo change. A tempo value of + zero would be zero, since that would mean literally execute + unlimited rows and tracks in just one tick. */ + uint16_t tempo;
+ /** Played nesting level (GoSub command maximum nesting depth). */ + uint16_t played; + + /** Track volume (this overrides settings in AVSequencerTrack). + To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME + must be set in flags. This allows have a basic default track + volume by still allowing to override the track volume in case + the track is used multiple times, e.g. for creating echoes. */ + uint8_t volume; + + /** Track sub-volume. This is basically track volume + divided by 256, but the sub-volume doesn't account + into actual mixer output (this overrides AVSequencerTrack). */ + uint8_t sub_volume; +} AVSequencerOrderData; + +/** AVSequencerOrderList->flags bitfield. */ +enum AVSequencerOrderListFlags {
+ AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND = 0x01, ///< Initial channel surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND = 0x02, ///< Initial track surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_MUTED = 0x04, ///< Initial muted channel +};
Did you mean "initially"?
More like: Use initial ...
+/** + * Song order list structure, This structure is actually for one channel + * and therefore actually pointed as an array with size of number of + * host channels. + * 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 AVSequencerOrderList { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original order list file name, order + * list name, artist and comment. */ + AVMetadata *metadata; + + /** Array (of size orders) of pointers containing all order list + data used by this channel. */ + AVSequencerOrderData **order_data; + + /** Number of order list data used for this channel. */ + uint16_t orders; + + /** Number of order list data entries to use for this channel. */ + uint16_t length; + + /** Repeat start order list data number for this channel. */ + uint16_t rep_start; + + /** Volume level for this channel (defaults to 255). */ + uint8_t volume; + + /** Sub-volume level for this channel. This is basically channel + volume divided by 256, but the sub-volume doesn't account + into actual mixer output (defaults 0). */ + uint8_t sub_volume; + + /** Stereo track panning level for this channel (defaults to + -128 = central stereo track panning). */ + int8_t track_panning; + + /** Stereo track sub-panning level for this channel. This is + basically track panning divided by 256, but the sub-panning + doesn't account into actual mixer output (defaults 0). */ + uint8_t track_sub_panning; + + /** Stereo panning level for this channel (defaults to + -128 = central stereo panning). */ + int8_t channel_panning; + + /** Stereo sub-panning level for this channel. This is + basically channel panning divided by 256, but the sub-panning + doesn't account into actual mixer output (defaults 0). */ + uint8_t channel_sub_panning;
+ /** Order list playback flags. Some sequencers feature + surround panning or allow initial muting. which has to + be taken care specially in the internal playback engine. + Also sequencers differ in how they handle slides. */ + uint8_t flags;
enum....
Again what you mean with enum here? -- Best regards, :-) Basty/CDGS (-:
On 08/13/2010 10:19 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:44 PM, Sebastian Vater wrote:
[...]
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 order.[ch] part of the BSS to review.
This version compiles perfectly.
diff --git a/libavsequencer/order.h b/libavsequencer/order.h new file mode 100644 index 0000000..70bcf0b --- /dev/null +++ b/libavsequencer/order.h @@ -0,0 +1,193 @@
[...]
+typedef struct AVSequencerOrderData { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original order entry file name, order + * entry name, artist and comment. */ + AVMetadata *metadata;
+ /** AVSequencerTrack pointer to track which should be played. */ + AVSequencerTrack *track;
If there is one and only one AVSequencerTrack per AVSequencerOrderData, why not having it directly as part of the struct, i.e.:
/** AVSequencerTrack pointer to track which should be played. */ AVSequencerTrack track;
?
Would be possible though, but remember, the track stuff is supposed to be editable and it's easier to just to replace the pointer instead of copying the data all over it.
Also it requires way less memory usage, please note that number of order data has to be multiplied by number of channels, i.e. for a 64 channel sub-song we have 64 * (order_list->orders) entries, which can quite a lot (a lot of modules have order_list->orders beyond 100).
so its fine, duplicating memory is evil.
+ /** Next order list data pointer if seeking forward one frame. */ + struct AVSequencerOrderData *next_pos; + + /** Previous order list data pointer if seeking backward one + frame. */ + struct AVSequencerOrderData *prev_pos; + + /** Number of row to jump to if forward seeking one frame. */ + uint16_t next_row; + + /** Number of row to jump to if backward seeking one frame. */ + uint16_t prev_row; + + /** Beginning row for this track. If this is a track synchronization + point, the high byte is interpreted as the first track number + to be synchronized with and the low byte as the second track + number or for all channels when all 4 tracks are 0. */ + uint16_t first_row; + + /** Last row for this track. If this is a track synchronization + point, the high byte is interpreted as the third track number + to be synchronized with and the low byte as the fourth track + number or for all channels when all 4 tracks are 0. + If last row is set to 65535 in non synchronization mode, + the last row is always taken from AVSequencerTrack. */ + uint16_t last_row;
+ /** Order list data playback flags. Some sequencers feature + special end markers or even different playback routes for + different playback modules (one-shot and repeat mode + playback), mark synchronization points or temporary + change volume), which has to be taken care specially + in the internal playback engine. */ + uint8_t flags;
enum...
Hmm, what you mean with this, it is already enum, right?
+ /** Relative note transpose for full track. Allows playing several + tracks some half-tones up/down. */ + int8_t transpose;
Comment unclear. Is this a flag? What does it means if transpose == -23?
It means to adjust -23 halftones to each track data instrument / note pair encountered. Let's say you have in the pattern: C-5 01 .. ...
So it is really missing in the comment to say this quantity is expressed in halftone units.
Since -24 would be 2 octaves back (i.e. C-3), -23 would playback as: C#3 01 .. ...
It's simply globally applied to the whole track which is used by this order entry.
This is, e.g. a feature heavily used by FutureComposer.
+ /** Instrument transpose. All instruments will be relatively + mapped to this if this is non-zero. */ + int16_t instr_transpose;
Why an int16_t for 0/1?
This is not a 0/1,
Again, missing unit. Maybe something like "Number of half-tones to transpose the instrument." or something like that.
this means adding instr_transpose to the instrument value. Considering the example above: C-5 01 .. ...
and instr_transpose = 4 then it will play: C-5 05 .. ...
i.e. instrument number 5 instead of 1.
Again, this is heavily used by FutureComposer.
+ /** Tempo change or zero to skip tempo change. A tempo value of + zero would be zero, since that would mean literally execute + unlimited rows and tracks in just one tick. */ + uint16_t tempo;
+ /** Played nesting level (GoSub command maximum nesting depth). */ + uint16_t played; + + /** Track volume (this overrides settings in AVSequencerTrack). + To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME + must be set in flags. This allows have a basic default track + volume by still allowing to override the track volume in case + the track is used multiple times, e.g. for creating echoes. */ + uint8_t volume; + + /** Track sub-volume. This is basically track volume + divided by 256, but the sub-volume doesn't account + into actual mixer output (this overrides AVSequencerTrack). */ + uint8_t sub_volume; +} AVSequencerOrderData; + +/** AVSequencerOrderList->flags bitfield. */ +enum AVSequencerOrderListFlags {
+ AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND = 0x01, ///< Initial channel surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND = 0x02, ///< Initial track surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_MUTED = 0x04, ///< Initial muted channel +};
Did you mean "initially"?
More like: Use initial ...
indeed, "initial" is better. -Vitor
Vitor Sessak a écrit :
On 08/13/2010 10:19 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:44 PM, Sebastian Vater wrote:
[...]
+ /** Next order list data pointer if seeking forward one frame. */ + struct AVSequencerOrderData *next_pos; + + /** Previous order list data pointer if seeking backward one + frame. */ + struct AVSequencerOrderData *prev_pos; + + /** Number of row to jump to if forward seeking one frame. */ + uint16_t next_row; + + /** Number of row to jump to if backward seeking one frame. */ + uint16_t prev_row; + + /** Beginning row for this track. If this is a track synchronization + point, the high byte is interpreted as the first track number + to be synchronized with and the low byte as the second track + number or for all channels when all 4 tracks are 0. */ + uint16_t first_row; + + /** Last row for this track. If this is a track synchronization + point, the high byte is interpreted as the third track number + to be synchronized with and the low byte as the fourth track + number or for all channels when all 4 tracks are 0. + If last row is set to 65535 in non synchronization mode, + the last row is always taken from AVSequencerTrack. */ + uint16_t last_row;
+ /** Order list data playback flags. Some sequencers feature + special end markers or even different playback routes for + different playback modules (one-shot and repeat mode + playback), mark synchronization points or temporary + change volume), which has to be taken care specially + in the internal playback engine. */ + uint8_t flags;
enum...
Hmm, what you mean with this, it is already enum, right?
+ /** Relative note transpose for full track. Allows playing several + tracks some half-tones up/down. */ + int8_t transpose;
Comment unclear. Is this a flag? What does it means if transpose == -23?
It means to adjust -23 halftones to each track data instrument / note pair encountered. Let's say you have in the pattern: C-5 01 .. ...
So it is really missing in the comment to say this quantity is expressed in halftone units.
Ehm, this is in the comments: Allows playing several tracks some half-tones up/down. This actually also is the definition of the word transpose in music. Anyway I fixed the comment a bit.
Since -24 would be 2 octaves back (i.e. C-3), -23 would playback as: C#3 01 .. ...
It's simply globally applied to the whole track which is used by this order entry.
This is, e.g. a feature heavily used by FutureComposer.
+ /** Instrument transpose. All instruments will be relatively + mapped to this if this is non-zero. */ + int16_t instr_transpose;
Why an int16_t for 0/1?
This is not a 0/1,
Again, missing unit. Maybe something like "Number of half-tones to transpose the instrument." or something like that.
This is number of instrument value to add relatively. Fixed!
this means adding instr_transpose to the instrument value. Considering the example above: C-5 01 .. ...
and instr_transpose = 4 then it will play: C-5 05 .. ...
i.e. instrument number 5 instead of 1.
Again, this is heavily used by FutureComposer.
+ /** Tempo change or zero to skip tempo change. A tempo value of + zero would be zero, since that would mean literally execute + unlimited rows and tracks in just one tick. */ + uint16_t tempo;
+ /** Played nesting level (GoSub command maximum nesting depth). */ + uint16_t played; + + /** Track volume (this overrides settings in AVSequencerTrack). + To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME + must be set in flags. This allows have a basic default track + volume by still allowing to override the track volume in case + the track is used multiple times, e.g. for creating echoes. */ + uint8_t volume; + + /** Track sub-volume. This is basically track volume + divided by 256, but the sub-volume doesn't account + into actual mixer output (this overrides AVSequencerTrack). */ + uint8_t sub_volume; +} AVSequencerOrderData; + +/** AVSequencerOrderList->flags bitfield. */ +enum AVSequencerOrderListFlags {
+ AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND = 0x01, ///< Initial channel surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND = 0x02, ///< Initial track surround instead of stereo panning + AVSEQ_ORDER_LIST_FLAG_MUTED = 0x04, ///< Initial muted channel +};
Did you mean "initially"?
More like: Use initial ...
indeed, "initial" is better.
Fixed. -- Best regards, :-) Basty/CDGS (-:
participants (2)
-
Sebastian Vater -
Vitor Sessak