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

[Sebastian Vater]

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