On 07/11/2010 10:04 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/07/2010 10:46 PM, Sebastian Vater wrote:
>>>
>>> diff --git a/libavsequencer/song.h b/libavsequencer/song.h
> /*
>  * AVSequencer sub-song management
>  * Copyright (c) 2010 Sebastian Vater <cdgs.basty@googlemail.com>
>  *
>  * 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_SONG_H
> #define AVSEQUENCER_SONG_H
>
> #include "libavformat/avformat.h"
> #include "libavsequencer/avsequencer.h"
> #include "libavsequencer/order.h"
> #include "libavsequencer/track.h"
> #include "libavsequencer/player.h"
>
> /**
>  * Sequencer song structure.
>  * 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 AVSequencerSong {
>     /** Metadata information: Original sub-song file name, sub-song
>      * title, song message, artist, genre, album, begin and finish
>      * date of composition and comment.  */
>     AVMetadata *metadata;

>     /** AVSequencerPlayerGlobals pointer to global channel data.  */
>     AVSequencerPlayerGlobals *global_data;
>
>     /** AVSequencerPlayerHostChannel pointer to host channel data.  */
>     AVSequencerPlayerHostChannel *channel_data;

Player?

>     /** AVSequencerOrderList pointer to list of order data.  */
>     AVSequencerOrderList *order_list;

Why not here an array of pointers like everywhere else?

>     /** Array of pointers containing all tracks for this sub-song.  */
>     AVSequencerTrack **track_list;
>
>     /** Duration of this sub-song, in AV_TIME_BASE fractional
>        seconds.  */
>     uint64_t duration;
>
>     /** Number of tracks attached to this sub-song.  */
>     uint16_t tracks;

>     /** Stack size, i.e. maximum recursion depth of GoSub command which
>        defaults to 4.  */
>     uint16_t gosub_stack_size;
> #define AVSEQ_SONG_GOSUB_STACK  4

Doesn't this depend on the player implementation? Or is it 
format-specific? Or is it read from the file?

>     /** Stack size, i.e. maximum recursion depth of the pattern loop
>        command, which defaults to 1 to imitate most trackers.  */
>     uint16_t loop_stack_size;
> #define AVSEQ_SONG_PATTERN_LOOP_STACK   1

Again, is this specified in the file?

>     /** Compatibility flags for playback. There are rare cases
>        where effect handling can not be mapped into internal
>        playback engine and have to be handled specially. For
>        each sub-song which needs this, this will define new
>        flags which tag the player to handle it to that special
>        way.  */
>     uint8_t compat_flags;
> #define AVSEQ_SONG_COMPAT_FLAG_SYNC             0x01 ///< Tracks are synchronous (linked together, pattern based)
> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_LOOP      0x02 ///< Global pattern loop memory
> #define AVSEQ_SONG_COMPAT_FLAG_AMIGA_LIMITS     0x04 ///< Enforce AMIGA sound hardware limits (portamento)
> #define AVSEQ_SONG_COMPAT_FLAG_OLD_VOLUMES      0x08 ///< All volume related commands range from 0-64 instead of 0-255
> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_NEW_ONLY  0x10 ///< Global volume/panning changes affect new notes only (S3M)
>
>     /** Song playback flags. Some sequencers use a totally
>        different timing scheme 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_SONG_FLAG_LINEAR_FREQ_TABLE   0x01 ///< Use linear instead of Amiga frequency table
> #define AVSEQ_SONG_FLAG_SPD                 0x02 ///< Use SPD (OctaMED style) timing instead of BpM
> #define AVSEQ_SONG_FLAG_MONO                0x04 ///< Use mono instead of stereo output
> #define AVSEQ_SONG_FLAG_SURROUND            0x08 ///< Initial global surround instead of stereo panning
>
>     /** Maximum number of host channels, as edited in the track view.
>        to be allocated and usable for order list (defaults to 16).  */
>     uint16_t channels;
> #define AVSEQ_SONG_CHANNELS     16
> #define AVSEQ_SONG_CHANNELS_MIN 1
> #define AVSEQ_SONG_CHANNELS_MAX 256
>
>     /** Initial number of frames per row, i.e. sequencer tempo
>        (defaults to 6 as in most tracker formats).  */
>     uint16_t frames;
> #define AVSEQ_SONG_FRAMES   6
>
>     /** Initial speed multiplier, i.e. nominator which defaults
>        to disabled = 0.  */
>     uint8_t speed_mul;
> #define AVSEQ_SONG_SPEED_MUL    0
>
>     /** Initial speed divider, i.e. denominator which defaults
>        to disabled = 0.  */
>     uint8_t speed_div;
> #define AVSEQ_SONG_SPEED_DIV    0

AVRational?

>     /** Initial MED style SPD speed (defaults to 33 as in
>        OctaMED Soundstudio).  */
>     uint16_t spd_speed;
> #define AVSEQ_SONG_SPD_SPEED    33
>
>     /** Initial number of rows per beat (defaults to 4 rows are a beat).  */
>     uint16_t bpm_tempo;
> #define AVSEQ_SONG_BPM_TEMPO    4
>
>     /** Initial beats per minute speed (defaults to 50 Hz => 125 BpM).  */
>     uint16_t bpm_speed;
> #define AVSEQ_SONG_BPM_SPEED    125

>     /** Minimum and lower limit of number of frames per row
>        (defaults to 1).  */
>     uint16_t frames_min;
> #define AVSEQ_SONG_FRAMES_MIN   1

Again, does this limit depend of the file format or is read directly 
from the file?

>     /** Maximum and upper limit of number of frames per row
>        (defaults to 255).  */
>     uint16_t frames_max;
> #define AVSEQ_SONG_FRAMES_MAX   255
>
>     /** Minimum and lower limit of MED style SPD timing values
>        (defaults to 1).  */
>     uint16_t spd_min;
> #define AVSEQ_SONG_SPD_MIN  1
>
>     /** Maximum and upper limit of MED style SPD timing values
>        (defaults to 255).  */
>     uint16_t spd_max;
> #define AVSEQ_SONG_SPD_MAX  255
>
>     /** Minimum and lower limit of rows per beat timing values
>        (defaults to 1).  */
>     uint16_t bpm_tempo_min;
> #define AVSEQ_SONG_BPM_TEMPO_MIN    1
>
>     /** Maximum and upper limit of rows per beat timing values
>        (defaults to 255).  */
>     uint16_t bpm_tempo_max;
> #define AVSEQ_SONG_BPM_TEMPO_MAX    255
>
>     /** Minimum and lower limit of beats per minute timing values
>        (defaults to 1).  */
>     uint16_t bpm_speed_min;
> #define AVSEQ_SONG_BPM_SPEED_MIN    1
>
>     /** Maximum and upper limit of beats per minute timing values
>        (defaults to 255).  */
>     uint16_t bpm_speed_max;
> #define AVSEQ_SONG_BPM_SPEED_MAX    255

Same for those.

>     /** Global volume of this sub-song. All other volume related
>        commands are scaled by this (defaults to 255 = no scaling).  */
>     uint8_t global_volume;
> #define AVSEQ_SONG_VOLUME   255
>
>     /** Global sub-volume of this sub-song. This is basically
>        volume divided by 256, but the sub-volume doesn't account
>        into actual mixer output (defaults to 0).  */
>     uint8_t global_sub_volume;
> #define AVSEQ_SONG_SUB_VOLUME   0
>
>     /** Global panning of this sub-song. All other panning related
>        commands are scaled by this stereo separation factor
>        (defaults to 0 which means full stereo separation).  */
>     uint8_t global_panning;
> #define AVSEQ_SONG_PANNING  0
>
>     /** Global sub-panning of this sub-song. This is basically
>        panning divided by 256, but the sub-panning doesn't account
>        into actual mixer output (defaults to 0).  */
>     uint8_t global_sub_panning;
> #define AVSEQ_SONG_SUB_PANNING  0

Again, a file can specify those, no? Or they are parameters for the player?

-Vitor

