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

[Vitor Sessak]

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?

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

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

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

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

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

-Vitor