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