On 07/13/2010 10:01 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/11/2010 10:04 PM, Sebastian Vater wrote:
>>> Vitor Sessak a écrit :
>>>> On 07/07/2010 10:46 PM, Sebastian Vater wrote:
>>>>>
>>>>> diff --git a/libavsequencer/song.h b/libavsequencer/song.h
>>> /*
>>>   * AVSequencer sub-song management
>>>   * Copyright (c) 2010 Sebastian Vater<cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@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_SONG_H
>>> #define AVSEQUENCER_SONG_H
>>>
>>> #include "libavformat/avformat.h"
>>> #include "libavsequencer/avsequencer.h"
>>> #include "libavsequencer/order.h"
>>> #include "libavsequencer/track.h"
>>> #include "libavsequencer/player.h"
>>>
>>> /**
>>>   * Sequencer song structure.
>>>   * 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 AVSequencerSong {
>>>      /** Metadata information: Original sub-song file name, sub-song
>>>       * title, song message, artist, genre, album, begin and finish
>>>       * date of composition and comment.  */
>>>      AVMetadata *metadata;
>>
>>>      /** AVSequencerPlayerGlobals pointer to global channel data.  */
>>>      AVSequencerPlayerGlobals *global_data;
>>>
>>>      /** AVSequencerPlayerHostChannel pointer to host channel data.  */
>>>      AVSequencerPlayerHostChannel *channel_data;
>>
>> Player?
>
> Can be moved to AVSequencerPlayerGlobals but would make some of the
> playback code requiring more accesses, but I will fix this!
>
>>
>>
>>>      /** AVSequencerOrderList pointer to list of order data.  */
>>>      AVSequencerOrderList *order_list;
>>
>> Why not here an array of pointers like everywhere else?
>
> There is only one order_list per sub-song, but multiple order list entries.
>
>>
>>>      /** Array of pointers containing all tracks for this sub-song.  */
>>>      AVSequencerTrack **track_list;
>>>
>>>      /** Duration of this sub-song, in AV_TIME_BASE fractional
>>>         seconds.  */
>>>      uint64_t duration;
>>>
>>>      /** Number of tracks attached to this sub-song.  */
>>>      uint16_t tracks;
>>
>>>      /** Stack size, i.e. maximum recursion depth of GoSub command which
>>>         defaults to 4.  */
>>>      uint16_t gosub_stack_size;
>>> #define AVSEQ_SONG_GOSUB_STACK  4
>>
>> Doesn't this depend on the player implementation? Or is it
>> format-specific? Or is it read from the file?
>
> GOSUB is a TuComposer only feature right now. I thought this to be a
> nice default value, for creating a new sub-song.

Ok, but does the player need to know this value or it could just fail if 
the song use more recursion depth than the player support?

>>>      /** Stack size, i.e. maximum recursion depth of the pattern loop
>>>         command, which defaults to 1 to imitate most trackers.  */
>>>      uint16_t loop_stack_size;
>>> #define AVSEQ_SONG_PATTERN_LOOP_STACK   1
>>
>> Again, is this specified in the file?
>
> This is, however, is the standard (and even only possible value) for all
> non-TuComposer trackers, no trackers otherwise allow nesting of the
> pattern loop command.

same.

>>>      /** Compatibility flags for playback. There are rare cases
>>>         where effect handling can not be mapped into internal
>>>         playback engine and have to be handled specially. For
>>>         each sub-song which needs this, this will define new
>>>         flags which tag the player to handle it to that special
>>>         way.  */
>>>      uint8_t compat_flags;
>>> #define AVSEQ_SONG_COMPAT_FLAG_SYNC             0x01 ///<  Tracks are
>>> synchronous (linked together, pattern based)
>>> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_LOOP      0x02 ///<  Global
>>> pattern loop memory
>>> #define AVSEQ_SONG_COMPAT_FLAG_AMIGA_LIMITS     0x04 ///<  Enforce
>>> AMIGA sound hardware limits (portamento)
>>> #define AVSEQ_SONG_COMPAT_FLAG_OLD_VOLUMES      0x08 ///<  All volume
>>> related commands range from 0-64 instead of 0-255
>>> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_NEW_ONLY  0x10 ///<  Global
>>> volume/panning changes affect new notes only (S3M)
>>>
>>>      /** Song playback flags. Some sequencers use a totally
>>>         different timing scheme 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_SONG_FLAG_LINEAR_FREQ_TABLE   0x01 ///<  Use linear
>>> instead of Amiga frequency table
>>> #define AVSEQ_SONG_FLAG_SPD                 0x02 ///<  Use SPD
>>> (OctaMED style) timing instead of BpM
>>> #define AVSEQ_SONG_FLAG_MONO                0x04 ///<  Use mono
>>> instead of stereo output
>>> #define AVSEQ_SONG_FLAG_SURROUND            0x08 ///<  Initial global
>>> surround instead of stereo panning
>>>
>>>      /** Maximum number of host channels, as edited in the track view.
>>>         to be allocated and usable for order list (defaults to 16).  */
>>>      uint16_t channels;
>>> #define AVSEQ_SONG_CHANNELS     16
>>> #define AVSEQ_SONG_CHANNELS_MIN 1
>>> #define AVSEQ_SONG_CHANNELS_MAX 256
>>>
>>>      /** Initial number of frames per row, i.e. sequencer tempo
>>>         (defaults to 6 as in most tracker formats).  */
>>>      uint16_t frames;
>>> #define AVSEQ_SONG_FRAMES   6
>>>
>>>      /** Initial speed multiplier, i.e. nominator which defaults
>>>         to disabled = 0.  */
>>>      uint8_t speed_mul;
>>> #define AVSEQ_SONG_SPEED_MUL    0
>>>
>>>      /** Initial speed divider, i.e. denominator which defaults
>>>         to disabled = 0.  */
>>>      uint8_t speed_div;
>>> #define AVSEQ_SONG_SPEED_DIV    0
>>
>> AVRational?
>
> Would be an idea, though. But would make the track effect manipulating
> with this incompatible and conversions required. So for sake of
> simplicity, I kept them all the same.
>
>
>>
>>>      /** Initial MED style SPD speed (defaults to 33 as in
>>>         OctaMED Soundstudio).  */
>>>      uint16_t spd_speed;
>>> #define AVSEQ_SONG_SPD_SPEED    33
>>>
>>>      /** Initial number of rows per beat (defaults to 4 rows are a
>>> beat).  */
>>>      uint16_t bpm_tempo;
>>> #define AVSEQ_SONG_BPM_TEMPO    4
>>>
>>>      /** Initial beats per minute speed (defaults to 50 Hz =>  125
>>> BpM).  */
>>>      uint16_t bpm_speed;
>>> #define AVSEQ_SONG_BPM_SPEED    125
>>
>>>      /** Minimum and lower limit of number of frames per row
>>>         (defaults to 1).  */
>>>      uint16_t frames_min;
>>> #define AVSEQ_SONG_FRAMES_MIN   1
>>
>> Again, does this limit depend of the file format or is read directly
>> from the file?
>
> Frames (=tempo) 0 is allocated for mark song end, which does not make
> sense as a start tempo in sub-songs, which would mean that the whole
> sub-song would initialized with song end at the very beginning.
>
> Allowing tempo 0 is like division by zero, it does not make sense,
> because otherwise infinite rows and tracks would be processed in just
> one tick.

Yes, so why is this a variable not a define? What happens with the 
player if a file specifies frames_min == 2 and this field is erroneously 
initialized frames_min == 1?

>>
>>>      /** Maximum and upper limit of number of frames per row
>>>         (defaults to 255).  */
>>>      uint16_t frames_max;
>>> #define AVSEQ_SONG_FRAMES_MAX   255
>>>
>>>      /** Minimum and lower limit of MED style SPD timing values
>>>         (defaults to 1).  */
>>>      uint16_t spd_min;
>>> #define AVSEQ_SONG_SPD_MIN  1
>>>
>>>      /** Maximum and upper limit of MED style SPD timing values
>>>         (defaults to 255).  */
>>>      uint16_t spd_max;
>>> #define AVSEQ_SONG_SPD_MAX  255
>>>
>>>      /** Minimum and lower limit of rows per beat timing values
>>>         (defaults to 1).  */
>>>      uint16_t bpm_tempo_min;
>>> #define AVSEQ_SONG_BPM_TEMPO_MIN    1
>>>
>>>      /** Maximum and upper limit of rows per beat timing values
>>>         (defaults to 255).  */
>>>      uint16_t bpm_tempo_max;
>>> #define AVSEQ_SONG_BPM_TEMPO_MAX    255
>>>
>>>      /** Minimum and lower limit of beats per minute timing values
>>>         (defaults to 1).  */
>>>      uint16_t bpm_speed_min;
>>> #define AVSEQ_SONG_BPM_SPEED_MIN    1
>>>
>>>      /** Maximum and upper limit of beats per minute timing values
>>>         (defaults to 255).  */
>>>      uint16_t bpm_speed_max;
>>> #define AVSEQ_SONG_BPM_SPEED_MAX    255
>>
>> Same for those.
>
> Would overflow multiply in playback time calculation for larger
> values...and also does not make sense, see track effects, they all set
> 8-bit values only, if we would allow a higher speed here, we could never
> change the speed values which are larger than 255.

same.

