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

[Sebastian Vater]

Vitor Sessak a écrit :
> On 07/13/2010 10:57 PM, Sebastian Vater wrote:
>>>
>>> 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.

I forgot to mention one point, the octave number is also used by the
keyboard definition table, which is hard-coded to a size 120 entries (10
octaves * 12 notes per octave). Allowing values outside this will cause
access to uninitialized memory. Fixed the documentation to mention this,
though.


>>
>>>> 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!

Still unsure about this yet, what about AVSequencerTrackRowData? This
clarifys that this structure belongs to tracks and not sth. else.

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

The effect numbers are supposed also to editable by the user (in
trackers you enter them has hex values directly, some trackers however,
do use alphabetical ordering, A-Z like S3M/IT), but internally they're
hex numbers.
Due to the large number of effects, I decided as a base approach to
display the hex number there, too.

I will however, remove the duplicate mentions, when I convert that to
enum tonight, ok?

>>
>> // 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.

Fixed by removing it.

-- 

Best regards,
                   :-) Basty/CDGS (-:

-------------- next part --------------
A non-text attachment was scrubbed...
Name: track.h_20100715.patch
Type: text/x-diff
Size: 90432 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100715/f5698d3f/attachment.patch>