[FFmpeg-soc] [soc] libavsequencer [PATCH 05/08] Instrument handling public API header file.

[Vitor Sessak]

On 08/13/2010 10:38 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 08/07/2010 09:54 PM, Sebastian Vater wrote:
>>> Sebastian Vater a écrit :
>>>> Vitor Sessak a écrit :
>>>>
>>>>> On 07/13/2010 11:13 PM, Sebastian Vater wrote:
>>>>>
>>>>>
>>>>>> Original TuComposer didn't even use typedef's at all, the only way to
>>>>>> access them was a struct TuComposerInstrEnvelope, etc.
>>>>>>
>>>>>> I also prefer the way to provide the target programmer more
>>>>>> freedom than
>>>>>> shrinking it, despite the fact that writing the header the way I did
>>>>>> doesn't require much amount of time.
>>>>>>
>>>>>> This, however, is just a target-programmer-user-friendly purpose and
>>>>>> wouldn't interfere with player.c by changing that, since I
>>>>>> replaced all
>>>>>> struct AVSequencer* with simply AVSequencer*
>>>>>>
>>>>>> However, changing that again, would require extra work by
>>>>>> additionally
>>>>>> making it a bit uncomfortable to target programmers. So if you are
>>>>>> not
>>>>>> piecy on this, I would keep is it at now, I will leave that decision
>>>>>> completely to you, though. If you want me to remove that, I'll do.
>>>>>>
>>>>> Leave it as is. Maybe it is just my personal taste and not very
>>>>> important ATM.
>>>>>
>>>>
>>>> Ok. Updated patch though.
>>>>
>>>
>>> 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 instr.[ch] part of the BSS to review.
>>>
>>> This version compiles perfectly.
>>
>>> diff --git a/libavsequencer/instr.h b/libavsequencer/instr.h
>>> new file mode 100755
>>> index 0000000..1a1eede
>>> --- /dev/null
>>> +++ b/libavsequencer/instr.h
>>> @@ -0,0 +1,571 @@
>>> +/*
>>> + * AVSequencer instrument 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_INSTR_H
>>> +#define AVSEQUENCER_INSTR_H
>>> +
>>> +#include "libavutil/log.h"
>>> +#include "libavformat/avformat.h"
>>> +
>>> +/** AVSequencerEnvelope->flags bitfield.  */
>>> +enum AVSequencerEnvelopeFlags {
>>> +    AVSEQ_ENVELOPE_LOOP             = 0x0001, ///<  Envelope uses
>>> loop nodes
>>> +    AVSEQ_ENVELOPE_SUSTAIN          = 0x0002, ///<  Envelope uses
>>> sustain nodes
>>
>> Why do you have to set a flag saying if it uses or not some feature?
>> Can't this be evaluated from the other fields?
>
> Again, this is set by the composer, the composer can decide fully for
> free, if he wants to have his/her envelope loop and/or sustain loop.

Let me pose the question in other way: you get a song that has no loops. 
You get its BSS, change this flag, and send to the player. Will anything 
change?

>>> +    AVSEQ_ENVELOPE_PINGPONG         = 0x0004, ///<  Envelope loop is
>>> in ping pong mode
>>> +    AVSEQ_ENVELOPE_SUSTAIN_PINGPONG = 0x0008, ///<  Envelope sustain
>>> loop is in ping pong mode
>>> +};
>>> +/**
>>> + * Envelope structure used by instruments to apply volume / panning
>>> + * or pitch manipulation according to an user defined waveform.
>>> + * 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 AVSequencerEnvelope {
>>> +    /**
>>> +     * information on struct for av_log
>>> +     * - set by avseq_alloc_context
>>> +     */
>>> +    const AVClass *av_class;
>>> +
>>> +    /** Metadata information: Original envelope file name, envelope
>>> +     *  name, artist and comment.  */
>>> +    AVMetadata *metadata;
>>> +
>>> +    /** The actual node data of this envelope as signed 16-bit integer.
>>> +       For a volume envelope, we have a default scale range of -32767
>>> +       to +32767, for panning envelopes the scale range is between
>>> -8191
>>> +       to +8191. For slide, vibrato, tremolo, pannolo (and their auto
>>> +       versions), the scale range is between -256 to +256.  */
>>> +    int16_t *data;
>>
>>> +    /** The node points values or 0 if the envelope is empty.  */
>>> +    uint16_t *node_points;
>>
>> What is the point of an empty envelope?
>
> Don't mix empty node points with an empty envelope.
> Node points are draggable points in the node. An empty node point
> envelope is just only a free-draw envelope. Node points cause node data
> to be connected in a linear way.
>
> Again, this is hard to describe in words, could you just install the
> schism package (SchismTracker), which is an IT clone and press F4 for
> the instrument editor and go to envelopes.
> You'll see some squares there which you can choose with cursor
> left+right and drag them (move them left / right, up / down).
>
> It will draw the envelope as a line from node point - 1 to node_point to
> node_point + 1.
>
> TuComposer however, internally doesn't use them at all, it internally
> use only know node data like a sample, the points are just there to
> maintain imported module data (and as well as user-defined actions to
> define them).
>
> The player therefore doesn't care about node points, just the node data.
>
>>
>>> +    /** Number of dragable nodes of this envelope (defaults to 12).  */
>>> +    uint16_t nodes;
>>> +
>>> +    /** Number of envelope points, i.e. node data values which
>>> +       defaults to 64.  */
>>> +    uint16_t points;
>>> +
>>> +    /** Instrument envelope flags. Some sequencers feature
>>> +       loop points of various kinds, which have to be taken
>>> +       care specially in the internal playback engine.  */
>>
>>> +    uint16_t flags;
>>
>> enum...
>
> Still, I don't understand here...enum ist just above struct / typedef
> declaration.
>
>>
>>> +    /** Envelope tempo in ticks (defaults to 1, i.e. change envelope
>>> +       at every frame / tick).  */
>>> +    uint16_t tempo;
>>> +
>>> +    /** Envelope sustain loop start point.  */
>>> +    uint16_t sustain_start;
>>> +
>>> +    /** Envelope sustain loop end point.  */
>>> +    uint16_t sustain_end;
>>> +
>>> +    /** Envelope sustain loop repeat counter for loop range.  */
>>> +    uint16_t sustain_count;
>>> +
>>> +    /** Envelope loop repeat start point.  */
>>> +    uint16_t loop_start;
>>> +
>>> +    /** Envelope loop repeat end point.  */
>>> +    uint16_t loop_end;
>>> +
>>> +    /** Envelope loop repeat counter for loop range.  */
>>> +    uint16_t loop_count;
>>> +
>>> +    /** Randomized lowest value allowed.  */
>>> +    int16_t value_min;
>>> +
>>> +    /** Randomized highest value allowed.  */
>>> +    int16_t value_max;
>>> +
>>> +    /** Array of pointers containing every unknown data field where
>>> +       the last element is indicated by a NULL pointer reference. The
>>> +       first 64-bit of the unknown data contains an unique identifier
>>> +       for this chunk and the second 64-bit data is actual unsigned
>>> +       length of the following raw data. Some formats are chunk based
>>> +       and can store information, which can't be handled by some
>>> +       other, in case of a transition the unknown data is kept as is.
>>> +       Some programs write editor settings for envelopes in those
>>> +       chunks, which then won't get lost in that case.  */
>>> +    uint8_t **unknown_data;
>>> +} AVSequencerEnvelope;
>>> +
>>> +/**
>>> + * Keyboard definitions structure used by instruments to map
>>> + * note to samples. C-0 is first key. B-9 is 120th key.
>>> + * 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 AVSequencerKeyboard {
>>> +    /**
>>> +     * information on struct for av_log
>>> +     * - set by avseq_alloc_context
>>> +     */
>>> +    const AVClass *av_class;
>>> +
>>> +    /** Metadata information: Original keyboard file name, keyboard
>>> +     *  name, artist and comment.  */
>>> +    AVMetadata *metadata;
>>
>>> +    struct AVSequencerKeyboardEntry {
>>> +        /** Sample number for this keyboard note.  */
>>> +        uint16_t sample;
>>> +
>>> +        /** Octave value for this keyboard note.  */
>>> +        uint8_t octave;
>>> +
>>> +        /** Note value for this keyboard note.  */
>>> +        uint8_t note;
>>> +    } key[120];
>>
>> Why is this needed if you set already C-0 as the first key and B-9 as
>> the 120th? Can't you just evaluate all that from the key index?
>
> Very nice idea, but would require larger changes to whole architecture,
> keep that in mind again when playback works (very soon).