>>
>>>      /** Global volume of this sub-song. All other volume related
>>>         commands are scaled by this (defaults to 255 = no scaling).  */
>>>      uint8_t global_volume;
>>> #define AVSEQ_SONG_VOLUME   255
>>>
>>>      /** Global sub-volume of this sub-song. This is basically
>>>         volume divided by 256, but the sub-volume doesn't account
>>>         into actual mixer output (defaults to 0).  */
>>>      uint8_t global_sub_volume;
>>> #define AVSEQ_SONG_SUB_VOLUME   0
>>>
>>>      /** Global panning of this sub-song. All other panning related
>>>         commands are scaled by this stereo separation factor
>>>         (defaults to 0 which means full stereo separation).  */
>>>      uint8_t global_panning;
>>> #define AVSEQ_SONG_PANNING  0
>>>
>>>      /** Global sub-panning of this sub-song. This is basically
>>>         panning divided by 256, but the sub-panning doesn't account
>>>         into actual mixer output (defaults to 0).  */
>>>      uint8_t global_sub_panning;
>>> #define AVSEQ_SONG_SUB_PANNING  0
>>
>> Again, a file can specify those, no? Or they are parameters for the
>> player?
>
> Yes, although TuComposer itself is the only one now who does support
> this, after extending from 8-bit to 16-bit I have had to find an usage
> for the lower 8-bits of the data word of track effects, these allow
> setting it.
>
> But remember again, all stuff in
> module.h/song.h/order.h/track.h/instr.h/sample.h/synth.h is supposed to
> be changed by editors, too. Only player.h is mostly read-only for
> external applications.

I don't understand this point. What can an editor do that cannot be read 
from a file? In which struct should I set the player parameters that are 
independent of the song (i.e, some global volume to scale the song 
global volume, or if the player should output floats or s16le or s32le)? 
It does not belongs to the BSS anyway...

-Vitor

Vitor Sessak a écrit :
> On 07/13/2010 10:01 PM, Sebastian Vater wrote:
>> Vitor Sessak a écrit :
>>> On 07/11/2010 10:04 PM, Sebastian Vater wrote:
>>>> Vitor Sessak a écrit :
>>>>> On 07/07/2010 10:46 PM, Sebastian Vater wrote:
>>>>>>
>>>>>> diff --git a/libavsequencer/song.h b/libavsequencer/song.h
>>>> /*
>>>>   * AVSequencer sub-song management
>>>>   * Copyright (c) 2010 Sebastian
>>>> Vater<cdgs.basty-gM/Ye1E23mwN+BqQ9rBEUg@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_SONG_H
>>>> #define AVSEQUENCER_SONG_H
>>>>
>>>> #include "libavformat/avformat.h"
>>>> #include "libavsequencer/avsequencer.h"
>>>> #include "libavsequencer/order.h"
>>>> #include "libavsequencer/track.h"
>>>> #include "libavsequencer/player.h"
>>>>
>>>> /**
>>>>   * Sequencer song structure.
>>>>   * 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 AVSequencerSong {
>>>>      /** Metadata information: Original sub-song file name, sub-song
>>>>       * title, song message, artist, genre, album, begin and finish
>>>>       * date of composition and comment.  */
>>>>      AVMetadata *metadata;
>>>
>>>>      /** AVSequencerPlayerGlobals pointer to global channel data.  */
>>>>      AVSequencerPlayerGlobals *global_data;
>>>>
>>>>      /** AVSequencerPlayerHostChannel pointer to host channel
>>>> data.  */
>>>>      AVSequencerPlayerHostChannel *channel_data;
>>>
>>> Player?
>>
>> Can be moved to AVSequencerPlayerGlobals but would make some of the
>> playback code requiring more accesses, but I will fix this!
>>
>>>
>>>
>>>>      /** AVSequencerOrderList pointer to list of order data.  */
>>>>      AVSequencerOrderList *order_list;
>>>
>>> Why not here an array of pointers like everywhere else?
>>
>> There is only one order_list per sub-song, but multiple order list
>> entries.
>>
>>>
>>>>      /** Array of pointers containing all tracks for this
>>>> sub-song.  */
>>>>      AVSequencerTrack **track_list;
>>>>
>>>>      /** Duration of this sub-song, in AV_TIME_BASE fractional
>>>>         seconds.  */
>>>>      uint64_t duration;
>>>>
>>>>      /** Number of tracks attached to this sub-song.  */
>>>>      uint16_t tracks;
>>>
>>>>      /** Stack size, i.e. maximum recursion depth of GoSub command
>>>> which
>>>>         defaults to 4.  */
>>>>      uint16_t gosub_stack_size;
>>>> #define AVSEQ_SONG_GOSUB_STACK  4
>>>
>>> Doesn't this depend on the player implementation? Or is it
>>> format-specific? Or is it read from the file?
>>
>> GOSUB is a TuComposer only feature right now. I thought this to be a
>> nice default value, for creating a new sub-song.
>
> Ok, but does the player need to know this value or it could just fail
> if the song use more recursion depth than the player support?
>
>>>>      /** Stack size, i.e. maximum recursion depth of the pattern loop
>>>>         command, which defaults to 1 to imitate most trackers.  */
>>>>      uint16_t loop_stack_size;
>>>> #define AVSEQ_SONG_PATTERN_LOOP_STACK   1
>>>
>>> Again, is this specified in the file?
>>
>> This is, however, is the standard (and even only possible value) for all
>> non-TuComposer trackers, no trackers otherwise allow nesting of the
>> pattern loop command.
>
> same.
>
>>>>      /** Compatibility flags for playback. There are rare cases
>>>>         where effect handling can not be mapped into internal
>>>>         playback engine and have to be handled specially. For
>>>>         each sub-song which needs this, this will define new
>>>>         flags which tag the player to handle it to that special
>>>>         way.  */
>>>>      uint8_t compat_flags;
>>>> #define AVSEQ_SONG_COMPAT_FLAG_SYNC             0x01 ///<  Tracks are
>>>> synchronous (linked together, pattern based)
>>>> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_LOOP      0x02 ///<  Global
>>>> pattern loop memory
>>>> #define AVSEQ_SONG_COMPAT_FLAG_AMIGA_LIMITS     0x04 ///<  Enforce
>>>> AMIGA sound hardware limits (portamento)
>>>> #define AVSEQ_SONG_COMPAT_FLAG_OLD_VOLUMES      0x08 ///<  All volume
>>>> related commands range from 0-64 instead of 0-255
>>>> #define AVSEQ_SONG_COMPAT_FLAG_GLOBAL_NEW_ONLY  0x10 ///<  Global
>>>> volume/panning changes affect new notes only (S3M)
>>>>
>>>>      /** Song playback flags. Some sequencers use a totally
>>>>         different timing scheme 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_SONG_FLAG_LINEAR_FREQ_TABLE   0x01 ///<  Use linear
>>>> instead of Amiga frequency table
>>>> #define AVSEQ_SONG_FLAG_SPD                 0x02 ///<  Use SPD
>>>> (OctaMED style) timing instead of BpM
>>>> #define AVSEQ_SONG_FLAG_MONO                0x04 ///<  Use mono
>>>> instead of stereo output
>>>> #define AVSEQ_SONG_FLAG_SURROUND            0x08 ///<  Initial global
>>>> surround instead of stereo panning
>>>>
>>>>      /** Maximum number of host channels, as edited in the track view.
>>>>         to be allocated and usable for order list (defaults to
>>>> 16).  */
>>>>      uint16_t channels;
>>>> #define AVSEQ_SONG_CHANNELS     16
>>>> #define AVSEQ_SONG_CHANNELS_MIN 1
>>>> #define AVSEQ_SONG_CHANNELS_MAX 256
>>>>
>>>>      /** Initial number of frames per row, i.e. sequencer tempo
>>>>         (defaults to 6 as in most tracker formats).  */
>>>>      uint16_t frames;
>>>> #define AVSEQ_SONG_FRAMES   6
>>>>
>>>>      /** Initial speed multiplier, i.e. nominator which defaults
>>>>         to disabled = 0.  */
>>>>      uint8_t speed_mul;
>>>> #define AVSEQ_SONG_SPEED_MUL    0
>>>>
>>>>      /** Initial speed divider, i.e. denominator which defaults
>>>>         to disabled = 0.  */
>>>>      uint8_t speed_div;
>>>> #define AVSEQ_SONG_SPEED_DIV    0
>>>
>>> AVRational?
>>
>> Would be an idea, though. But would make the track effect manipulating
>> with this incompatible and conversions required. So for sake of
>> simplicity, I kept them all the same.
>>
>>
>>>
>>>>      /** Initial MED style SPD speed (defaults to 33 as in
>>>>         OctaMED Soundstudio).  */
>>>>      uint16_t spd_speed;
>>>> #define AVSEQ_SONG_SPD_SPEED    33
>>>>
>>>>      /** Initial number of rows per beat (defaults to 4 rows are a
>>>> beat).  */
>>>>      uint16_t bpm_tempo;
>>>> #define AVSEQ_SONG_BPM_TEMPO    4
>>>>
>>>>      /** Initial beats per minute speed (defaults to 50 Hz =>  125
>>>> BpM).  */
>>>>      uint16_t bpm_speed;
>>>> #define AVSEQ_SONG_BPM_SPEED    125
>>>
>>>>      /** Minimum and lower limit of number of frames per row
>>>>         (defaults to 1).  */
>>>>      uint16_t frames_min;
>>>> #define AVSEQ_SONG_FRAMES_MIN   1
>>>
>>> Again, does this limit depend of the file format or is read directly
>>> from the file?
>>
>> Frames (=tempo) 0 is allocated for mark song end, which does not make
>> sense as a start tempo in sub-songs, which would mean that the whole
>> sub-song would initialized with song end at the very beginning.
>>
>> Allowing tempo 0 is like division by zero, it does not make sense,
>> because otherwise infinite rows and tracks would be processed in just
>> one tick.
>
> Yes, so why is this a variable not a define? What happens with the
> player if a file specifies frames_min == 2 and this field is
> erroneously initialized frames_min == 1?
>
>>>
>>>>      /** Maximum and upper limit of number of frames per row
>>>>         (defaults to 255).  */
>>>>      uint16_t frames_max;
>>>> #define AVSEQ_SONG_FRAMES_MAX   255
>>>>
>>>>      /** Minimum and lower limit of MED style SPD timing values
>>>>         (defaults to 1).  */
>>>>      uint16_t spd_min;
>>>> #define AVSEQ_SONG_SPD_MIN  1
>>>>
>>>>      /** Maximum and upper limit of MED style SPD timing values
>>>>         (defaults to 255).  */
>>>>      uint16_t spd_max;
>>>> #define AVSEQ_SONG_SPD_MAX  255
>>>>
>>>>      /** Minimum and lower limit of rows per beat timing values
>>>>         (defaults to 1).  */
>>>>      uint16_t bpm_tempo_min;
>>>> #define AVSEQ_SONG_BPM_TEMPO_MIN    1
>>>>
>>>>      /** Maximum and upper limit of rows per beat timing values
>>>>         (defaults to 255).  */
>>>>      uint16_t bpm_tempo_max;
>>>> #define AVSEQ_SONG_BPM_TEMPO_MAX    255
>>>>
>>>>      /** Minimum and lower limit of beats per minute timing values
>>>>         (defaults to 1).  */
>>>>      uint16_t bpm_speed_min;
>>>> #define AVSEQ_SONG_BPM_SPEED_MIN    1
>>>>
>>>>      /** Maximum and upper limit of beats per minute timing values
>>>>         (defaults to 255).  */
>>>>      uint16_t bpm_speed_max;
>>>> #define AVSEQ_SONG_BPM_SPEED_MAX    255
>>>
>>> Same for those.
>>
>> Would overflow multiply in playback time calculation for larger
>> values...and also does not make sense, see track effects, they all set
>> 8-bit values only, if we would allow a higher speed here, we could never
>> change the speed values which are larger than 255.
>
> same.
>
>>>
>>>>      /** Global volume of this sub-song. All other volume related
>>>>         commands are scaled by this (defaults to 255 = no
>>>> scaling).  */
>>>>      uint8_t global_volume;
>>>> #define AVSEQ_SONG_VOLUME   255
>>>>
>>>>      /** Global sub-volume of this sub-song. This is basically
>>>>         volume divided by 256, but the sub-volume doesn't account
>>>>         into actual mixer output (defaults to 0).  */
>>>>      uint8_t global_sub_volume;
>>>> #define AVSEQ_SONG_SUB_VOLUME   0
>>>>
>>>>      /** Global panning of this sub-song. All other panning related
>>>>         commands are scaled by this stereo separation factor
>>>>         (defaults to 0 which means full stereo separation).  */
>>>>      uint8_t global_panning;
>>>> #define AVSEQ_SONG_PANNING  0
>>>>
>>>>      /** Global sub-panning of this sub-song. This is basically
>>>>         panning divided by 256, but the sub-panning doesn't account
>>>>         into actual mixer output (defaults to 0).  */
>>>>      uint8_t global_sub_panning;
>>>> #define AVSEQ_SONG_SUB_PANNING  0
>>>
>>> Again, a file can specify those, no? Or they are parameters for the
>>> player?
>>
>> Yes, although TuComposer itself is the only one now who does support
>> this, after extending from 8-bit to 16-bit I have had to find an usage
>> for the lower 8-bits of the data word of track effects, these allow
>> setting it.
>>
>> But remember again, all stuff in
>> module.h/song.h/order.h/track.h/instr.h/sample.h/synth.h is supposed to
>> be changed by editors, too. Only player.h is mostly read-only for
>> external applications.
>
> I don't understand this point. What can an editor do that cannot be
> read from a file? In which struct should I set the player parameters
> that are independent of the song (i.e, some global volume to scale the
> song global volume, or if the player should output floats or s16le or
> s32le)? It does not belongs to the BSS anyway...

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 song.[ch] part of the BSS to review.

