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

[Sebastian Vater]

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.

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

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

>
>> +/**
>> + * Arpeggio control envelope used by all instrumental stuff.
>> + * 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 AVSequencerArpeggio {
>> +    /**
>> +     * information on struct for av_log
>> +     * - set by avseq_alloc_context
>> +     */
>> +    const AVClass *av_class;
>> +
>> +    /** Metadata information: Original arpeggio file name, arpeggio
>> +     *  name, artist and comment.  */
>> +    AVMetadata *metadata;
>> +
>> +    /** AVSequencerArpeggioData pointer to arpeggio data structure.  */
>> +    AVSequencerArpeggioData *data;
>> +
>> +    /** Instrument arpeggio control flags. Some sequencers feature
>
>> +       customized arpeggio command control.which have to be taken
>
> s/control.which/control, which/
> Also, enum.

Oops, damn typo! Fixed!

-- 

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