[FFmpeg-soc] [soc] libavsequencer [PATCH 03/08] Order list public API header file.

[Vitor Sessak]

On 08/13/2010 10:19 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 08/07/2010 09:44 PM, Sebastian Vater wrote:
>>
>> [...]
>>
>>> 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 order.[ch] part of the BSS to review.
>>>
>>> This version compiles perfectly.
>>
>>>
>>> diff --git a/libavsequencer/order.h b/libavsequencer/order.h
>>> new file mode 100644
>>> index 0000000..70bcf0b
>>> --- /dev/null
>>> +++ b/libavsequencer/order.h
>>> @@ -0,0 +1,193 @@
>>
>> [...]
>>
>>> +typedef struct AVSequencerOrderData {
>>> +    /**
>>> +     * information on struct for av_log
>>> +     * - set by avseq_alloc_context
>>> +     */
>>> +    const AVClass *av_class;
>>> +
>>> +    /** Metadata information: Original order entry file name, order
>>> +     *  entry name, artist and comment.  */
>>> +    AVMetadata *metadata;
>>
>>> +    /** AVSequencerTrack pointer to track which should be played.  */
>>> +    AVSequencerTrack *track;
>>
>> If there is one and only one AVSequencerTrack per
>> AVSequencerOrderData, why not having it directly as part of the
>> struct, i.e.:
>>
>>      /** AVSequencerTrack pointer to track which should be played.  */
>>      AVSequencerTrack track;
>>
>> ?
>>
>
> Would be possible though, but remember, the track stuff is supposed to
> be editable and it's easier to just to replace the pointer instead of
> copying the data all over it.
>
> Also it requires way less memory usage, please note that number of order
> data has to be multiplied by number of channels, i.e. for a 64 channel
> sub-song we have 64 * (order_list->orders) entries, which can quite a
> lot (a lot of modules have order_list->orders beyond 100).

so its fine, duplicating memory is evil.

>>> +    /** Next order list data pointer if seeking forward one frame.  */
>>> +    struct AVSequencerOrderData *next_pos;
>>> +
>>> +    /** Previous order list data pointer if seeking backward one
>>> +       frame.  */
>>> +    struct AVSequencerOrderData *prev_pos;
>>> +
>>> +    /** Number of row to jump to if forward seeking one frame.  */
>>> +    uint16_t next_row;
>>> +
>>> +    /** Number of row to jump to if backward seeking one frame.  */
>>> +    uint16_t prev_row;
>>> +
>>> +    /** Beginning row for this track. If this is a track
>>> synchronization
>>> +       point, the high byte is interpreted as the first track number
>>> +       to be synchronized with and the low byte as the second track
>>> +       number or for all channels when all 4 tracks are 0.  */
>>> +    uint16_t first_row;
>>> +
>>> +    /** Last row for this track. If this is a track synchronization
>>> +       point, the high byte is interpreted as the third track number
>>> +       to be synchronized with and the low byte as the fourth track
>>> +       number or for all channels when all 4 tracks are 0.
>>> +       If last row is set to 65535 in non synchronization mode,
>>> +       the last row is always taken from AVSequencerTrack.  */
>>> +    uint16_t last_row;
>>
>>> +    /** Order list data playback flags. Some sequencers feature
>>> +       special end markers or even different playback routes for
>>> +       different playback modules (one-shot and repeat mode
>>> +       playback), mark synchronization points or temporary
>>> +       change volume), which has to be taken care specially
>>> +       in the internal playback engine.  */
>>> +    uint8_t flags;
>>
>> enum...
>
> Hmm, what you mean with this, it is already enum, right?
>
>>
>>> +    /** Relative note transpose for full track. Allows playing several
>>> +       tracks some half-tones up/down.  */
>>> +    int8_t transpose;
>>
>> Comment unclear. Is this a flag? What does it means if transpose == -23?
>
> It means to adjust -23 halftones to each track data instrument / note
> pair encountered. Let's say you have in the pattern:
> C-5 01 .. ...

So it is really missing in the comment to say this quantity is expressed 
in halftone units.

> Since -24 would be 2 octaves back (i.e. C-3), -23 would playback as:
> C#3 01 .. ...
>
> It's simply globally applied to the whole track which is used by this
> order entry.
>
> This is, e.g. a feature heavily used by FutureComposer.
>
>>
>>> +    /** Instrument transpose. All instruments will be relatively
>>> +       mapped to this if this is non-zero.  */
>>> +    int16_t instr_transpose;
>>
>> Why an int16_t for 0/1?
>
> This is not a 0/1,

Again, missing unit. Maybe something like "Number of half-tones to 
transpose the instrument." or something like that.

> this means adding instr_transpose to the instrument
> value.
> Considering the example above:
> C-5 01 .. ...
>
> and instr_transpose = 4 then it will play:
> C-5 05 .. ...
>
> i.e. instrument number 5 instead of 1.
>
> Again, this is heavily used by FutureComposer.
>
>>
>>> +    /** Tempo change or zero to skip tempo change. A tempo value of
>>> +       zero would be zero, since that would mean literally execute
>>> +       unlimited rows and tracks in just one tick.  */
>>> +    uint16_t tempo;
>>
>>> +    /** Played nesting level (GoSub command maximum nesting depth).  */
>>> +    uint16_t played;
>>> +
>>> +    /** Track volume (this overrides settings in AVSequencerTrack).
>>> +       To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME
>>> +       must be set in flags. This allows have a basic default track
>>> +       volume by still allowing to override the track volume in case
>>> +       the track is used multiple times, e.g. for creating echoes.  */
>>> +    uint8_t volume;
>>> +
>>> +    /** Track sub-volume. This is basically track volume
>>> +       divided by 256, but the sub-volume doesn't account
>>> +       into actual mixer output (this overrides AVSequencerTrack).  */
>>> +    uint8_t sub_volume;
>>> +} AVSequencerOrderData;
>>> +
>>> +/** AVSequencerOrderList->flags bitfield.  */
>>> +enum AVSequencerOrderListFlags {
>>
>>> +    AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND  = 0x01, ///<  Initial
>>> channel surround instead of stereo panning
>>> +    AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND    = 0x02, ///<  Initial
>>> track surround instead of stereo panning
>>> +    AVSEQ_ORDER_LIST_FLAG_MUTED             = 0x04, ///<  Initial
>>> muted channel
>>> +};
>>
>> Did you mean "initially"?
>
> More like:
> Use initial ...

indeed, "initial" is better.

-Vitor