This version compiles perfectly.

-- 

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

Vitor Sessak a écrit :

On 08/13/2010 10:11 PM, Sebastian Vater wrote:

...

Vitor Sessak a écrit :

...

Shouldn't be the job of whoever fills up the BSS to translate the format commands to ours, instead of having two possible interpretation of the commands by the player?

This is not only a flag for demuxers, but also for composers, sometimes composers are such familar with the usual 0-64 volume range, they simply want to continue using it instead of being forced to new 0-255 range.

Composers are not using the BSS, they will use a composing tool. In the worse case, when transcoding from a 0-64 range format to another we will do the conversion to the 0-255 range and back.

Regarding the volume stuff I'm long time thinking about to extend that to at least 16-bit at all, i.e. union of volume and sub-volume, the same would apply to panning and sub-panning, etc. Up to now, they're distinct entities, sub-volume and sub-panning is only considered during slides but never actually taken into account on final calculation. This stuff will surely cause interesting discussions. Or to be short, yes, there's a lot of things to improve! But all this please, when the basic stuff here works, i.e. module playback as original TuComposer did.

...

This also affects slides, consider a initial volume of 0xFF and 0x40 (old volume).

This should be possible to set elsewhere in the BSS, no?

Hmm where? Would you like that extra defined in each track or sth. like that? I think once in a song is enough. Maybe I also should mention, that in TuComposer the relationship between module and sub-song is like the relationship of album between track.

...

Now we volume slide down by 04 (which is 01 in old volume). 0xFF will become 0xFB then and 0x40 will become 0x3F. But 0x3F is 0xFC in 8-bit volume so it is off by one level.

Well, converting 8-bit to 6-bit is lossly, but what about if we convert everything to 8-bit?

Well, removing this would not actually be a problem, because I never used that, in TuComposer all loaders, indeed, have a flag to tell if to use 8-bit or 6-bit volumes, but my current implementation always used 8-bit though. Maybe this is really a candidate to fully remove.

...

To be honest, this flag is for those: "I'm a hardcore MOD correct playback freak!" guys ;)

...

...

+ AVSEQ_SONG_COMPAT_FLAG_GLOBAL_NEW_ONLY = 0x10, ///< Global volume/panning changes affect new notes only (S3M)

Hmm, a flag for making "global X affect only Y" looks a sign of bad design...

This again, is a compatibility flag, S3M global volume/panning effects are different than used in other trackers. Removing these would require an intelligent parser of the whole song track data which is a nightmare to do.

However is filling the BSS up wouldn't need to do this parsing anyway?

No, it requires fully understanding of what's being parsed. Again, this is hard to describe in words (this problem was detected after disassembly nightmares almost 10 years ago, so I barely remember what was the exact cause for this flag, now). Let me explain the problem: In XM/IT when you use the global volume/panning commands, they only affect the next note being played, but not in S3M, here it affects all notes being currently played already!

...

Also OctaMED has a strange feature switching just the timing type (SPD <=> BpM) without changing the actual value.

...

...

+ AVSEQ_SONG_FLAG_MONO = 0x04, ///< Use mono instead of stereo output

...

+ AVSEQ_SONG_FLAG_SURROUND = 0x08, ///< Initial global surround instead of stereo panning

I don't really understand the comment...

Should mean: Use initial global surround panning instead of stereo, i.e. all channels default to surround panning. Should I fix this to this description?

yes.

Fixed.

...

...

...

