[FFmpeg-soc] [soc] libavsequencer [PATCH 04/08] Track / pattern handling public API header file.
Sebastian Vater
cdgs.basty at googlemail.com
Sat Aug 7 21:46:14 CEST 2010
Vitor Sessak a écrit :
> 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.
Hi I have excellent news!
libavsequencer now flawlessly integrates into FFmpeg, just check out my
latest git. Please do a git pull --rebase, Stefano had problems without
using it.
Here are the track.[ch] part of the BSS to review.
This version compiles perfectly.
--
Best regards,
:-) Basty/CDGS (-:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: track.c_20100807.patch
Type: text/x-patch
Size: 3089 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100807/e413b467/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: track.h_20100807.patch
Type: text/x-patch
Size: 82624 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100807/e413b467/attachment-0001.bin>
More information about the FFmpeg-soc
mailing list