[soc] libavsequencer [PATCH 04/08] Track / pattern handling public API header file.
-- Best regards, :-) Basty/CDGS (-:
On Wed, Jul 07, 2010 at 10:47:43PM +0200, Sebastian Vater wrote:
--
Best regards, :-) Basty/CDGS (-:
track.h | 1777 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 1777 insertions(+) da558dab4d987632cce29e9ac8ec816cd85f0ab0 track.h.patch
i guess alot of these #defines could be enums and then the correct types could be used instead of int*_t [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
Michael Niedermayer a écrit :
i guess alot of these #defines could be enums and then the correct types could be used instead of int*_t
[...]
Hi Michael! I will change them the next days, this will incorporate a lot of changes to player.c also. Anyway, I have updated track.h, so new patch attached anyway. -- Best regards, :-) Basty/CDGS (-:
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
Michael Niedermayer a écrit :
i guess alot of these #defines could be enums and then the correct types could be used instead of int*_t
[...]
Hi Michael! I will change them the next days, this will incorporate a lot of changes to player.c also. Anyway, I have updated track.h, so new patch attached anyway.
/* * AVSequencer track and pattern 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_TRACK_H #define AVSEQUENCER_TRACK_H
#include "libavformat/avformat.h" #include "libavsequencer/avsequencer.h"
/** * Song track data structure, This structure is actually for one row * and therefore actually pointed as an array with the amount of * rows of the whole track. * 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 AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
/** Note to be played (see defines below, n is octave number). */ int8_t note; /** C-n */ #define AVSEQ_TRACK_DATA_NOTE 1 #define AVSEQ_TRACK_DATA_NOTE_C 1 #define AVSEQ_TRACK_DATA_NOTE_MIN AVSEQ_TRACK_DATA_NOTE_MIN
/** C#n = Dbn */ #define AVSEQ_TRACK_DATA_NOTE_C_SHARP 2
/** D-n */ #define AVSEQ_TRACK_DATA_NOTE_D 3
/** D#n = Ebn */ #define AVSEQ_TRACK_DATA_NOTE_D_SHARP 4
/** E-n */ #define AVSEQ_TRACK_DATA_NOTE_E 5
/** F-n */ #define AVSEQ_TRACK_DATA_NOTE_F 6
/** F#n = Gbn */ #define AVSEQ_TRACK_DATA_NOTE_F_SHARP 7
/** G-n */ #define AVSEQ_TRACK_DATA_NOTE_G 8
/** G#n = Abn */ #define AVSEQ_TRACK_DATA_NOTE_G_SHARP 9
/** A-n */ #define AVSEQ_TRACK_DATA_NOTE_A 10
/** A#n = Bbn */ #define AVSEQ_TRACK_DATA_NOTE_A_SHARP 11
/** B-n = H-n */ #define AVSEQ_TRACK_DATA_NOTE_B 12 #define AVSEQ_TRACK_DATA_NOTE_H FF_AVSEQ_TRACK_DATA_NOTE_B #define AVSEQ_TRACK_DATA_NOTE_MAX FF_AVSEQ_TRACK_DATA_NOTE_B
/** --- = first special empty note */ #define AVSEQ_TRACK_DATA_NOTE_SPECIAL 0 #define AVSEQ_TRACK_DATA_NOTE_NONE FF_AVSEQ_TRACK_DATA_NOTE_SPECIAL
/** ^^^ = note kill */ #define AVSEQ_TRACK_DATA_NOTE_KILL -1
/** ^^- = note off */ #define AVSEQ_TRACK_DATA_NOTE_OFF -2
/** === = keyoff note */ #define AVSEQ_TRACK_DATA_NOTE_KEYOFF -3
/** -|- = hold delay */ #define AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY -4
/** -\- = note fade */ #define AVSEQ_TRACK_DATA_NOTE_FADE -5
/** END = pattern end marker */ #define AVSEQ_TRACK_DATA_NOTE_END -16
/** Number of instrument to be played or 0 to take previous one. */ uint16_t instrument;
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do if (instrument == previous_instrument) {...}
/** C-4 = default tone */ #define AVSEQ_TRACK_DATA_TONE ((FF_AVSEQ_TRACK_DATA_OCTAVE << 8) | FF_AVSEQ_TRACK_DATA_NOTE)
/** Number of effects used by this track. */ uint16_t effects; } AVSequencerTrackData;
/** * Song track 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 AVSequencerTrack { /** Metadata information: Original track file name, track title, track message, track artist, track album, track begin and finish date of composition and comment. */ AVMetadata *metadata;
/** AVSequencerTrackData pointer to array of track data. */ AVSequencerTrackData *data;
This naming bothers me a little. If AVSequencerTrackData contains the track data, what does AVSequencerTrack contains?
/** Last valid row of track (defaults to 63 = 0x3F) which is the default for most trackers (64 rows per pattern). */ uint16_t last_row; #define AVSEQ_TRACK_LAST_ROW 63 #define AVSEQ_TRACK_LENGTH ((FF_AVSEQ_TRACK_LAST_ROW) + 1) #define AVSEQ_TRACK_LENGTH_MIN 0 #define AVSEQ_TRACK_LENGTH_MAX 65536
/** Track global volume (defaults to 255 = no volume scaling). */ uint8_t volume; #define AVSEQ_TRACK_VOLUME 255
/** Sub-volume level for this track. This is basically track global volume divided by 256, but the sub-volume doesn't account into actual mixer output (defaults 0). */ uint8_t sub_volume; #define AVSEQ_TRACK_SUB_VOLUME 0
/** Stereo panning level for this track (defaults to -128 = central stereo panning). */ int8_t panning; #define AVSEQ_TRACK_PANNING -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 sub_panning; #define AVSEQ_TRACK_SUB_PANNING 0
/** Track transpose. Add this value to octave * 12 + note to get the final octave / note played (defaults to 0). */ int8_t transpose; #define AVSEQ_TRACK_TRANSPOSE 0
/** Compatibility flags for playback. There are rare cases where track handling can not be mapped into internal playback engine and have to be handled specially. For each sub-song which needs this, this will define new flags which tag the player to handle it to that special way. */ uint8_t compat_flags; #define AVSEQ_TRACK_COMPAT_FLAG_SAMPLE_OFFSET 0x01 ///< Sample offset beyond end of sample will be ignored (IT compatibility) #define AVSEQ_TRACK_COMPAT_FLAG_TONE_PORTA 0x02 ///< Share tone portamento memory with portamentoes and unlock tone portamento samples and adjusts frequency to: new_freq = freq * new_rate / old_rate. If an instrument number is given the envelope will be retriggered (IT compatibility). #define AVSEQ_TRACK_COMPAT_FLAG_SLIDES 0x04 ///< Portamentos of same type share the same memory (e.g. porta up/fine porta up) #define AVSEQ_TRACK_COMPAT_FLAG_VOLUME_SLIDES 0x08 ///< All except portamento slides share the same memory (e.g. volume/panning slides) #define AVSEQ_TRACK_COMPAT_FLAG_OP_SLIDES 0x10 ///< Oppositional portamento directions don't share the same memory (e.g. porta up and porta down) #define AVSEQ_TRACK_COMPAT_FLAG_OP_VOLUME_SLIDES 0x20 ///< Oppositional non-portamento slide directions don't share the same memory #define AVSEQ_TRACK_COMPAT_FLAG_VOLUME_PITCH 0x40 ///< Volume & pitch slides share same memory (S3M compatibility)
/** Track playback flags. Some sequencers feature surround panning or allow initial reverse playback, different timing methods which have all to be taken care specially in the internal playback engine. */ uint8_t flags; #define AVSEQ_TRACK_FLAG_USE_TIMING 0x01 ///< Use track timing fields #define AVSEQ_TRACK_FLAG_SPD_TIMING 0x02 ///< SPD speed timing instead of BpM #define AVSEQ_TRACK_FLAG_PANNING 0x04 ///< Use track panning and sub-panning fields #define AVSEQ_TRACK_FLAG_SURROUND 0x08 ///< Use track surround panning #define AVSEQ_TRACK_FLAG_REVERSE 0x10 ///< Playback of track in backward direction
/** Initial number of frames per row, i.e. sequencer tempo (defaults to 6 as in most tracker formats). */ uint16_t frames; #define AVSEQ_TRACK_FRAMES 6
/** Initial speed multiplier, i.e. nominator which defaults to disabled = 0. */ uint8_t speed_mul; #define AVSEQ_TRACK_SPEED_MUL 0
/** Initial speed divider, i.e. denominator which defaults to disabled = 0. */ uint8_t speed_div; #define AVSEQ_TRACK_SPEED_DIV 0
/** Initial MED style SPD speed (defaults to 33 as in OctaMED Soundstudio). */ uint16_t spd_speed; #define AVSEQ_TRACK_SPD_SPEED 33
/** Initial number of rows per beat (defaults to 4 rows are a beat). */ uint16_t bpm_tempo; #define AVSEQ_TRACK_BPM_TEMPO 4
/** Initial beats per minute speed (defaults to 50 Hz => 125 BpM). */ uint16_t bpm_speed; #define AVSEQ_SONG_BPM_SPEED 125
/** 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 tracks in those chunks, which then won't get lost in that case. */ uint8_t **unknown_data; } AVSequencerTrack;
/** * Song track effect structure, This structure is actually for one row * and therefore actually pointed as an array with the amount of * rows of the whole track. * 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 AVSequencerTrackEffect {
/** Unused and reserved for future expansions, leave 0 for now. */ uint8_t reserved;
Can be removed.
/** Effect command byte. */ uint8_t command;
/** Command types. */
Doxygen will not parse it as you want. Better just /* Command types. */
/** Note effects (00-1F). */ #define AVSEQ_TRACK_EFFECT_NOTE_MIN 0x00 #define AVSEQ_TRACK_EFFECT_NOTE_MAX 0x1F
/** Volume control related effects (20-2F). */ #define AVSEQ_TRACK_EFFECT_VOLUME_MIN 0x20 #define AVSEQ_TRACK_EFFECT_VOLUME_MAX 0x2F
/** Panning control related effects (30-3F). */ #define AVSEQ_TRACK_EFFECT_PANNING_MIN 0x30 #define AVSEQ_TRACK_EFFECT_PANNING_MAX 0x3F
/** Track and channel control related effects (40-4F). */ #define AVSEQ_TRACK_EFFECT_TRACK_MIN 0x40 #define AVSEQ_TRACK_EFFECT_TRACK_MAX 0x4F
/** Instrument, sample and synth control related effects (50-5F). */ #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MIN 0x50 #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MAX 0x5F
/** Global control related effects (60-7E). */ #define AVSEQ_TRACK_EFFECT_GLOBAL_MIN 0x60 #define AVSEQ_TRACK_EFFECT_GLOBAL_MAX 0x7E
This is ugly and fragile (what would you do if a new format shows up with a new volume effect). Why not an array instead? static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] }; Note how easy would be change it to a struct to add more information about each effect. BTW, using Arpeggio == 0x00 is specific for some format or some ad-hoc choice? If it is ad-hoc, it should not be part of the Doxy comment, it is not useful information.
/** User customized effect for trigger in demos, etc. */ #define AVSEQ_TRACK_EFFECT_USER 0x7F
/** Note effect commands. 0x00 - Arpeggio: Data word consists of two 8-bit pairs named xx and yy where xx is the first halftone and yy the second halftone. Both xx and yy are signed values which means that a negative value is a backward arpeggio. */ #define AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO 0x00
[...] -Vitor
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
typedef struct AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
More a display octaves below 0 and beyond 9 problem, most trackers allocate a single character for displaying the octave (display in decimal), therefore octaves < 0 || > 9 are a serious problem. Also it's mostly useless, consider base octave 4 or 5. Playing an instrument such far away either causes silence (most instruments even fail to give hearable audible sound even at octave 2 and 8). Also track data is not a player structure, it's a track editor structure USED by the player as well. The data here is supposed to be directly editable by the user.
/** Number of instrument to be played or 0 to take previous one. */ uint16_t instrument;
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do
Changing this will seriously break compatibility to ANY tracker, ALL trackers I know use instrument as an identifier to use the previously intialized instrument. This field also is supposed to be directly editable by the user, and the user doesn't want to repeat the instrument number all and all again.
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
/** AVSequencerTrackData pointer to array of track data. */ AVSequencerTrackData *data;
This naming bothers me a little. If AVSequencerTrackData contains the track data, what does AVSequencerTrack contains?
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
/** * Song track effect structure, This structure is actually for one row * and therefore actually pointed as an array with the amount of * rows of the whole track. * 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 AVSequencerTrackEffect {
/** Unused and reserved for future expansions, leave 0 for now. */ uint8_t reserved;
Can be removed.
Fixed.
/** Effect command byte. */ uint8_t command;
/** Command types. */
Doxygen will not parse it as you want. Better just
/* Command types. */
/** Note effects (00-1F). */ #define AVSEQ_TRACK_EFFECT_NOTE_MIN 0x00 #define AVSEQ_TRACK_EFFECT_NOTE_MAX 0x1F
/** Volume control related effects (20-2F). */ #define AVSEQ_TRACK_EFFECT_VOLUME_MIN 0x20 #define AVSEQ_TRACK_EFFECT_VOLUME_MAX 0x2F
/** Panning control related effects (30-3F). */ #define AVSEQ_TRACK_EFFECT_PANNING_MIN 0x30 #define AVSEQ_TRACK_EFFECT_PANNING_MAX 0x3F
/** Track and channel control related effects (40-4F). */ #define AVSEQ_TRACK_EFFECT_TRACK_MIN 0x40 #define AVSEQ_TRACK_EFFECT_TRACK_MAX 0x4F
/** Instrument, sample and synth control related effects (50-5F). */ #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MIN 0x50 #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MAX 0x5F
/** Global control related effects (60-7E). */ #define AVSEQ_TRACK_EFFECT_GLOBAL_MIN 0x60 #define AVSEQ_TRACK_EFFECT_GLOBAL_MAX 0x7E
This is ugly and fragile (what would you do if a new format shows up with a new volume effect). Why not an array instead?
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] };
Note how easy would be change it to a struct to add more information about each effect.
??? I really don't understand this approach besides using enums.
BTW, using Arpeggio == 0x00 is specific for some format or some ad-hoc choice? If it is ad-hoc, it should not be part of the Doxy comment, it is not useful information.
Arpeggio is in most trackers the first effect (which start at 00). Also it is required for documenting the arpeggio effect, i.e. parameters. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
typedef struct AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
More a display octaves below 0 and beyond 9 problem, most trackers allocate a single character for displaying the octave (display in decimal), therefore octaves< 0 ||> 9 are a serious problem. Also it's mostly useless, consider base octave 4 or 5. Playing an instrument such far away either causes silence (most instruments even fail to give hearable audible sound even at octave 2 and 8).
Also track data is not a player structure, it's a track editor structure USED by the player as well. The data here is supposed to be directly editable by the user.
In this case it is not a hard limit. It is better than to put a comment instead of the defines: /** Which octave the note is played upon, if note is a positive value (defaults to 4). Sane values are in the [0:9] range, expect trouble with most trackers if you use other values. */
/** Number of instrument to be played or 0 to take previous one. */ uint16_t instrument;
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do
Changing this will seriously break compatibility to ANY tracker,
The loader can do if (read_instrument == 0) instrument = last_instrument; and no compatibility is lost.
ALL trackers I know use instrument as an identifier to use the previously intialized instrument.
That's a good point. If (instrument == 0) obviously means "last instrument" for everyone who has a good experience of tracker formats, this improves readability instead of obfuscating it.
This field also is supposed to be directly editable by the user, and the user doesn't want to repeat the instrument number all and all again.
The user is a software, it can hide it from the user.
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
/** AVSequencerTrackData pointer to array of track data. */ AVSequencerTrackData *data;
This naming bothers me a little. If AVSequencerTrackData contains the track data, what does AVSequencerTrack contains?
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
/** * Song track effect structure, This structure is actually for one row * and therefore actually pointed as an array with the amount of * rows of the whole track. * 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 AVSequencerTrackEffect {
/** Unused and reserved for future expansions, leave 0 for now. */ uint8_t reserved;
Can be removed.
Fixed.
/** Effect command byte. */ uint8_t command;
/** Command types. */
Doxygen will not parse it as you want. Better just
/* Command types. */
/** Note effects (00-1F). */ #define AVSEQ_TRACK_EFFECT_NOTE_MIN 0x00 #define AVSEQ_TRACK_EFFECT_NOTE_MAX 0x1F
/** Volume control related effects (20-2F). */ #define AVSEQ_TRACK_EFFECT_VOLUME_MIN 0x20 #define AVSEQ_TRACK_EFFECT_VOLUME_MAX 0x2F
/** Panning control related effects (30-3F). */ #define AVSEQ_TRACK_EFFECT_PANNING_MIN 0x30 #define AVSEQ_TRACK_EFFECT_PANNING_MAX 0x3F
/** Track and channel control related effects (40-4F). */ #define AVSEQ_TRACK_EFFECT_TRACK_MIN 0x40 #define AVSEQ_TRACK_EFFECT_TRACK_MAX 0x4F
/** Instrument, sample and synth control related effects (50-5F). */ #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MIN 0x50 #define AVSEQ_TRACK_EFFECT_INSTRUMENT_MAX 0x5F
/** Global control related effects (60-7E). */ #define AVSEQ_TRACK_EFFECT_GLOBAL_MIN 0x60 #define AVSEQ_TRACK_EFFECT_GLOBAL_MAX 0x7E
This is ugly and fragile (what would you do if a new format shows up with a new volume effect). Why not an array instead?
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] };
Note how easy would be change it to a struct to add more information about each effect.
??? I really don't understand this approach besides using enums.
The idea was replacing if (cmd >= 0x20 && cmd <= 0x2F) // check if it is a volume command by if (effect_type[cmd] == AVSEQ_TRACK_EFFECT_VOLUME) -Vitor
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
typedef struct AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
More a display octaves below 0 and beyond 9 problem, most trackers allocate a single character for displaying the octave (display in decimal), therefore octaves< 0 ||> 9 are a serious problem. Also it's mostly useless, consider base octave 4 or 5. Playing an instrument such far away either causes silence (most instruments even fail to give hearable audible sound even at octave 2 and 8).
Also track data is not a player structure, it's a track editor structure USED by the player as well. The data here is supposed to be directly editable by the user.
In this case it is not a hard limit. It is better than to put a comment instead of the defines:
/** Which octave the note is played upon, if note is a positive value (defaults to 4). Sane values are in the [0:9] range, expect trouble with most trackers if you use other values. */
There is actually a hard limit, remember increasing octave by one means pow by 2 a more, i.e. octave n to n+1 means chaning playback rate 2^n to 2^(n+1). This will quickly overflow with typical integers. When this happens is mainly dependant on the base frequency, if it's 44100 Hz, then using octave 9 (when base is 4) will result playback that sample with 44100*2^(9-4) Hz. Limiting that to 9 will not cause 32-bit overflow, except you are such crazy and set base frequency far higher than 16777216 or sth. like that, were you are really close to the limit.
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do
Changing this will seriously break compatibility to ANY tracker,
The loader can do
if (read_instrument == 0) instrument = last_instrument;
and no compatibility is lost.
Theoretically, yes, but would make much more complex! They would have to analyse the complete order list range and pattern data, solely for this purpose. Also this destroys transcoding as close to the original. Calling any transcoder in that case change the data the original composer entered, which should be avoided as much possible. Remember the user can also enter sth. like: C-5 03 .. ... ... .. .. ... C-5 00 .. ... // Use instrument 03 again ... .. .. ... etc. By converting back, we don't know if the composer entered originally the above example or did enter: C-5 03 .. ... ... .. .. ... C-5 03 .. ... This is a very bad idea! Pattern data should only be changed during transcoding if this is really necessary.
ALL trackers I know use instrument as an identifier to use the previously intialized instrument.
That's a good point. If (instrument == 0) obviously means "last instrument" for everyone who has a good experience of tracker formats, this improves readability instead of obfuscating it.
Yepp! ;-)
This field also is supposed to be directly editable by the user, and the user doesn't want to repeat the instrument number all and all again.
The user is a software, it can hide it from the user.
Not really, ok, you could invent an extra bit: The user used the same instrument as before, but remember, users can change pattern ordering / order list changing, etc. For every normal tracker, 0 does that simply, removing 0 will require all pattern data changing for all constellations for it, which will be just a plain nightmare. And sometimes, the user does do things like: C-5 03 .. ... ... C-5 00 .. ... ... C-5 03 .. ... ... Because he eventually wants to change the latter C-5 03 with sth. else, but is sure to keep the first C-5 03 with the follwing C-5 00's.
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM <=> S3M/IT. But this is part of the demuxer to convert the origins...
static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] };
Note how easy would be change it to a struct to add more information about each effect.
??? I really don't understand this approach besides using enums.
The idea was replacing
if (cmd >= 0x20 && cmd <= 0x2F) // check if it is a volume command
by
if (effect_type[cmd] == AVSEQ_TRACK_EFFECT_VOLUME)
Nice idea, but actually I'm not checking internally for that, I just use a jump-table (see player.c) and that solely decides what happens. The range defines are just for user-friendlyness, i.e. more in a sense of: // User has the cursor in a volume command and presses F1: if (cmd >= 0x20 && cmd <= 0x2F) // Display help for volume command list. The player doesn't care about these, there is simply a jump table and that's being executed. If you think we don't need that, my suggestion is to remove that completely. -- Best regards, :-) Basty/CDGS (-:
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
typedef struct AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
More a display octaves below 0 and beyond 9 problem, most trackers allocate a single character for displaying the octave (display in decimal), therefore octaves< 0 ||> 9 are a serious problem. Also it's mostly useless, consider base octave 4 or 5. Playing an instrument such far away either causes silence (most instruments even fail to give hearable audible sound even at octave 2 and 8).
Also track data is not a player structure, it's a track editor structure USED by the player as well. The data here is supposed to be directly editable by the user.
In this case it is not a hard limit. It is better than to put a comment instead of the defines:
/** Which octave the note is played upon, if note is a positive value (defaults to 4). Sane values are in the [0:9] range, expect trouble with most trackers if you use other values. */
There is actually a hard limit, remember increasing octave by one means pow by 2 a more, i.e. octave n to n+1 means chaning playback rate 2^n to 2^(n+1). This will quickly overflow with typical integers.
When this happens is mainly dependant on the base frequency, if it's 44100 Hz, then using octave 9 (when base is 4) will result playback that sample with 44100*2^(9-4) Hz. Limiting that to 9 will not cause 32-bit overflow, except you are such crazy and set base frequency far higher than 16777216 or sth. like that, were you are really close to the limit.
What about -1? Anyway, I think this is not part of the BSS. If someone is stupid and want to create a file with senseless octaves, well, it is not the business of the BSS to enforce it but either the muxer should check it or the player should refuse to play it.
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do
Changing this will seriously break compatibility to ANY tracker,
The loader can do
if (read_instrument == 0) instrument = last_instrument;
and no compatibility is lost.
Theoretically, yes, but would make much more complex! They would have to analyse the complete order list range and pattern data, solely for this purpose. Also this destroys transcoding as close to the original. Calling any transcoder in that case change the data the original composer entered, which should be avoided as much possible.
Remember the user can also enter sth. like: C-5 03 .. ... ... .. .. ... C-5 00 .. ... // Use instrument 03 again ... .. .. ...
etc.
By converting back, we don't know if the composer entered originally the above example or did enter: C-5 03 .. ... ... .. .. ... C-5 03 .. ...
This is a very bad idea! Pattern data should only be changed during transcoding if this is really necessary.
ALL trackers I know use instrument as an identifier to use the previously intialized instrument.
That's a good point. If (instrument == 0) obviously means "last instrument" for everyone who has a good experience of tracker formats, this improves readability instead of obfuscating it.
Yepp! ;-)
This field also is supposed to be directly editable by the user, and the user doesn't want to repeat the instrument number all and all again.
The user is a software, it can hide it from the user.
Not really, ok, you could invent an extra bit: The user used the same instrument as before, but remember, users can change pattern ordering / order list changing, etc.
For every normal tracker, 0 does that simply, removing 0 will require all pattern data changing for all constellations for it, which will be just a plain nightmare.
And sometimes, the user does do things like: C-5 03 .. ... ... C-5 00 .. ... ... C-5 03 .. ... ...
Because he eventually wants to change the latter C-5 03 with sth. else, but is sure to keep the first C-5 03 with the follwing C-5 00's.
Not losing information during transcoding is also a good point.
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do: /** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66 is confusing and misleading in comparison with /** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] };
Note how easy would be change it to a struct to add more information about each effect.
??? I really don't understand this approach besides using enums.
The idea was replacing
if (cmd>= 0x20&& cmd<= 0x2F) // check if it is a volume command
by
if (effect_type[cmd] == AVSEQ_TRACK_EFFECT_VOLUME)
Nice idea, but actually I'm not checking internally for that, I just use a jump-table (see player.c) and that solely decides what happens.
The range defines are just for user-friendlyness, i.e. more in a sense of:
// User has the cursor in a volume command and presses F1: if (cmd>= 0x20&& cmd<= 0x2F) // Display help for volume command list.
The player doesn't care about these, there is simply a jump table and that's being executed. If you think we don't need that, my suggestion is to remove that completely.
I agree completely. Let's start by getting committed what is used currently. -Vitor
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
In this case it is not a hard limit. It is better than to put a comment instead of the defines:
/** Which octave the note is played upon, if note is a positive value (defaults to 4). Sane values are in the [0:9] range, expect trouble with most trackers if you use other values. */
There is actually a hard limit, remember increasing octave by one means pow by 2 a more, i.e. octave n to n+1 means chaning playback rate 2^n to 2^(n+1). This will quickly overflow with typical integers.
When this happens is mainly dependant on the base frequency, if it's 44100 Hz, then using octave 9 (when base is 4) will result playback that sample with 44100*2^(9-4) Hz. Limiting that to 9 will not cause 32-bit overflow, except you are such crazy and set base frequency far higher than 16777216 or sth. like that, were you are really close to the limit.
What about -1? Anyway, I think this is not part of the BSS. If someone is stupid and want to create a file with senseless octaves, well, it is not the business of the BSS to enforce it but either the muxer should check it or the player should refuse to play it.
I forgot to mention one point, the octave number is also used by the keyboard definition table, which is hard-coded to a size 120 entries (10 octaves * 12 notes per octave). Allowing values outside this will cause access to uninitialized memory. Fixed the documentation to mention this, though.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
Still unsure about this yet, what about AVSequencerTrackRowData? This clarifys that this structure belongs to tracks and not sth. else.
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
The effect numbers are supposed also to editable by the user (in trackers you enter them has hex values directly, some trackers however, do use alphabetical ordering, A-Z like S3M/IT), but internally they're hex numbers. Due to the large number of effects, I decided as a base approach to display the hex number there, too. I will however, remove the duplicate mentions, when I convert that to enum tonight, ok?
// User has the cursor in a volume command and presses F1: if (cmd>= 0x20&& cmd<= 0x2F) // Display help for volume command list.
The player doesn't care about these, there is simply a jump table and that's being executed. If you think we don't need that, my suggestion is to remove that completely.
I agree completely. Let's start by getting committed what is used currently.
Fixed by removing it. -- Best regards, :-) Basty/CDGS (-:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote:
typedef struct AVSequencerTrackData { /** Array of pointers containing all effects used by this track. */ AVSequencerTrack **effects_data;
/** Which octave the note is played upon, if note is a positive value (defaults to 4). */ uint8_t octave; #define AVSEQ_TRACK_DATA_OCTAVE 4 #define AVSEQ_TRACK_DATA_OCTAVE_MIN 0 #define AVSEQ_TRACK_DATA_OCTAVE_MAX 9
Those MAX/MIN are characteristics of the player, no? So...
More a display octaves below 0 and beyond 9 problem, most trackers allocate a single character for displaying the octave (display in decimal), therefore octaves< 0 ||> 9 are a serious problem. Also it's mostly useless, consider base octave 4 or 5. Playing an instrument such far away either causes silence (most instruments even fail to give hearable audible sound even at octave 2 and 8).
Also track data is not a player structure, it's a track editor structure USED by the player as well. The data here is supposed to be directly editable by the user.
In this case it is not a hard limit. It is better than to put a comment instead of the defines:
/** Which octave the note is played upon, if note is a positive value (defaults to 4). Sane values are in the [0:9] range, expect trouble with most trackers if you use other values. */
There is actually a hard limit, remember increasing octave by one means pow by 2 a more, i.e. octave n to n+1 means chaning playback rate 2^n to 2^(n+1). This will quickly overflow with typical integers.
When this happens is mainly dependant on the base frequency, if it's 44100 Hz, then using octave 9 (when base is 4) will result playback that sample with 44100*2^(9-4) Hz. Limiting that to 9 will not cause 32-bit overflow, except you are such crazy and set base frequency far higher than 16777216 or sth. like that, were you are really close to the limit.
What about -1? Anyway, I think this is not part of the BSS. If someone is stupid and want to create a file with senseless octaves, well, it is not the business of the BSS to enforce it but either the muxer should check it or the player should refuse to play it.
Why 0 to take previous one? This forces your array to start at 1 and IMHO does not simplify any code. In the player you can always do
Changing this will seriously break compatibility to ANY tracker,
The loader can do
if (read_instrument == 0) instrument = last_instrument;
and no compatibility is lost.
Theoretically, yes, but would make much more complex! They would have to analyse the complete order list range and pattern data, solely for this purpose. Also this destroys transcoding as close to the original. Calling any transcoder in that case change the data the original composer entered, which should be avoided as much possible.
Remember the user can also enter sth. like: C-5 03 .. ... ... .. .. ... C-5 00 .. ... // Use instrument 03 again ... .. .. ...
etc.
By converting back, we don't know if the composer entered originally the above example or did enter: C-5 03 .. ... ... .. .. ... C-5 03 .. ...
This is a very bad idea! Pattern data should only be changed during transcoding if this is really necessary.
ALL trackers I know use instrument as an identifier to use the previously intialized instrument.
That's a good point. If (instrument == 0) obviously means "last instrument" for everyone who has a good experience of tracker formats, this improves readability instead of obfuscating it.
Yepp! ;-)
This field also is supposed to be directly editable by the user, and the user doesn't want to repeat the instrument number all and all again.
The user is a software, it can hide it from the user.
Not really, ok, you could invent an extra bit: The user used the same instrument as before, but remember, users can change pattern ordering / order list changing, etc.
For every normal tracker, 0 does that simply, removing 0 will require all pattern data changing for all constellations for it, which will be just a plain nightmare.
And sometimes, the user does do things like: C-5 03 .. ... ... C-5 00 .. ... ... C-5 03 .. ... ...
Because he eventually wants to change the latter C-5 03 with sth. else, but is sure to keep the first C-5 03 with the follwing C-5 00's.
Not losing information during transcoding is also a good point.
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
static const Enum EffectType effect_type[AVSEQ_EFFECTS_NUMBER] = { [AVSEQ_TRACK_EFFECT_CMD_ARPEGGIO] = AVSEQ_TRACK_EFFECT_NOTE, [AVSEQ_TRACK_EFFECT_CMD_PORTA_UP] = AVSEQ_TRACK_EFFECT_NOTE, [...] };
Note how easy would be change it to a struct to add more information about each effect.
??? I really don't understand this approach besides using enums.
The idea was replacing
if (cmd>= 0x20&& cmd<= 0x2F) // check if it is a volume command
by
if (effect_type[cmd] == AVSEQ_TRACK_EFFECT_VOLUME)
Nice idea, but actually I'm not checking internally for that, I just use a jump-table (see player.c) and that solely decides what happens.
The range defines are just for user-friendlyness, i.e. more in a sense of:
// User has the cursor in a volume command and presses F1: if (cmd>= 0x20&& cmd<= 0x2F) // Display help for volume command list.
The player doesn't care about these, there is simply a jump table and that's being executed. If you think we don't need that, my suggestion is to remove that completely.
I agree completely. Let's start by getting committed what is used currently.
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 track.[ch] part of the BSS to review. This version compiles perfectly. -- Best regards, :-) Basty/CDGS (-:
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/11/2010 10:07 PM, Sebastian Vater wrote: > typedef struct AVSequencerTrackData {
[...]
if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
So?
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
So? [...]
diff --git a/libavsequencer/track.h b/libavsequencer/track.h new file mode 100755 index 0000000..cf131af --- /dev/null +++ b/libavsequencer/track.h @@ -0,0 +1,1636 @@ +/* + * AVSequencer track and pattern 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_TRACK_H +#define AVSEQUENCER_TRACK_H + +#include "libavutil/log.h" +#include "libavformat/avformat.h" +
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
+ /** Effect command data word. */ + uint16_t data; +} AVSequencerTrackEffect; + +/** AVSequencerTrackEffect->note values. */ +enum AVSequencerTrackDataNote { + /** --- */ + AVSEQ_TRACK_DATA_NOTE_NONE = 0, + + /** C-n */ + AVSEQ_TRACK_DATA_NOTE_C = 1, + + /** C#n = Dbn */ + AVSEQ_TRACK_DATA_NOTE_C_SHARP = 2, + + /** D-n */ + AVSEQ_TRACK_DATA_NOTE_D = 3, + + /** D#n = Ebn */ + AVSEQ_TRACK_DATA_NOTE_D_SHARP = 4, + + /** E-n */ + AVSEQ_TRACK_DATA_NOTE_E = 5, + + /** F-n */ + AVSEQ_TRACK_DATA_NOTE_F = 6, + + /** F#n = Gbn */ + AVSEQ_TRACK_DATA_NOTE_F_SHARP = 7, + + /** G-n */ + AVSEQ_TRACK_DATA_NOTE_G = 8, + + /** G#n = Abn */ + AVSEQ_TRACK_DATA_NOTE_G_SHARP = 9, + + /** A-n */ + AVSEQ_TRACK_DATA_NOTE_A = 10, + + /** A#n = Bbn */ + AVSEQ_TRACK_DATA_NOTE_A_SHARP = 11, + + /** B-n = H-n */ + AVSEQ_TRACK_DATA_NOTE_B = 12, + + /** ^^^ = note kill */ + AVSEQ_TRACK_DATA_NOTE_KILL = -1, + + /** ^^- = note off */ + AVSEQ_TRACK_DATA_NOTE_OFF = -2, + + /** === = keyoff note */ + AVSEQ_TRACK_DATA_NOTE_KEYOFF = -3, + + /** -|- = hold delay */ + AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY = -4, + + /** -\- = note fade */ + AVSEQ_TRACK_DATA_NOTE_FADE = -5, + + /** END = pattern end marker */ + AVSEQ_TRACK_DATA_NOTE_END = -16, +};
Why hardcoding values? Why not just use: /** AVSequencerTrackEffect->note values. */ enum AVSequencerTrackDataNote { /** --- */ AVSEQ_TRACK_DATA_NOTE_NONE, /** C-n */ AVSEQ_TRACK_DATA_NOTE_C, /** C#n = Dbn */ AVSEQ_TRACK_DATA_NOTE_C_SHARP, [...] } And let the compiler assign whatever numbers it seems fit?
+ +/** + * Song track data structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackData { + /** Array (of size effects) of pointers containing all effects + used by this track. */ + AVSequencerTrackEffect **effects_data; + + /** Number of effects used by this track. */ + uint16_t effects; + + /** Which octave the note is played upon, if note is a positive + value (defaults to 4). Allowed values are in the [0:9] range, + since the keyboard definition table has 120 entries (10 octave + range * 12 notes per octave), also expect trouble with most + trackers if values outside this range are used. */ + uint8_t octave; +
+ /** Note to be played (see defines below, n is octave number). */
which defines?
+ int8_t note;
enum?
+ /** Number of instrument to be played or 0 to take previous one. */ + uint16_t instrument; +} AVSequencerTrackData; + +/** AVSequencerTrack->compat_flags bitfield. */ +enum AVSequencerTrackCompatFlags { + AVSEQ_TRACK_COMPAT_FLAG_SAMPLE_OFFSET = 0x01, ///< Sample offset beyond end of sample will be ignored (IT compatibility) + AVSEQ_TRACK_COMPAT_FLAG_TONE_PORTA = 0x02, ///< Share tone portamento memory with portamentoes and unlock tone portamento samples and adjusts frequency to: new_freq = freq * new_rate / old_rate. If an instrument number is given the envelope will be retriggered (IT compatibility). + AVSEQ_TRACK_COMPAT_FLAG_SLIDES = 0x04, ///< Portamentos of same type share the same memory (e.g. porta up/fine porta up) + AVSEQ_TRACK_COMPAT_FLAG_VOLUME_SLIDES = 0x08, ///< All except portamento slides share the same memory (e.g. volume/panning slides) + AVSEQ_TRACK_COMPAT_FLAG_OP_SLIDES = 0x10, ///< Oppositional portamento directions don't share the same memory (e.g. porta up and porta down) + AVSEQ_TRACK_COMPAT_FLAG_OP_VOLUME_SLIDES = 0x20, ///< Oppositional non-portamento slide directions don't share the same memory + AVSEQ_TRACK_COMPAT_FLAG_VOLUME_PITCH = 0x40, ///< Volume & pitch slides share same memory (S3M compatibility) +}; + +/** AVSequencerTrack->flags bitfield. */ +enum AVSequencerTrackFlags { + AVSEQ_TRACK_FLAG_USE_TIMING = 0x01, ///< Use track timing fields + AVSEQ_TRACK_FLAG_SPD_TIMING = 0x02, ///< SPD speed timing instead of BpM + AVSEQ_TRACK_FLAG_PANNING = 0x04, ///< Use track panning and sub-panning fields + AVSEQ_TRACK_FLAG_SURROUND = 0x08, ///< Use track surround panning + AVSEQ_TRACK_FLAG_REVERSE = 0x10, ///< Playback of track in backward direction +}; + +/** + * Song track 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 AVSequencerTrack { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original track file name, track + title, track message, track artist, track album, + track begin and finish date of composition and comment. */ + AVMetadata *metadata; + + /** AVSequencerTrackData pointer to array of track data. */ + AVSequencerTrackData *data;
+ /** Last valid row of track (defaults to 63 = 0x3F) which + is the default for most trackers (64 rows per pattern). */ + uint16_t last_row;
Again, is this needed to play a track?
+ /** Track global volume (defaults to 255 = no volume scaling). */ + uint8_t volume; + + /** Sub-volume level for this track. This is basically track + global volume divided by 256, but the sub-volume doesn't + account into actual mixer output (defaults 0). */ + uint8_t sub_volume; + + /** Stereo panning level for this track (defaults to + -128 = central stereo panning). */ + int8_t 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 sub_panning; + + /** Track transpose. Add this value to octave * 12 + note to + get the final octave / note played (defaults to 0). */ + int8_t transpose;
+ /** Compatibility flags for playback. There are rare cases + where track handling can not be mapped into internal + playback engine and have to be handled specially. For + each sub-song which needs this, this will define new + flags which tag the player to handle it to that special + way. */ + uint8_t compat_flags;
enum?
+ /** Track playback flags. Some sequencers feature + surround panning or allow initial reverse playback, + different timing methods which have all to be taken + care specially in the internal playback engine. */ + uint8_t flags;
enum? -Vitor
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote:
Vitor Sessak a écrit : > On 07/11/2010 10:07 PM, Sebastian Vater wrote: >> typedef struct AVSequencerTrackData {
[...]
> if (instrument == previous_instrument) {...}
Please note that instrument number 0 also differs whether you play the track in once mode (pattern play mode) and the instrument number was initialized in the previous order. All trackers don't output sound in that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
TrackData is the octave, note, effect and effect data for each row, Track itself defines the structure containing it, i.e. number of rows, maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
So?
AVSequencerTrackRow? Just to make the hierarchy clearer...agree on this?
This would require extending using 0x80-0xFF command effect which would conflict with synth sound handling (see there at the synth sound instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
So?
Sorry, don't understand here, what you mean.
[...]
diff --git a/libavsequencer/track.h b/libavsequencer/track.h new file mode 100755 index 0000000..cf131af --- /dev/null +++ b/libavsequencer/track.h @@ -0,0 +1,1636 @@ +/* + * AVSequencer track and pattern 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_TRACK_H +#define AVSEQUENCER_TRACK_H + +#include "libavutil/log.h" +#include "libavformat/avformat.h" +
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
+ /** Effect command data word. */ + uint16_t data; +} AVSequencerTrackEffect; + +/** AVSequencerTrackEffect->note values. */ +enum AVSequencerTrackDataNote { + /** --- */ + AVSEQ_TRACK_DATA_NOTE_NONE = 0, + + /** C-n */ + AVSEQ_TRACK_DATA_NOTE_C = 1, + + /** C#n = Dbn */ + AVSEQ_TRACK_DATA_NOTE_C_SHARP = 2, + + /** D-n */ + AVSEQ_TRACK_DATA_NOTE_D = 3, + + /** D#n = Ebn */ + AVSEQ_TRACK_DATA_NOTE_D_SHARP = 4, + + /** E-n */ + AVSEQ_TRACK_DATA_NOTE_E = 5, + + /** F-n */ + AVSEQ_TRACK_DATA_NOTE_F = 6, + + /** F#n = Gbn */ + AVSEQ_TRACK_DATA_NOTE_F_SHARP = 7, + + /** G-n */ + AVSEQ_TRACK_DATA_NOTE_G = 8, + + /** G#n = Abn */ + AVSEQ_TRACK_DATA_NOTE_G_SHARP = 9, + + /** A-n */ + AVSEQ_TRACK_DATA_NOTE_A = 10, + + /** A#n = Bbn */ + AVSEQ_TRACK_DATA_NOTE_A_SHARP = 11, + + /** B-n = H-n */ + AVSEQ_TRACK_DATA_NOTE_B = 12, + + /** ^^^ = note kill */ + AVSEQ_TRACK_DATA_NOTE_KILL = -1, + + /** ^^- = note off */ + AVSEQ_TRACK_DATA_NOTE_OFF = -2, + + /** === = keyoff note */ + AVSEQ_TRACK_DATA_NOTE_KEYOFF = -3, + + /** -|- = hold delay */ + AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY = -4, + + /** -\- = note fade */ + AVSEQ_TRACK_DATA_NOTE_FADE = -5, + + /** END = pattern end marker */ + AVSEQ_TRACK_DATA_NOTE_END = -16, +};
Why hardcoding values? Why not just use:
/** AVSequencerTrackEffect->note values. */ enum AVSequencerTrackDataNote { /** --- */ AVSEQ_TRACK_DATA_NOTE_NONE,
/** C-n */ AVSEQ_TRACK_DATA_NOTE_C,
/** C#n = Dbn */ AVSEQ_TRACK_DATA_NOTE_C_SHARP,
[...] }
And let the compiler assign whatever numbers it seems fit?
Would require every demuxer/muxer to convert these values if they don't match, also current player engine checks for < 0 to see if it's a special note. Anway, for notes between 1 and 12 (C-5 and B-5, e.g.), this is a great idea! Will fix this!
+ +/** + * Song track data structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackData { + /** Array (of size effects) of pointers containing all effects + used by this track. */ + AVSequencerTrackEffect **effects_data; + + /** Number of effects used by this track. */ + uint16_t effects; + + /** Which octave the note is played upon, if note is a positive + value (defaults to 4). Allowed values are in the [0:9] range, + since the keyboard definition table has 120 entries (10 octave + range * 12 notes per octave), also expect trouble with most + trackers if values outside this range are used. */ + uint8_t octave; +
+ /** Note to be played (see defines below, n is octave number). */
which defines?
Oops, will fix this, too!
+ int8_t note;
enum?
Again what you mean with that?
+ /** Number of instrument to be played or 0 to take previous one. */ + uint16_t instrument; +} AVSequencerTrackData; + +/** AVSequencerTrack->compat_flags bitfield. */ +enum AVSequencerTrackCompatFlags { + AVSEQ_TRACK_COMPAT_FLAG_SAMPLE_OFFSET = 0x01, ///< Sample offset beyond end of sample will be ignored (IT compatibility) + AVSEQ_TRACK_COMPAT_FLAG_TONE_PORTA = 0x02, ///< Share tone portamento memory with portamentoes and unlock tone portamento samples and adjusts frequency to: new_freq = freq * new_rate / old_rate. If an instrument number is given the envelope will be retriggered (IT compatibility). + AVSEQ_TRACK_COMPAT_FLAG_SLIDES = 0x04, ///< Portamentos of same type share the same memory (e.g. porta up/fine porta up) + AVSEQ_TRACK_COMPAT_FLAG_VOLUME_SLIDES = 0x08, ///< All except portamento slides share the same memory (e.g. volume/panning slides) + AVSEQ_TRACK_COMPAT_FLAG_OP_SLIDES = 0x10, ///< Oppositional portamento directions don't share the same memory (e.g. porta up and porta down) + AVSEQ_TRACK_COMPAT_FLAG_OP_VOLUME_SLIDES = 0x20, ///< Oppositional non-portamento slide directions don't share the same memory + AVSEQ_TRACK_COMPAT_FLAG_VOLUME_PITCH = 0x40, ///< Volume & pitch slides share same memory (S3M compatibility) +}; + +/** AVSequencerTrack->flags bitfield. */ +enum AVSequencerTrackFlags { + AVSEQ_TRACK_FLAG_USE_TIMING = 0x01, ///< Use track timing fields + AVSEQ_TRACK_FLAG_SPD_TIMING = 0x02, ///< SPD speed timing instead of BpM + AVSEQ_TRACK_FLAG_PANNING = 0x04, ///< Use track panning and sub-panning fields + AVSEQ_TRACK_FLAG_SURROUND = 0x08, ///< Use track surround panning + AVSEQ_TRACK_FLAG_REVERSE = 0x10, ///< Playback of track in backward direction +}; + +/** + * Song track 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 AVSequencerTrack { + /** + * information on struct for av_log + * - set by avseq_alloc_context + */ + const AVClass *av_class; + + /** Metadata information: Original track file name, track + title, track message, track artist, track album, + track begin and finish date of composition and comment. */ + AVMetadata *metadata; + + /** AVSequencerTrackData pointer to array of track data. */ + AVSequencerTrackData *data;
+ /** Last valid row of track (defaults to 63 = 0x3F) which + is the default for most trackers (64 rows per pattern). */ + uint16_t last_row;
Again, is this needed to play a track?
Of course, how should the player (or the BSS) know at which row a track ends, it's like module->song_list vs. module->songs indicating total number of songs.
+ /** Track global volume (defaults to 255 = no volume scaling). */ + uint8_t volume; + + /** Sub-volume level for this track. This is basically track + global volume divided by 256, but the sub-volume doesn't + account into actual mixer output (defaults 0). */ + uint8_t sub_volume; + + /** Stereo panning level for this track (defaults to + -128 = central stereo panning). */ + int8_t 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 sub_panning; + + /** Track transpose. Add this value to octave * 12 + note to + get the final octave / note played (defaults to 0). */ + int8_t transpose;
+ /** Compatibility flags for playback. There are rare cases + where track handling can not be mapped into internal + playback engine and have to be handled specially. For + each sub-song which needs this, this will define new + flags which tag the player to handle it to that special + way. */ + uint8_t compat_flags;
enum?
Again, what you mean with this, enums are there, so...
+ /** Track playback flags. Some sequencers feature + surround panning or allow initial reverse playback, + different timing methods which have all to be taken + care specially in the internal playback engine. */ + uint8_t flags;
enum?
And again once a more time... -- Best regards, :-) Basty/CDGS (-:
On Fri, Aug 13, 2010 at 10:28:55PM +0200, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote: > Vitor Sessak a écrit : >> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>> typedef struct AVSequencerTrackData {
[...]
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
The problem is that it seems you intended 'command' to be an enum, but you declared it as an uint8_t instead. You have to declare it as an enum to allow proper type checking (among other things). So for this one, what you want is: enum AVSequencerTrackEffectCommand command; The same apply to all the enum remarks that you didn't understood. Aurel
Aurelien Jacobs a écrit :
On Fri, Aug 13, 2010 at 10:28:55PM +0200, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
> On 07/13/2010 09:35 PM, Sebastian Vater wrote: > >> Vitor Sessak a écrit : >> >>> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>> >>>> typedef struct AVSequencerTrackData { >>>> [...]
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
The problem is that it seems you intended 'command' to be an enum, but you declared it as an uint8_t instead. You have to declare it as an enum to allow proper type checking (among other things). So for this one, what you want is: enum AVSequencerTrackEffectCommand command;
The same apply to all the enum remarks that you didn't understood.
Ahh ok, thank you very much, I missed that point. But there are cases, where the enum types should be really uint8_t and uint16_t...how to solve that? -- Best regards, :-) Basty/CDGS (-:
On Sat, Aug 14, 2010 at 12:01:08AM +0200, Sebastian Vater wrote:
Aurelien Jacobs a écrit :
On Fri, Aug 13, 2010 at 10:28:55PM +0200, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit : > >> On 07/13/2010 09:35 PM, Sebastian Vater wrote: >> >>> Vitor Sessak a écrit : >>> >>>> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>>> >>>>> typedef struct AVSequencerTrackData { >>>>> [...]
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
The problem is that it seems you intended 'command' to be an enum, but you declared it as an uint8_t instead. You have to declare it as an enum to allow proper type checking (among other things). So for this one, what you want is: enum AVSequencerTrackEffectCommand command;
The same apply to all the enum remarks that you didn't understood.
Ahh ok, thank you very much, I missed that point. But there are cases, where the enum types should be really uint8_t and uint16_t...how to solve that?
I really can't imagine any good reason why you would need to store those struct fields as uint8_t or uint16_t... Aurel
On 08/14/2010 12:01 AM, Sebastian Vater wrote:
Aurelien Jacobs a écrit :
On Fri, Aug 13, 2010 at 10:28:55PM +0200, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit : > >> On 07/13/2010 09:35 PM, Sebastian Vater wrote: >> >>> Vitor Sessak a écrit : >>> >>>> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>>> >>>>> typedef struct AVSequencerTrackData { >>>>> [...]
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
The problem is that it seems you intended 'command' to be an enum, but you declared it as an uint8_t instead. You have to declare it as an enum to allow proper type checking (among other things). So for this one, what you want is: enum AVSequencerTrackEffectCommand command;
The same apply to all the enum remarks that you didn't understood.
Ahh ok, thank you very much, I missed that point. But there are cases, where the enum types should be really uint8_t and uint16_t...how to solve that?
In which case you can not replace any "int8_t" or "int16_t" of the BSS by a single "int" or even "int64_t"? Everything in the BSS should be internal stuff, so if you read 8 bits, you can store it internally in any integer type you wish that fits eight bits. What happens if a new format shows up that decides to have a set of (2^64) possible commands? Will you have to change completely all your code just to change command from a uint8_t to a uint64_t? -Vitor
Vitor Sessak a écrit :
On 08/14/2010 12:01 AM, Sebastian Vater wrote:
Aurelien Jacobs a écrit :
On Fri, Aug 13, 2010 at 10:28:55PM +0200, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
> On 07/13/2010 10:57 PM, Sebastian Vater wrote: > >> Vitor Sessak a écrit : >> >>> On 07/13/2010 09:35 PM, Sebastian Vater wrote: >>> >>>> Vitor Sessak a écrit : >>>> >>>>> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>>>> >>>>>> typedef struct AVSequencerTrackData { >>>>>> [...]
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
The problem is that it seems you intended 'command' to be an enum, but you declared it as an uint8_t instead. You have to declare it as an enum to allow proper type checking (among other things). So for this one, what you want is: enum AVSequencerTrackEffectCommand command;
The same apply to all the enum remarks that you didn't understood.
Ahh ok, thank you very much, I missed that point. But there are cases, where the enum types should be really uint8_t and uint16_t...how to solve that?
In which case you can not replace any "int8_t" or "int16_t" of the BSS by a single "int" or even "int64_t"? Everything in the BSS should be internal stuff, so if you read 8 bits, you can store it internally in any integer type you wish that fits eight bits.
That's true. But let's consider effects, we have not only bit 7 allocated for the packer, but also the synth sound assembly stuff. Having negative value for synth sound assembly code results the normal effect if doing NOT on it, this effectively limits to 7-bit range. Changing this, will not only require changing uint8_t to int but a lot of other internal logic also. Anyway, I see that this is a good idea to extend, but this has to be done carefully. Just saying here, changing uint8_t to int isn't enough. We have also find a solution for, how to handle compression of 16-/32-bit commands, as well as introducing 16-/32-bit synth sound instructions (please note here that negative synth sound instruction code (which is 8-bit) means normal track effect). Anyway, I have plans to extend synth sound instruction set, I have already a idea pack to add a true stack to synth sound assembly language code. But this is all in the future and should be done when the basic stuff works, which means at least full playback of all TCM files.
What happens if a new format shows up that decides to have a set of (2^64) possible commands? Will you have to change completely all your code just to change command from a uint8_t to a uint64_t?
Uhm, if this really happens, I become a monk and move to the next cave and remain there for the rest of life. No, just kidding! All trackers I seen don't even use close to the 7-bit range, TuComposer just becomes close to this, because it a) merges all commands and b) it defines a lot of new on its own. You have to know that practically all trackers represent effects to the user either simply in the A-Z range (this being S3M/IT) which limits to 26 commands, or 0-15 (in hex 0-F represented by MOD/XM). That's why I thought a full 7-bit range is more than enough. Although I have practically allocated most of them for new features, there is still enough place to addition even in this range. If we have to add a new command, we have to change the playback engine anyway, and thus break compatibility. In the same point we can extend the command depth, if that's necessary. The API functions itself are always at least uint32_t in the call parameters, so there will not be external breakage about this. If we ever need extension to 16-bit of commands or like that, we can do this harmlessly since API calls are still 32-bit (ok if we need 64-bit commands, but as said, then I'll become a monk ^^). -- Best regards, :-) Basty/CDGS (-:
On 08/13/2010 10:28 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 09:35 PM, Sebastian Vater wrote: > Vitor Sessak a écrit : >> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>> typedef struct AVSequencerTrackData {
[...]
>> if (instrument == previous_instrument) {...} > > Please note that instrument number 0 also differs whether you > play the > track in once mode (pattern play mode) and the instrument number was > initialized in the previous order. All trackers don't output > sound in > that case, changing this would make this part incompatible.
That's another good point, so I'm fine with instrument==0 meaning last instrument. But if the behavior of setting instrument==0 is different of the behavior of setting instrument==last_instrument, it should be documented somewhere.
> TrackData is the octave, note, effect and effect data for each row, > Track itself defines the structure containing it, i.e. number of > rows, > maybe track name, etc.
Maybe AVSequencerRowData then?
Nice name, will think on it!
So?
AVSequencerTrackRow?
Just to make the hierarchy clearer...agree on this?
yes.
> > This would require extending using 0x80-0xFF command effect which > would > conflict with synth sound handling (see there at the synth sound > instructions).
Ok, let me pose the question differently. Suppose we have an effect foo. In MOD, effect foo is command 0x66 while in XM it is command 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such thing never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
So?
Sorry, don't understand here, what you mean.
I mean that if the choice of 0x66 for AVSEQ_TRACK_EFFECT_CMD_FOO is merely a convention, it should not be reinforced in the comment.
diff --git a/libavsequencer/track.h b/libavsequencer/track.h new file mode 100755 index 0000000..cf131af --- /dev/null +++ b/libavsequencer/track.h @@ -0,0 +1,1636 @@ +/* + * AVSequencer track and pattern 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_TRACK_H +#define AVSEQUENCER_TRACK_H + +#include "libavutil/log.h" +#include "libavformat/avformat.h" +
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
+ /** Effect command data word. */ + uint16_t data; +} AVSequencerTrackEffect; + +/** AVSequencerTrackEffect->note values. */ +enum AVSequencerTrackDataNote { + /** --- */ + AVSEQ_TRACK_DATA_NOTE_NONE = 0, + + /** C-n */ + AVSEQ_TRACK_DATA_NOTE_C = 1, + + /** C#n = Dbn */ + AVSEQ_TRACK_DATA_NOTE_C_SHARP = 2, + + /** D-n */ + AVSEQ_TRACK_DATA_NOTE_D = 3, + + /** D#n = Ebn */ + AVSEQ_TRACK_DATA_NOTE_D_SHARP = 4, + + /** E-n */ + AVSEQ_TRACK_DATA_NOTE_E = 5, + + /** F-n */ + AVSEQ_TRACK_DATA_NOTE_F = 6, + + /** F#n = Gbn */ + AVSEQ_TRACK_DATA_NOTE_F_SHARP = 7, + + /** G-n */ + AVSEQ_TRACK_DATA_NOTE_G = 8, + + /** G#n = Abn */ + AVSEQ_TRACK_DATA_NOTE_G_SHARP = 9, + + /** A-n */ + AVSEQ_TRACK_DATA_NOTE_A = 10, + + /** A#n = Bbn */ + AVSEQ_TRACK_DATA_NOTE_A_SHARP = 11, + + /** B-n = H-n */ + AVSEQ_TRACK_DATA_NOTE_B = 12, + + /** ^^^ = note kill */ + AVSEQ_TRACK_DATA_NOTE_KILL = -1, + + /** ^^- = note off */ + AVSEQ_TRACK_DATA_NOTE_OFF = -2, + + /** === = keyoff note */ + AVSEQ_TRACK_DATA_NOTE_KEYOFF = -3, + + /** -|- = hold delay */ + AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY = -4, + + /** -\- = note fade */ + AVSEQ_TRACK_DATA_NOTE_FADE = -5, + + /** END = pattern end marker */ + AVSEQ_TRACK_DATA_NOTE_END = -16, +};
Why hardcoding values? Why not just use:
/** AVSequencerTrackEffect->note values. */ enum AVSequencerTrackDataNote { /** --- */ AVSEQ_TRACK_DATA_NOTE_NONE,
/** C-n */ AVSEQ_TRACK_DATA_NOTE_C,
/** C#n = Dbn */ AVSEQ_TRACK_DATA_NOTE_C_SHARP,
[...] }
And let the compiler assign whatever numbers it seems fit?
Would require every demuxer/muxer to convert these values if they don't match,
Every format uses the same convention? -Vitor
Vitor Sessak a écrit :
On 08/13/2010 10:28 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 08/07/2010 09:46 PM, Sebastian Vater wrote:
Vitor Sessak a écrit :
On 07/13/2010 10:57 PM, Sebastian Vater wrote:
Vitor Sessak a écrit : > On 07/13/2010 09:35 PM, Sebastian Vater wrote: >> Vitor Sessak a écrit : >>> On 07/11/2010 10:07 PM, Sebastian Vater wrote: >>>> typedef struct AVSequencerTrackData {
[...]
>>> if (instrument == previous_instrument) {...} >> >> Please note that instrument number 0 also differs whether you >> play the >> track in once mode (pattern play mode) and the instrument >> number was >> initialized in the previous order. All trackers don't output >> sound in >> that case, changing this would make this part incompatible. > > That's another good point, so I'm fine with instrument==0 meaning > last > instrument. But if the behavior of setting instrument==0 is > different > of the behavior of setting instrument==last_instrument, it > should be > documented somewhere.
>> TrackData is the octave, note, effect and effect data for each >> row, >> Track itself defines the structure containing it, i.e. number of >> rows, >> maybe track name, etc. > > Maybe AVSequencerRowData then?
Nice name, will think on it!
So?
AVSequencerTrackRow?
Just to make the hierarchy clearer...agree on this?
yes.
Fixed.
>> >> This would require extending using 0x80-0xFF command effect which >> would >> conflict with synth sound handling (see there at the synth sound >> instructions). > > Ok, let me pose the question differently. Suppose we have an effect > foo. In MOD, effect foo is command 0x66 while in XM it is command > 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such > thing > never happens?
This happens very frequently, esp. between MOD/XM<=> S3M/IT. But this is part of the demuxer to convert the origins...
Sure, but then doing as you do:
/** 0x66 - Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
is confusing and misleading in comparison with
/** Do foo to stuff */ #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
So?
Sorry, don't understand here, what you mean.
I mean that if the choice of 0x66 for AVSEQ_TRACK_EFFECT_CMD_FOO is merely a convention, it should not be reinforced in the comment.
Fixed.
diff --git a/libavsequencer/track.h b/libavsequencer/track.h new file mode 100755 index 0000000..cf131af --- /dev/null +++ b/libavsequencer/track.h @@ -0,0 +1,1636 @@ +/* + * AVSequencer track and pattern 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_TRACK_H +#define AVSEQUENCER_TRACK_H + +#include "libavutil/log.h" +#include "libavformat/avformat.h" +
+/** + * Song track effect structure, This structure is actually for one row + * and therefore actually pointed as an array with the amount of + * rows of the whole track. + * 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 AVSequencerTrackEffect { + /** Effect command byte. */ + uint8_t command;
enum...
Again where's the problem? enums are all above.
+ /** Effect command data word. */ + uint16_t data; +} AVSequencerTrackEffect; + +/** AVSequencerTrackEffect->note values. */ +enum AVSequencerTrackDataNote { + /** --- */ + AVSEQ_TRACK_DATA_NOTE_NONE = 0, + + /** C-n */ + AVSEQ_TRACK_DATA_NOTE_C = 1, + + /** C#n = Dbn */ + AVSEQ_TRACK_DATA_NOTE_C_SHARP = 2, + + /** D-n */ + AVSEQ_TRACK_DATA_NOTE_D = 3, + + /** D#n = Ebn */ + AVSEQ_TRACK_DATA_NOTE_D_SHARP = 4, + + /** E-n */ + AVSEQ_TRACK_DATA_NOTE_E = 5, + + /** F-n */ + AVSEQ_TRACK_DATA_NOTE_F = 6, + + /** F#n = Gbn */ + AVSEQ_TRACK_DATA_NOTE_F_SHARP = 7, + + /** G-n */ + AVSEQ_TRACK_DATA_NOTE_G = 8, + + /** G#n = Abn */ + AVSEQ_TRACK_DATA_NOTE_G_SHARP = 9, + + /** A-n */ + AVSEQ_TRACK_DATA_NOTE_A = 10, + + /** A#n = Bbn */ + AVSEQ_TRACK_DATA_NOTE_A_SHARP = 11, + + /** B-n = H-n */ + AVSEQ_TRACK_DATA_NOTE_B = 12, + + /** ^^^ = note kill */ + AVSEQ_TRACK_DATA_NOTE_KILL = -1, + + /** ^^- = note off */ + AVSEQ_TRACK_DATA_NOTE_OFF = -2, + + /** === = keyoff note */ + AVSEQ_TRACK_DATA_NOTE_KEYOFF = -3, + + /** -|- = hold delay */ + AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY = -4, + + /** -\- = note fade */ + AVSEQ_TRACK_DATA_NOTE_FADE = -5, + + /** END = pattern end marker */ + AVSEQ_TRACK_DATA_NOTE_END = -16, +};
Why hardcoding values? Why not just use:
/** AVSequencerTrackEffect->note values. */ enum AVSequencerTrackDataNote { /** --- */ AVSEQ_TRACK_DATA_NOTE_NONE,
/** C-n */ AVSEQ_TRACK_DATA_NOTE_C,
/** C#n = Dbn */ AVSEQ_TRACK_DATA_NOTE_C_SHARP,
[...] }
And let the compiler assign whatever numbers it seems fit?
Would require every demuxer/muxer to convert these values if they don't match,
Every format uses the same convention?
For the positive notes, this is the case, as far as I know. For the negative special note effects it differs. But most use negative values, too, if I remember correctly, so you know it's a special note by just checking for < 0. Anyway, I fixed this by removing the 1 to 12 assignments and just set NONE to 0, the compiler automatically always adds one in a subsequent enum, right? This is required, since the player uses a lut for calculating the target frequency which is base_freq * (octave - 4) * 2^((note - 1)/12). -- Best regards, :-) Basty/CDGS (-:
participants (4)
-
Aurelien Jacobs -
Michael Niedermayer -
Sebastian Vater -
Vitor Sessak