+ + /** AVSequencerOrderList pointer to list of order data. */ + AVSequencerOrderList *order_list;

How does one find the end of the list?

order_list has number of channels elements

So this should be expressed in the comment.

Fixed.

...

...

...

+ /** Stack size, i.e. maximum recursion depth of GoSub command which + defaults to 4. */ + uint16_t gosub_stack_size;

Quoting what we discussed:

...

...

...

>>> >>> Doesn't this depend on the player implementation? Or is it >>> format-specific? Or is it read from the file? > > GOSUB is a TuComposer only feature right now. I thought this to > be a > nice default value, for creating a new sub-song.

Ok, but does the player need to know this value or it could just fail if the song use more recursion depth than the player support?

???

GOSUB is TuComposer only right now. The player needs to know this value to determine if it reaches end of stack pointer (i.e. allow no further increase of it), or if it's safe to push the new order list onto a new element on the stack.

GOSUB is basically a position jump command which pushes next order entry (before jumping) onto the gosub stack and return will pop it and continue at that position.

This is useful to call refrains, in old trackers you had to replicate the whole pattern sequence for the refrain part of a song. In TuComposer you just can do a GOSUB to the refrain and that's done.

Regarding the default of 4, that's an intuitive impression I got to use at default, in fact, we can change that to any value we like. ;-)

Wait, you are adding a new MOD command and you are expecting people to use it to exploit the fact that the stack pointer is limited to write songs? The clean way to do it is everybody supposes that the stack depth is infinite, if ever a song shows up that uses more than the player supports the player fail with an error. Anyway, is there any sense having a depth of more than, ie, 256?

Ehm, here you got me really wrong! The initial default does NOT mean, you're stick to it. You can set that to any valid 16-bit value you want to. The point why I did choose 4 entries is that it will sastify 99% (of my felt) needs, since most will call that for refrains and maybe for some nice drum loops, etc. For those who need more, can simply use the GUI slider to increase the stack and then use the gosub with that. But good question, if a depth of more than 256 is necessary, maybe some professors want to do recursive fibonacci with it? *gg* Anyway there will be public API functions for setting new stack sizes.

...

...

...

+ /** Compatibility flags for playback. There are rare cases + where effect handling can not be mapped into internal + playback engine and have to be handled specially. For + each sub-song which needs this, this will define new + flags which tag the player to handle it to that special + way. */ + uint8_t compat_flags;

enum AVSequencerSongFlags compat_flags;

...

+ /** Song playback flags. Some sequencers use a totally + different timing scheme which has to be taken care + specially in the internal playback engine. Also + sequencers differ in how they handle slides. */ + uint8_t flags;

same

...

+ /** Maximum number of host channels, as edited in the track view. + to be allocated and usable for order list (defaults to 16). */ + uint16_t channels; +

Who sets this, based on what? Does anything fails if one sets this to twice the correct value?

Valid channels for sub-songs are 0-255 right now, i.e. channels = 256 being the maximum value. The reason is that the effects use only 8-bit values to set channel data, i.e. making this value larger would make them impossible to address them with effects, etc.

Again, is this a limitation of the player? If yes, it does not belong to the BSS.

The BSS should not pass actually data to the player which it can't handle, and more than 8-bit channels is not handled correctly, it will fallback a lots of features normally available, i.e. exact channel control via effects. The engines itself don't care much about numbers actually there, in theory both the module virtual channels and song host channels can handle infinite channels. But the effects are designed to access and control them, also, which is done currently by 8-bit values (other 8-bit decide control mechanism). But I do expect, as a musician that I can control what I play. Imagine you're driving a car and I tell you, well, you can control 3 of your hoops, but the 4th one is beyond access boundary. Would you buy/take such one? No, right? That's why I keep the limitations to be consistent, i.e. fitting in each other. We can't simply say, ok let's change that to uint16_t from uint8_t. Or vice versa. I did carefully choose the data structures when writing TuComposer. I know, that this not set in stone, but they're useful and the whole engine currently is based on this as is. To summarize, the current limitations ensure that you can at least can control all channels, etc. by effects, which wouldn't be possible anymore if we extend that blindnessly.

To see if it really belongs to the BSS or to the player API, imagine the following situation: someday someone decides to write wrappers so FFmpeg can play MOD using either libavsequence, libmodplug or mikmod (at user's choice). All it would do is to pass the BSS to the wrapper that would call the external library. Would the 256 limitation still makes sense? What if libmodplug supports up to one million channels?

This limitation is not hard, it's only a soft-limitation in the sense of: the commands can't address more channels for direct control. Well, I know, I just added a check to avseq_song_set_channels which enforces these limits, but these can be removed if necessary. External apps are not necessary required to enforce that limits. Well, one million channels is quite a lot, you'ld need an CPU 100 times faster than most modern ones, even with the low quality mixer.

...

...

...

+ /** Initial speed divider, i.e. denominator which defaults + to disabled = 0. */ + uint8_t speed_div; + + /** Initial MED style SPD speed (defaults to 33 as in + OctaMED Soundstudio). */ + uint16_t spd_speed; + + /** Initial number of rows per beat (defaults to 4 rows are a beat). */ + uint16_t bpm_tempo; + + /** Initial beats per minute speed (defaults to 50 Hz => 125 BpM). */ + uint16_t bpm_speed;

Writing the defaults in the comment is a bad idea, since there is 99% of change that if someday one changes them they will forget to update the comment.

It's the all trackers default, I encountered so far. ANY tracker I seen for now, sets BpM speed to 125 and BpM Tempo 4 (most even don't BpM tempo but only BpM speed, but for even for those BpM tempo 4 is default).

Anyway, have you a better idea where to put the defaults then?

Nowhere. There is already a function that all it does is setting the defaults. People who want to know what the defaults are should read its source.

For the comment, I'd suggest:

/** Initial beats per minute speed (125 BpM for the vast majority of formats). */ uint16_t bpm_speed;

Note how my example is _not_ one line of code away to being wrong, while yours is.

Fixed! Anyway it's the default value which is used when the structure is initialized (the same value as used in all trackers as initial value).

...

...

...

+ /** Minimum and lower limit of number of frames per row + (defaults to 1), a value of zero is pointless, since + that would mean to play unlimited rows and tracks in + just one tick. */ + uint16_t frames_min;

Again, why is this needed? What happens if the correct value is 10 and someone sets it to 1?

There is no "correct" value in this sense regarding to the BSS. The correct ranges solely are based on the input format.

So this field do not below in the BSS. The whole idea of the BSS is to be input-format independent!

Yes, and that's why these values are there. Also composers can set this manually. Please don't forgot that ALL fields in the BSS are supposed to be editable by the user, too. Please note, removing any of these fields will break compatibility with IFF-TCM1 file format since TuComposer stores the values.

...

Some trackers allow 1-255, some 2-255, some 1-65535 (like TuComposer).

...

This field is also supposed to be user editable to ensure that speed slides never exceed min/max ranges.

What if a user is editing a MOD file that he intends to save as a completely different format? Again, the idea is the BSS should be format-agnostic. Whoever is trying to create a file (or filling up the BSS for the creation of some file) should take care of that.

Well in that case, it can save the limits with the new target format (if it supports it), or it simply has to discard them. Also the muxer/encoder could issue a warning that certain value are not supported.

...

...

...

+ /** Maximum and upper limit of number of frames per row + (defaults to 255) since a larger value would not make + sense (see track effects, they all set 8-bit values only), + if we would allow a higher speed here, we could never + change the speed values which are larger than 255. */ + uint16_t frames_max;

Again, looks like a limitation of the player, not something that should be part of the sub-song description.

It's in fact, a compatibility flag, too. Most trackers support only 8-bit tempo setting, requiring that values at 255 keep at 255.

So this should be named frames_clip_limit and the comment should reflect that. You should then clearly say that for some formats, when dealing with frames per row, 255+1 == 255.

...

TuComposer however supports a maximum value of 65535. In order not to break compatibility, this field was introduced. It will retain compatibility with 8-bit data trackers.

This is reasonable.

...

...

These last comments apply to all the limits fields.

The min/max fields are all supposed (as anything in the BSS) to be directly user editable, e.g. for preventing slides to exceed certain values.

This is a problem of the GUI editor, not of the BSS. You might define later (very later, after playing is working and committed) a new struct containing the different limits of each file format, but should be separated from the BSS.

