[FFmpeg-soc] [soc] libavsequencer [PATCH 02/08] Sub-song public API header file.

[Vitor Sessak]

On 08/13/2010 10:11 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 08/07/2010 09:43 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 song.[ch] part of the BSS to review.
>>
>>>
>>> diff --git a/libavsequencer/song.h b/libavsequencer/song.h
>>> new file mode 100755
>>> index 0000000..15f8b3e
>>> --- /dev/null
>>> +++ b/libavsequencer/song.h
>>> @@ -0,0 +1,208 @@
>>> +/*
>>> + * AVSequencer sub-song management
>>> + * Copyright (c) 2010 Sebastian Vater<cdgs.basty at 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 "libavutil/log.h"
>>> +#include "libavformat/avformat.h"
>>> +#include "libavsequencer/order.h"
>>> +#include "libavsequencer/track.h"
>>> +
>>> +/** AVSequencerSong->compat_flags bitfield.  */
>>> +enum AVSequencerSongCompatFlags {
>>> +    AVSEQ_SONG_COMPAT_FLAG_SYNC             = 0x01, ///<  Tracks are
>>> synchronous (linked together, pattern based)
>>> +    AVSEQ_SONG_COMPAT_FLAG_GLOBAL_LOOP      = 0x02, ///<  Global
>>> pattern loop memory
>>
>>> +    AVSEQ_SONG_COMPAT_FLAG_AMIGA_LIMITS     = 0x04, ///<  Enforce
>>> AMIGA sound hardware limits (portamento)
>>
>> Any song sounds wrong if you don't enforce these limits?
>
> Yes, I have some modules which sound wrong when this isn't set.
> This is because, some composers do a portamento way longer than it
> should be. When that hit the amiga hardware limits the frequency slide
> simply didn't do anything. This flag ensures that this will happen.

ok

>>> +    AVSEQ_SONG_COMPAT_FLAG_OLD_VOLUMES      = 0x08, ///<  All volume
>>> related commands range from 0-64 instead of 0-255
>>
>> 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.

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

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

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

>>
>>> +/** AVSequencerSong->compat_flags bitfield.  */
>>> +enum AVSequencerSongFlags {
>>> +    AVSEQ_SONG_FLAG_LINEAR_FREQ_TABLE   = 0x01, ///<  Use linear
>>> instead of Amiga frequency table
>>
>> AMIGA is the default? Is amiga frequency tables that common in mod
>> formats?
>
> Yes, it is! Even on some PC trackers, Amiga slide frequency table (when
> you can choose in between) is the default.
> Also most PC trackers have this field to switch between both (they
> usually set amiga frequency table automatically when importing MOD).
> Anyway, there is no true way of choosing a reasonable default between
> those. You could equally simply throw a coin and let that choose.
>
> Both tables have advantages and disadvantages, which are hard to
> describe in words (try out a tracker and compare both types using
> portamentoes, you'll see that one instrument sounds better with amiga
> slide table, but another better with linear, etc. there's simply no
> generic in that).
>
>>
>>
>>> +    AVSEQ_SONG_FLAG_SPD                 = 0x02, ///<  Use SPD
>>> (OctaMED style) timing instead of BpM
>>
>> I'm not sure if this should be handled by the player instead of
>> choosing BPM and forcing the decoders to do any necessary conversion.
>
> SPD timing is fundamental different than BPM timing, you can't simply
> convert them to each other without losing accuracy. They both are based
> on very different formulas.
>
> 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.

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

>>
>>> +    /** 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?

Again, the limitations of the player do not belong to the BSS. It 
describes the SONG.

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

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?

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

>>> +    /** 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!

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

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

-Vitor