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

[Vitor Sessak]

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