The BSS is supposed to be 100% compatible with the IFF-TCM1 file format (in fact it's the only defined format right now which is able to store the whole BSS), that's also why I don't want to extend the sizes of the structs, because that would break 100% IFF-TCM1 compatibility and would also requiring introduction of a new format. Anyway, there will be a point, when TCM1 will be updated because of really new features. In that case, I feel comfortable to change these limits, too. This also makes it easy to transfer the BSS beyond networks and even allows to put it in extradata (just save as IFF-TCM1 and you're 100% sure that it can be decoded correctly again on the other side). -- Best regards, :-) Basty/CDGS (-:

Vitor Sessak a écrit :

On 08/14/2010 02:50 PM, Sebastian Vater wrote:

...

Up to now, they're distinct entities, sub-volume and sub-panning is only considered during slides but never actually taken into account on final calculation.

This stuff will surely cause interesting discussions. Or to be short, yes, there's a lot of things to improve! But all this please, when the basic stuff here works, i.e. module playback as original TuComposer did. [...] Well, removing this would not actually be a problem, because I never used that, in TuComposer all loaders, indeed, have a flag to tell if to use 8-bit or 6-bit volumes, but my current implementation always used 8-bit though.

Maybe this is really a candidate to fully remove.

Everything that is not used by any code _should_ be fully removed! It is easy to start simple and improve afterwards than to try to commit a complicated patch with unused parts.

It is used by the player, i.e. the player handles 6-bit volumes when this flag is set. I also remembered somethings which makes it impossible to replace this without getting trouble (synth sound support). The synth sound assembler also uses 6-bit in that case and since this a full-featured assembly language, it's practically impossible to convert TCM files correctly to 8-bit instead. :( In TuComposer the user was able to put a parameter for the demuxers which decides to use original 6-bit or 8-bit volume. The current demuxer in FFmpeg, however doesn't allow this now, but it might encounter a file which already has this flag set.

...

No, it requires fully understanding of what's being parsed. Again, this is hard to describe in words (this problem was detected after disassembly nightmares almost 10 years ago, so I barely remember what was the exact cause for this flag, now).

Let me explain the problem: In XM/IT when you use the global volume/panning commands, they only affect the next note being played, but not in S3M, here it affects all notes being currently played already!

Then it looks clear to me to duplicate all the volume formats to have an "immediate" version and a "next note" version. Do you agree?

Which is much more complex and more code than just defining a new flag.

Let me repeat my question again: Why is the stack depth should be described in the BSS? Why not set it in player.h?

player.h is read-only for composers and external users, i.e. anything that will go there isn't user editable anymore and this flag is supposed to be user editable. The other point was your idea with moving the actual stack data to player.h, that was an excellent idea, because this is not supposed to be writeable to the end-user.

...

The BSS should not pass actually data to the player which it can't handle,

No, of course it should! How can the BSS avoid doing it if it is not supposed to know anything about the player? If the players limitations should be defined somewhere, it should be in player.h!

The player itself can also play more than 256 channels, in fact it is a limitation of both the player and the BSS (the track limitations and effect limitations are accounting for the BSS, too).

...

and more than 8-bit channels is not handled correctly, it will fallback a lots of features normally available, i.e. exact channel control via effects.

The engines itself don't care much about numbers actually there, in theory both the module virtual channels and song host channels can handle infinite channels.

But the effects are designed to access and control them, also, which is done currently by 8-bit values (other 8-bit decide control mechanism). But I do expect, as a musician that I can control what I play.

Again, what the player can or cannot do is completely irrelevant for the discussion of the BSS. The BSS should be a way to describe a song, no matter who (or if) it is going to be played!

The point is that when we don't have a format which can save / restore the BSS in a 100% state, this can be awful. Imagine you're sitting one week on a new song, using an extended BSS which no format can save and/or load 100% because there is still are still no (de-)muxers for it and you notice that 90% of your work is crap because you can't save it. I'm not generally against the idea to extend that, but we should make first sure that it can be 100% exported and imported. Or consider the following, you see that FFmpeg has a nice avsequencer and start using it writing a GUI, since you see that it supports 16-bit effect commands you write the GUI it way, i.e. the users think they can use also the 16-bit range and when they save it, because they think, hmm 16-bit, so I have lots of user effects for my game / demo and then save...BAM! Please note that most tracker editors are more like hex editors, i.e. command, instrument and command data are edited directly in hex (and sometimes decimal) values.

...

Imagine you're driving a car and I tell you, well, you can control 3 of your hoops, but the 4th one is beyond access boundary. Would you buy/take such one? No, right?

Imagine you are driving a car. When you are reading your map (that you brought in some random shop), you find instructions of how bad the 4th gear in Toyota cars behave, right in the middle of the map! Ok, your car happens to be a Toyota and the problem happens to be real, but this does not means such info about belong to the map, it belongs to the car manual!!!

I'll understand the point better now, but there are still some issues to solve before. My point now is: We should first define a new muxer/demuxer format which can im-/export the new extended features, before actually implementing them (or at least in parallel). Imagine having a word processor where you can set bold/italic/etc. but it can only save ASCII, therefore loosing all that. It simply doesn't make sense to have features which cannot be exported and re-imported at all.

...

That's why I keep the limitations to be consistent, i.e. fitting in each other.

I don't really care much at this point about _what_ the limitations are, I care now about _where_ they are, and IMHO it is not in the BSS.

Limitations to be also defineable by the user should be in the BSS, read-only limitations can, of course be moved into the player. Please always note (i.e. never forgot) that you also can create an avsequencer empty file and edit it, in that case you can edit during composing time your limitations to ensure you won't have surprises when you save to your format. For example if I create a new empty file and want to save it on composition finish as S3M, I can set the use S3M-like global volume command flag manually and I know that it's fine when I finally save it in S3M. The same applies if I want to do a module for a friend which only has an Amiga 500 and I want to ensure that it will playback fine there with an ordinary MOD player, so I can again set the Amiga limits manually.

...

Yes, and that's why these values are there. Also composers can set this manually. Please don't forgot that ALL fields in the BSS are supposed to be editable by the user, too.

Please note, removing any of these fields will break compatibility with IFF-TCM1 file format since TuComposer stores the values.

How so? If it is defined somewhere in IFF-TCM1 files, the decoder are free to ignore this field, no?

And therefore lose ability to playback files correctly. Also again here the same accounts, the min/max pairs are also supposed to be user-editable, moving them to player.h will again make this read-only. Imagine, I want to start a new song with avsequencer but want to ensure that it will save correctly let's say in IT. Then I can set manually these values to the IT limits and I'm sure that when saving my work, I get a correct file.

...

The BSS is supposed to be 100% compatible with the IFF-TCM1 file format (in fact it's the only defined format right now which is able to store the whole BSS), that's also why I don't want to extend the sizes of the structs, because that would break 100% IFF-TCM1 compatibility and would also requiring introduction of a new format.

??? How so? How hard is to read an integer field from the BSS, convert it so uint8_t (or fails with an error if it doesn't fit) and write it in the TCM file? Conversely, for decoding, how hard is converting an uint8_t read to an int in the BSS?

See above, we can do this, but we should additionally define a new (de-)muxer pair which can save this, otherwise it's pointless. Reading would not be the problem anyway, since the int is larger as uint8_t, but writing is and there is probably no format around anymore which can store / restore the whole BSS. But again, don't forget, I'm not against this idea generally, it's just we don't have 100% storage containers yet as a file format, so let's do this in the future, when I either have extended the TCM1 format (since it's IFF I can do this) or something new (maybe RIFF-AVS1), ok? -- Best regards, :-) Basty/CDGS (-:

On date Sunday 2010-08-15 19:35:14 +0200, Vitor Sessak encoded:

On 08/15/2010 06:41 PM, Sebastian Vater wrote:

...

Vitor Sessak a écrit :

...

On 08/14/2010 02:50 PM, Sebastian Vater wrote:

...

Up to now, they're distinct entities, sub-volume and sub-panning is only considered during slides but never actually taken into account on final calculation.

This stuff will surely cause interesting discussions. Or to be short, yes, there's a lot of things to improve! But all this please, when the basic stuff here works, i.e. module playback as original TuComposer did. [...] Well, removing this would not actually be a problem, because I never used that, in TuComposer all loaders, indeed, have a flag to tell if to use 8-bit or 6-bit volumes, but my current implementation always used 8-bit though.

Maybe this is really a candidate to fully remove.

Everything that is not used by any code _should_ be fully removed! It is easy to start simple and improve afterwards than to try to commit a complicated patch with unused parts.

It is used by the player, i.e. the player handles 6-bit volumes when this flag is set. I also remembered somethings which makes it impossible to replace this without getting trouble (synth sound support). The synth sound assembler also uses 6-bit in that case and since this a full-featured assembly language, it's practically impossible to convert TCM files correctly to 8-bit instead. :(

What about other file formats?

...

In TuComposer the user was able to put a parameter for the demuxers which decides to use original 6-bit or 8-bit volume. The current demuxer in FFmpeg, however doesn't allow this now, but it might encounter a file which already has this flag set.

...

...

No, it requires fully understanding of what's being parsed. Again, this is hard to describe in words (this problem was detected after disassembly nightmares almost 10 years ago, so I barely remember what was the exact cause for this flag, now).

Let me explain the problem: In XM/IT when you use the global volume/panning commands, they only affect the next note being played, but not in S3M, here it affects all notes being currently played already!

Then it looks clear to me to duplicate all the volume formats to have an "immediate" version and a "next note" version. Do you agree?

Which is much more complex and more code than just defining a new flag.

...

Let me repeat my question again: Why is the stack depth should be described in the BSS? Why not set it in player.h?

player.h is read-only for composers and external users, i.e. anything that will go there isn't user editable anymore and this flag is supposed to be user editable.

So you need something that is both user-editable and not part of the BSS. Again, where should be stuff that is not part of the description of a song _and_ is set by the user?

...

The other point was your idea with moving the actual stack data to player.h, that was an excellent idea, because this is not supposed to be writeable to the end-user.

...

...

The BSS should not pass actually data to the player which it can't handle,

No, of course it should! How can the BSS avoid doing it if it is not supposed to know anything about the player? If the players limitations should be defined somewhere, it should be in player.h!

The player itself can also play more than 256 channels, in fact it is a limitation of both the player and the BSS (the track limitations and effect limitations are accounting for the BSS, too).

So where the limitation would be if we change the BSS to a int16_t (no, I don't think this has any use doing it, but you understand my point)?

What it looks like to me you are doing is:

1- You have some BSS parameter foo. 2- Sane values for the parameter foo is between 0 and AA 3- You look for the integer type whose maximum value is closest to AA 4- You use this type for foo.

I don't think this is the cleaner neither the most future-proof way of determining sane values for fields.

...

...

...

and more than 8-bit channels is not handled correctly, it will fallback a lots of features normally available, i.e. exact channel control via effects.

The engines itself don't care much about numbers actually there, in theory both the module virtual channels and song host channels can handle infinite channels.

But the effects are designed to access and control them, also, which is done currently by 8-bit values (other 8-bit decide control mechanism). But I do expect, as a musician that I can control what I play.

Again, what the player can or cannot do is completely irrelevant for the discussion of the BSS. The BSS should be a way to describe a song, no matter who (or if) it is going to be played!

The point is that when we don't have a format which can save / restore the BSS in a 100% state, this can be awful.

Imagine you're sitting one week on a new song, using an extended BSS which no format can save and/or load 100% because there is still are still no (de-)muxers for it and you notice that 90% of your work is crap because you can't save it.

You are using a crap editor to fill up the BSS. Moreover, your point is only valid considering everybody will want to write only TCM files. Your GUI should not let you use unsupported values, even if it fit in the data types.

...

I'm not generally against the idea to extend that, but we should make first sure that it can be 100% exported and imported.

I'm not talking about extending. I mean that converting a field from uint8_t to int should not mean every 16-bit value is now valid!

...

Or consider the following, you see that FFmpeg has a nice avsequencer and start using it writing a GUI, since you see that it supports 16-bit effect commands you write the GUI it way, i.e. the users think they can use also the 16-bit range and when they save it, because they think, hmm 16-bit, so I have lots of user effects for my game / demo and then save...BAM!

GUI developers should not need babysitting. And _if_ we want to specify this limits somewhere, it should not be in the BSS (limits.h?).

...

Please note that most tracker editors are more like hex editors, i.e. command, instrument and command data are edited directly in hex (and sometimes decimal) values.

And they don't do any sanity-check?

...

...

...

Imagine you're driving a car and I tell you, well, you can control 3 of your hoops, but the 4th one is beyond access boundary. Would you buy/take such one? No, right?

Imagine you are driving a car. When you are reading your map (that you brought in some random shop), you find instructions of how bad the 4th gear in Toyota cars behave, right in the middle of the map! Ok, your car happens to be a Toyota and the problem happens to be real, but this does not means such info about belong to the map, it belongs to the car manual!!!

I'll understand the point better now, but there are still some issues to solve before. My point now is: We should first define a new muxer/demuxer format which can im-/export the new extended features,

I'm not suggesting adding any new features, I suggest not depending on the BSS data types to know what is supported or not.

...

before actually implementing them (or at least in parallel).

Imagine having a word processor where you can set bold/italic/etc. but it can only save ASCII, therefore loosing all that. It simply doesn't make sense to have features which cannot be exported and re-imported at all.

And you would say that the problem in this case would be _the internal data structures_ used by the word processor?

...

...

...

That's why I keep the limitations to be consistent, i.e. fitting in each other.

I don't really care much at this point about _what_ the limitations are, I care now about _where_ they are, and IMHO it is not in the BSS.

Limitations to be also defineable by the user should be in the BSS, read-only limitations can, of course be moved into the player.

Is this _part of the song description_?

...

Please always note (i.e. never forgot) that you also can create an avsequencer empty file and edit it, in that case you can edit during composing time your limitations to ensure you won't have surprises when you save to your format.

For example if I create a new empty file and want to save it on composition finish as S3M, I can set the use S3M-like global volume command flag manually and I know that it's fine when I finally save it in S3M.

The same applies if I want to do a module for a friend which only has an Amiga 500 and I want to ensure that it will playback fine there with an ordinary MOD player, so I can again set the Amiga limits manually.

Again, in the BSS? This enforcing this limits is the job of the editor, and he should read from somewhere else than the BSS (again, the BSS should be _format independent_).

...

...

...

Yes, and that's why these values are there. Also composers can set this manually. Please don't forgot that ALL fields in the BSS are supposed to be editable by the user, too.

Please note, removing any of these fields will break compatibility with IFF-TCM1 file format since TuComposer stores the values.

How so? If it is defined somewhere in IFF-TCM1 files, the decoder are free to ignore this field, no?

And therefore lose ability to playback files correctly.

How so? We are talking about a field that we just agreed that won't change playback.

...

Also again here the same accounts, the min/max pairs are also supposed to be user-editable, moving them to player.h will again make this read-only.

So player_config.h or something else.

...

Imagine, I want to start a new song with avsequencer but want to ensure that it will save correctly let's say in IT. Then I can set manually these values to the IT limits and I'm sure that when saving my work, I get a correct file.

Again, I think the BSS should do _one_ _single_ _thing_: describing the song in a format-independent format. IT limits are useful to be set somewhere, but not in the BSS.

I tend to agree with Vitor here. BSS = Big Sound Struct = Sound Module Descriptor This describes a sound module, so it should be as generic and abstract as possible, possibly it shouldn't contain format-specific information. This is very important if we want to keep a clean design. As for the compatibility problem, let's consider this scenario. Anna is composing a module song. She's using a fancy editor which links against FFmpeg/libavsequencer, which works through saving a binary representation of the libavsequencer BSS. This representation is really generic, so it doesn't depend on the features of the format which will be used to store the song module. Now Anna wants to let his friend Bob hear the song module she's composing. She can save the song using one of the many sound module formats, S3M, XM/IT, TCM1. When she tries to save to S3M, her program may cry out that it isn't possible to save to that format, as that format doesn't allow to store some of the features of the composed song. Is that a problem? I don't think so, and indeed when using a word processor this happens all the times, when saving you can either do two choices: * lose the information stored in the binary representation. Which information to lose and which to keep is a problem of the program or * store the information in a more expressive format For example if you save a word document to txt file, the program will discard some of the features of the document when performing the conversion.

From the usability point of view, it is a problem of the interface (not of the internal representation of the information), to provide compatibility hints to the user. This can be implemented in various ways, for example Anna knows that his friend Bob only likes to hear S3M sound modules, so she may check a S3M compatibility flag in her fancy composer, so she'll be sure that the composer application *won't allow her* to add features which cannot be represented in a S3M file.

libavsequencer may help the application providing such compatibility information in some table or providing an API for that, which should be anyway separated from the sound module representation aka BSS. Regards.

Vitor Sessak a écrit :

On 08/15/2010 06:41 PM, Sebastian Vater wrote:

...

It is used by the player, i.e. the player handles 6-bit volumes when this flag is set. I also remembered somethings which makes it impossible to replace this without getting trouble (synth sound support). The synth sound assembler also uses 6-bit in that case and since this a full-featured assembly language, it's practically impossible to convert TCM files correctly to 8-bit instead. :(

What about other file formats?

Support usually only 6-bit volumes, though. But shouldn't be the rule here, if there is one exception that we have to address that issue? If we want it format independent, then it should also consider the fact, that TuComposer requires this. Sorry that I didn't recognize that already yesterday, I remembered that after my writing only.

...

In TuComposer the user was able to put a parameter for the demuxers which decides to use original 6-bit or 8-bit volume. The current demuxer in FFmpeg, however doesn't allow this now, but it might encounter a file which already has this flag set. [...] player.h is read-only for composers and external users, i.e. anything that will go there isn't user editable anymore and this flag is supposed to be user editable.

So you need something that is both user-editable and not part of the BSS. Again, where should be stuff that is not part of the description of a song _and_ is set by the user?

Setting the stack size is part of the BSS since it belongs to song description. Please note a song with a different stack size doesn't sound necessary the same (and this is not because of the player but pure logic, the a song with a different starting tempo doesn't sound the same also), and the composers should really be free in what they need here. I know it's rather an unconventional feature. Again, the stack pointers itself (which contain the data) are a completely different thing and that's why I moved them to player.h without big questions, since that really belongs to the player and not to the BSS, but that's not the case for the size.

...

The player itself can also play more than 256 channels, in fact it is a limitation of both the player and the BSS (the track limitations and effect limitations are accounting for the BSS, too).

So where the limitation would be if we change the BSS to a int16_t (no, I don't think this has any use doing it, but you understand my point)?

What it looks like to me you are doing is:

1- You have some BSS parameter foo. 2- Sane values for the parameter foo is between 0 and AA 3- You look for the integer type whose maximum value is closest to AA 4- You use this type for foo.

I don't think this is the cleaner neither the most future-proof way of determining sane values for fields.

I already extended integer types where it made sense. That's also why the sub-song channels in the BSS (and player!) is uint16_t and not uint8_t (which would be 0-255). But more than 65535 host and virtual channels will probably never ever make sense (imagine just the time to fill that all up by a composer track to track, row to row). Also if you noticed that all the public API calls take uint32_t as a parameter, even for channels, so they can assign larger values (which are either clipped or return AVERROR_INVALIDDATA. The same applies, btw, to stack size, it is limited to 65535 entries (which would mean a loop / gosub stack of 65535 which is way larger than everyone requires). Looping a pattern, e.g. 65535 times would make the sound REALLY boring (you can't extend pattern loop beyond one pattern). Having an order list range played with a gosub stack size of 65535 would also make the song very boring. For the volume / panning stuff we already clarified that, I want to extend that to full 16-bit someday. The same also applies to the boundary ranges, they are not as hard as they look at first, since all of these values are 16 bit, too. But having e.g. tempo larger than 65535 would delay a row soo much long that you can wait hours before it increases, BpR && BpM > 65535 is the same, this is "speed of light" and would increase CPU consumptation such heavy that even a single channel module couldn't playback with a Core2Duo and alike. Making data types way larger than required is sometimes no problem, unless it is required very, very often (like the track effects), which would cause a significant increase in memory usage. Also note, all trackers I know of are in this regarding this still 8-bit only, since command data is 8-bit. Extending that to 16-bit is way more than enough and should sastify for the next 50 years.

...

The point is that when we don't have a format which can save / restore the BSS in a 100% state, this can be awful.

Imagine you're sitting one week on a new song, using an extended BSS which no format can save and/or load 100% because there is still are still no (de-)muxers for it and you notice that 90% of your work is crap because you can't save it.

You are using a crap editor to fill up the BSS. Moreover, your point is only valid considering everybody will want to write only TCM files. Your GUI should not let you use unsupported values, even if it fit in the data types.

The editor can't know which formats are supported in exact detail by the current FFmpeg version, the editor should simply use the public API calls which either a) return INVALIDDATA or b) clip to allowed range where it makes sense (i.e. setting number of maximum channels) and are already 32-bit. After all, I invented the API for taking as much burden as possible from application writers.

...

I'm not generally against the idea to extend that, but we should make first sure that it can be 100% exported and imported.

I'm not talking about extending. I mean that converting a field from uint8_t to int should not mean every 16-bit value is now valid!

Agree here! For the flags we can really extend them to int (enum). As long we won't define any new ones across the current boundaries it won't be any problem.

...

Or consider the following, you see that FFmpeg has a nice avsequencer and start using it writing a GUI, since you see that it supports 16-bit effect commands you write the GUI it way, i.e. the users think they can use also the 16-bit range and when they save it, because they think, hmm 16-bit, so I have lots of user effects for my game / demo and then save...BAM!

GUI developers should not need babysitting. And _if_ we want to specify this limits somewhere, it should not be in the BSS (limits.h?).

The limits.h idea is great! :) But give me some time on really think on it, how to implement it the best way.

...

Please note that most tracker editors are more like hex editors, i.e. command, instrument and command data are edited directly in hex (and sometimes decimal) values.

And they don't do any sanity-check?

For effects? No, either they ignore it or do something undocumented, sometimes they also set to an internal maximum / minimum value which is handled differently from program to program, which is also one of the biggest problems having all the different MOD players to get the same playback for the same file.

...

I'll understand the point better now, but there are still some issues to solve before. My point now is: We should first define a new muxer/demuxer format which can im-/export the new extended features,

I'm not suggesting adding any new features, I suggest not depending on the BSS data types to know what is supported or not.

Most data types are long enough already, the point I see is mainly the flags (where I now finally agree, see below) and volume / panning stuff.

...

before actually implementing them (or at least in parallel).

Imagine having a word processor where you can set bold/italic/etc. but it can only save ASCII, therefore loosing all that. It simply doesn't make sense to have features which cannot be exported and re-imported at all.

And you would say that the problem in this case would be _the internal data structures_ used by the word processor?

Internal? Well, the BSS is public API, right?

...

...

...

That's why I keep the limitations to be consistent, i.e. fitting in each other.

I don't really care much at this point about _what_ the limitations are, I care now about _where_ they are, and IMHO it is not in the BSS.

Limitations to be also defineable by the user should be in the BSS, read-only limitations can, of course be moved into the player.

Is this _part of the song description_?

Yes it is. Remember, these limitations are not BSS and player specific. It's like saying (to take the car example): Yes, I know I can drive 300 km/h, but I prefer just 100 km/h and limit me myself consciousnessly by it. The actual BSS / player limitations are as said all either 16-bit range (for the speed stuff) or even 32-bit (sample rate). BTW, the limits are set by default to the whole integer type range (in most cases), if you want to restrict that actually, you have to do that for yourself (see some of my source files in github, to see of it is actually used).

...

Please always note (i.e. never forgot) that you also can create an avsequencer empty file and edit it, in that case you can edit during composing time your limitations to ensure you won't have surprises when you save to your format.

For example if I create a new empty file and want to save it on composition finish as S3M, I can set the use S3M-like global volume command flag manually and I know that it's fine when I finally save it in S3M.

The same applies if I want to do a module for a friend which only has an Amiga 500 and I want to ensure that it will playback fine there with an ordinary MOD player, so I can again set the Amiga limits manually.

Again, in the BSS? This enforcing this limits is the job of the editor, and he should read from somewhere else than the BSS (again, the BSS should be _format independent_).

Wouldn't that require that the editor would know the limits of the BSS?

...

And therefore lose ability to playback files correctly.

How so? We are talking about a field that we just agreed that won't change playback.

Yes that was my fault, I just forgot the synth sound assembler when I thought that this won't change playback.

...

Also again here the same accounts, the min/max pairs are also supposed to be user-editable, moving them to player.h will again make this read-only.

So player_config.h or something else.

limits.h?

...

Imagine, I want to start a new song with avsequencer but want to ensure that it will save correctly let's say in IT. Then I can set manually these values to the IT limits and I'm sure that when saving my work, I get a correct file.

Again, I think the BSS should do _one_ _single_ _thing_: describing the song in a format-independent format. IT limits are useful to be set somewhere, but not in the BSS.

I know and that's why all these fields are here, please note, I didn't added them, because I was bored, but because I found that to be necessary. TuComposer's main strength is compatibility playback and that's because of these fields and flags, if we remove them, we have just another MOD engine which fails here and there. Anyway moving actual limits to limits.h is a great idea!

...

See above, we can do this, but we should additionally define a new (de-)muxer pair which can save this, otherwise it's pointless.

The whole point was allowing changing an uint8_t to an enum who could be no greater than 255 (but could be internally handled by the compiler as a 16/32-bit integer). How would it change things?

Oh, well that's right. Agree here now! One last question, do you agree that we do the structural changes when everything works fine? I mean FFmpeg is able to playback TCM modules since today (read my mail?), but you see there are some issues right now probably mostly arrived simply by doing a simple change from linked-list storage to ** array. Anyway, very very much thanks for your constructive critics and ideas! I'll submit now the current patch set for the BSS as suggested by Ronald all in one patch. -- Best regards, :-) Basty/CDGS (-: