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

[Vitor Sessak]

On 07/13/2010 09:49 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/11/2010 10:05 PM, Sebastian Vater wrote:
>>
>>> /*
>>>   * AVSequencer order list and data management
>>>   * Copyright (c) 2010 Sebastian Vater<cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg at public.gmane.org>
>>>   *
>>>   * This file is part of FFmpeg.
>>>   *
>>>   * FFmpeg is free software; you can redistribute it and/or
>>>   * modify it under the terms of the GNU Lesser General Public
>>>   * License as published by the Free Software Foundation; either
>>>   * version 2.1 of the License, or (at your option) any later version.
>>>   *
>>>   * FFmpeg is distributed in the hope that it will be useful,
>>>   * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>   * Lesser General Public License for more details.
>>>   *
>>>   * You should have received a copy of the GNU Lesser General Public
>>>   * License along with FFmpeg; if not, write to the Free Software
>>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>> 02110-1301 USA
>>>   */
>>>
>>> #ifndef AVSEQUENCER_ORDER_H
>>> #define AVSEQUENCER_ORDER_H
>>>
>>> #include "libavsequencer/track.h"
>>>
>>> /**
>>>   * Song order list structure, This structure is actually for one channel
>>>   * and therefore actually pointed as an array with size of number of
>>>   * host channels.
>>
>> Wouldn't it be better named AVSequencerChannelData?
>
> It is only channel based if you import from a tracker which separates
> channels for each track, but most trackers don't do this, they have just
> one entry for all the channels, since they only know synchronized channels.
>
>>
>>>   * New fields can be added to the end with minor version bumps.
>>>   * Removal, reordering and changes to existing fields require a major
>>>   * version bump.
>>>   */
>>> typedef struct AVSequencerOrderList {
>>>      /** Array of pointers containing all order list data used by this
>>>        channel.  */
>>>      AVSequencerOrderData **order_data;
>>
>> Please replace in the comment "order list data" for a brief
>> description of what it means. And BTW, is this an "order _list_ data"?
>
> Actually it is the data of the order list, order_data contains all order
> list entries like AVSequencerTrackData contains all the rows of
> AVSequencerTrack.

Is it the list of the rows in chronological order? Is it the list of 
notes in alphabetical order? Also, if I understand well, it is an array 
of linked lists. Why?

>>>      /** Number of order list data used for this channel.  */
>>>      uint16_t orders;
>>>
>>>      /** Number of order list data entries to use for this channel.  */
>>>      uint16_t length;
>>>
>>>      /** Repeat start order list data number for this channel.  */
>>>      uint16_t rep_start;
>>>
>>>      /** Volume level for this channel (defaults to 255).  */
>>>      uint8_t volume;
>>> #define AVSEQ_ORDER_LIST_VOLUME 255
>>>
>>>      /** Sub-volume level for this channel. This is basically channel
>>>         volume divided by 256, but the sub-volume doesn't account
>>>         into actual mixer output (defaults 0).  */
>>>      uint8_t sub_volume;
>>> #define AVSEQ_ORDER_LIST_SUB_VOLUME 0
>>>
>>>      /** Stereo track panning level for this channel (defaults to
>>>         -128 = central stereo track panning).  */
>>>      int8_t track_panning;
>>> #define AVSEQ_ORDER_LIST_TRACK_PAN  -128
>>>
>>>      /** Stereo track sub-panning level for this channel. This is
>>>         basically track panning divided by 256, but the sub-panning
>>>         doesn't account into actual mixer output (defaults 0).  */
>>>      uint8_t track_sub_panning;
>>> #define AVSEQ_ORDER_LIST_TRACK_SUB_PAN  -128
>>>
>>>      /** Stereo panning level for this channel (defaults to
>>>         -128 = central stereo panning).  */
>>>      int8_t channel_panning;
>>> #define AVSEQ_ORDER_LIST_PANNING    -128
>>>
>>>      /** Stereo sub-panning level for this channel. This is
>>>         basically channel panning divided by 256, but the sub-panning
>>>         doesn't account into actual mixer output (defaults 0).  */
>>>      uint8_t channel_sub_panning;
>>> #define AVSEQ_ORDER_LIST_SUB_PANNING    0
>>
>>>      /** Compatibility flags for playback. There are rare cases
>>>         where order handling can not be mapped into internal
>>>         playback engine and have to be handled specially. For
>>>         each order list which needs this, this will define new
>>>         flags which tag the player to handle it to that special
>>>         way.  */
>>>      uint8_t compat_flags;
>>
>> I suppose this is not unused ATM?
>
> Fixed.
>
>>
>>>      /** Order list playback flags. Some sequencers feature
>>>         surround panning or allow initial muting. which has to
>>>         be taken care specially in the internal playback engine.
>>>         Also sequencers differ in how they handle slides.  */
>>>      uint8_t flags;
>>> #define AVSEQ_ORDER_LIST_FLAG_CHANNEL_SURROUND  0x01 ///<  Initial
>>> channel surround instead of stereo panning
>>> #define AVSEQ_ORDER_LIST_FLAG_TRACK_SURROUND    0x02 ///<  Initial
>>> track surround instead of stereo panning
>>> #define AVSEQ_ORDER_LIST_FLAG_MUTED             0x04 ///<  Initial
>>> muted channel
>>>
>>> } AVSequencerOrderList;
>>>
>>> /**
>>>   * Song order list data structure, this contains actual order list data.
>>>   * New fields can be added to the end with minor version bumps.
>>>   * Removal, reordering and changes to existing fields require a major
>>>   * version bump.
>>>   */
>>> typedef struct AVSequencerOrderData {
>>>      /** AVSequencerTrack pointer to track which should be played.  */
>>>      AVSequencerTrack *track;
>>
>>>      /** Next order list data pointer if seeking forward one frame.  */
>>>      AVSequencerOrderData *next_pos;
>>>
>>>      /** Previous order list data pointer if seeking backward one
>>>         frame.  */
>>>      AVSequencerOrderData *prev_pos;
>>
>> These do not look to belong to the BSS.
>
> They are used to allow the composer to define alternative order
> sequences in case of forward and backward seeking. These alternatives
> can pre-initialize note data in that case. This is a very rarely used
> feature, but avoids the problem that instruments are missing if you skip
> one channel.
>
> Example:
> We have a track playing a new instrument at row 60. If the user skips
> this track before arriving row 60, it will not be played at all, thus
> causing silence. Now consider the normal next row, would change data
> here or even has a complete empty row (but expecting to play a long
> looped instrument), in that case it would confuse the actual output.
> These pointers allow the composer to initialize that in case.

Is this about an human user seeking the file he is playing or a tracker 
formats that specify seeking commands?

>>>      /** Tempo change or zero to skip tempo change.  */
>>>      uint16_t tempo;
>>
>> Cryptic comment. This is the timestamp where the tempo change? An
>> index to a list a tempo changing structures? The instrument number
>> that has a different tempo?
>
> Fixed.
>
>>
>>>      /** Played nesting level (GoSub command maximum nesting depth).  */
>>>      uint16_t played;
>>
>> Again, BSS?
>
> Would require duplicating each track information in the player and
> therefore add a lot of unnecessary complexity.

So that means that the player _writes_ to this struct? This seems not 
optimal design in a modularity point of view. IMHO, everything that is 
feeded to the player should be read-only and the player should only 
write in his own context structure.

>>>      /** Track volume (this overrides settings in AVSequencerTrack).
>>>         To enable this, the flag AVSEQ_ORDER_DATA_FLAG_SET_VOLUME
>>>         must be set in flags.  */
>>>      uint8_t volume;
>>
>> This is ugly, why not making it a signed int and summing it to
>> AVSequencerTrack.volume no matter what?
>
> This again would seriously break compatibility with almost any tracker.
> Trackers either set volume by track data XOR by order data, never both.
> It is not a volume transpose but a set volume command.

What is the difference between a track volume and a order data volume?
If they are the same, since only one or the other is specified per file, 
why not using a single struct field for it, without caring where in the 
file it is defined (and the module loader should know where to look)?

-Vitor