Not so large, you can just replace:

key[a].note

to

get_note(a)

where get_note() is an inline function.

>>> +} AVSequencerKeyboard;
>>> +
>>> +/**
>>> + * Arpeggio data structure, This structure is actually for one tick
>>> + * and therefore actually pointed as an array with the amount of
>>> + * different ticks handled by the arpeggio control.
>>> + * 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 AVSequencerArpeggioData {
>>> +    /** Packed note or 0 if this is an arpeggio note.  */
>>> +    uint8_t tone;
>>> +
>>> +    /** Transpose for this arpeggio tick.  */
>>> +    int8_t transpose;
>>> +
>>> +    /** Instrument number to switch to or 0 for original
>>> instrument.  */
>>> +    uint16_t instrument;
>>> +
>>> +    /** The four effect command bytes which are executed.  */
>>> +    uint8_t command[4];
>>> +
>>> +    /** The four data word values of the four effect command bytes.  */
>>> +    uint16_t data[4];
>>> +} AVSequencerArpeggioData;
>>
>>
>>> +/** AVSequencerArpeggio->flags bitfield.  */
>>> +enum AVSequencerArpeggioFlags {
>>> +    AVSEQ_ARPEGGIO_FLAG_LOOP                = 0x0001, ///<  Arpeggio
>>> control is looped
>>> +    AVSEQ_ARPEGGIO_FLAG_SUSTAIN             = 0x0002, ///<  Arpeggio
>>> control has a sustain loop
>>> +    AVSEQ_ARPEGGIO_FLAG_PINGPONG            = 0x0004, ///<  Arpeggio
>>> control will be looped in ping pong mpde
>>> +    AVSEQ_ARPEGGIO_FLAG_SUSTAIN_PINGPONG    = 0x0008, ///<  Arpeggio
>>> control will have sustain loop ping pong mode enabled
>>> +};
>>
>> This looks duplicated.
>
> I know, arpeggio structures are very, very similar to normal envelope
> data, just that the data size is different than uint16_t.
> Currently they define the same flags, but this might change in the
> future, so I keep them in a different field.
> But would of course not harm for now, to union them, if you wish this.

I'd prefer you keep it simple for now. Complicating for the future 
normally doesn't pays up.

-Vitor