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

[Vitor Sessak]

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?

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

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

> +};

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

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

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

> +};

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

How does one find the end of the list?

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

???

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

The player limitations does not belong in the BSS!!!

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

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

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

These last comments apply to all the limits fields.

-Vitor