[FFmpeg-soc] [soc] libavsequencer [PATCH 04/08] Track / pattern handling public API header file.

[Sebastian Vater]

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 (-: