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

[Vitor Sessak]

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