[FFmpeg-soc] [soc] libavsequencer [PATCH 04/08] Track / pattern handling public API header file.

[Sebastian Vater]

Vitor Sessak a écrit :
> On 08/13/2010 10:28 PM, Sebastian Vater wrote:
>> Vitor Sessak a écrit :
>>> On 08/07/2010 09:46 PM, Sebastian Vater wrote:
>>>> Vitor Sessak a écrit :
>>>>> On 07/13/2010 10:57 PM, Sebastian Vater wrote:
>>>>>> Vitor Sessak a écrit :
>>>>>>> On 07/13/2010 09:35 PM, Sebastian Vater wrote:
>>>>>>>> Vitor Sessak a écrit :
>>>>>>>>> On 07/11/2010 10:07 PM, Sebastian Vater wrote:
>>>>>>>>>> typedef struct AVSequencerTrackData {
>>>
>>> [...]
>>>
>>>>>>>>> if (instrument == previous_instrument) {...}
>>>>>>>>
>>>>>>>> Please note that instrument number 0 also differs whether you
>>>>>>>> play the
>>>>>>>> track in once mode (pattern play mode) and the instrument
>>>>>>>> number was
>>>>>>>> initialized in the previous order. All trackers don't output
>>>>>>>> sound in
>>>>>>>> that case, changing this would make this part incompatible.
>>>>>>>
>>>>>>> That's another good point, so I'm fine with instrument==0 meaning
>>>>>>> last
>>>>>>> instrument. But if the behavior of setting instrument==0 is
>>>>>>> different
>>>>>>> of the behavior of setting instrument==last_instrument, it
>>>>>>> should be
>>>>>>> documented somewhere.
>>>>>>
>>>>>>>> TrackData is the octave, note, effect and effect data for each
>>>>>>>> row,
>>>>>>>> Track itself defines the structure containing it, i.e. number of
>>>>>>>> rows,
>>>>>>>> maybe track name, etc.
>>>>>>>
>>>>>>> Maybe AVSequencerRowData then?
>>>>>>
>>>>>> Nice name, will think on it!
>>>
>>> So?
>>
>> AVSequencerTrackRow?
>>
>> Just to make the hierarchy clearer...agree on this?
>
> yes.

Fixed.

>
>>>>>>>>
>>>>>>>> This would require extending using 0x80-0xFF command effect which
>>>>>>>> would
>>>>>>>> conflict with synth sound handling (see there at the synth sound
>>>>>>>> instructions).
>>>>>>>
>>>>>>> Ok, let me pose the question differently. Suppose we have an effect
>>>>>>> foo. In MOD, effect foo is command 0x66 while in XM it is command
>>>>>>> 0x55. What is the value of AVSEQ_TRACK_EFFECT_CMD_FOO? Or such
>>>>>>> thing
>>>>>>> never happens?
>>>>>>
>>>>>> This happens very frequently, esp. between MOD/XM<=>    S3M/IT.
>>>>>> But this is part of the demuxer to convert the origins...
>>>>>
>>>>> Sure, but then doing as you do:
>>>>>
>>>>>       /** 0x66 - Do foo to stuff */
>>>>>        #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
>>>>>
>>>>> is confusing and misleading in comparison with
>>>>>
>>>>>       /** Do foo to stuff */
>>>>>        #define AVSEQ_TRACK_EFFECT_CMD_FOO 0x66
>>>
>>> So?
>>
>> Sorry, don't understand here, what you mean.
>
> I mean that if the choice of 0x66 for AVSEQ_TRACK_EFFECT_CMD_FOO is
> merely a convention, it should not be reinforced in the comment.

Fixed.

>
>>>> diff --git a/libavsequencer/track.h b/libavsequencer/track.h
>>>> new file mode 100755
>>>> index 0000000..cf131af
>>>> --- /dev/null
>>>> +++ b/libavsequencer/track.h
>>>> @@ -0,0 +1,1636 @@
>>>> +/*
>>>> + * AVSequencer track and pattern 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_TRACK_H
>>>> +#define AVSEQUENCER_TRACK_H
>>>> +
>>>> +#include "libavutil/log.h"
>>>> +#include "libavformat/avformat.h"
>>>> +
>>>
>>>
>>>> +/**
>>>> + * Song track effect structure, This structure is actually for one
>>>> row
>>>> + * and therefore actually pointed as an array with the amount of
>>>> + * rows of the whole track.
>>>> + * 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 AVSequencerTrackEffect {
>>>> +    /** Effect command byte.  */
>>>> +    uint8_t command;
>>>
>>> enum...
>>
>> Again where's the problem? enums are all above.
>>
>>>
>>>> +    /** Effect command data word.  */
>>>> +    uint16_t data;
>>>> +} AVSequencerTrackEffect;
>>>> +
>>>> +/** AVSequencerTrackEffect->note values.  */
>>>> +enum AVSequencerTrackDataNote {
>>>> +    /** ---  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_NONE          = 0,
>>>> +
>>>> +    /** C-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_C             = 1,
>>>> +
>>>> +    /** C#n = Dbn  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_C_SHARP       = 2,
>>>> +
>>>> +    /** D-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_D             = 3,
>>>> +
>>>> +    /** D#n = Ebn  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_D_SHARP       = 4,
>>>> +
>>>> +    /** E-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_E             = 5,
>>>> +
>>>> +    /** F-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_F             = 6,
>>>> +
>>>> +    /** F#n = Gbn  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_F_SHARP       = 7,
>>>> +
>>>> +    /** G-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_G             = 8,
>>>> +
>>>> +    /** G#n = Abn  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_G_SHARP       = 9,
>>>> +
>>>> +    /** A-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_A             = 10,
>>>> +
>>>> +    /** A#n = Bbn  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_A_SHARP       = 11,
>>>> +
>>>> +    /** B-n = H-n  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_B             = 12,
>>>> +
>>>> +    /** ^^^ = note kill  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_KILL          = -1,
>>>> +
>>>> +    /** ^^- = note off  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_OFF           = -2,
>>>> +
>>>> +    /** === = keyoff note  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_KEYOFF        = -3,
>>>> +
>>>> +    /** -|- = hold delay  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_HOLD_DELAY    = -4,
>>>> +
>>>> +    /** -\- = note fade  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_FADE          = -5,
>>>> +
>>>> +    /** END = pattern end marker  */
>>>> +    AVSEQ_TRACK_DATA_NOTE_END           = -16,
>>>> +};
>>>
>>> Why hardcoding values? Why not just use:
>>>
>>> /** AVSequencerTrackEffect->note values.  */
>>> enum AVSequencerTrackDataNote {
>>>      /** ---  */
>>>      AVSEQ_TRACK_DATA_NOTE_NONE,
>>>
>>>      /** C-n  */
>>>      AVSEQ_TRACK_DATA_NOTE_C,
>>>
>>>      /** C#n = Dbn  */
>>>      AVSEQ_TRACK_DATA_NOTE_C_SHARP,
>>>
>>>      [...]
>>> }
>>>
>>> And let the compiler assign whatever numbers it seems fit?
>>
>> Would require every demuxer/muxer to convert these values if they don't
>> match,
>
> Every format uses the same convention?

For the positive notes, this is the case, as far as I know. For the
negative special note effects it differs. But most use negative values,
too, if I remember correctly, so you know it's a special note by just
checking for < 0.

Anyway, I fixed this by removing the 1 to 12 assignments and just set
NONE to 0, the compiler automatically always adds one in a subsequent
enum, right?

This is required, since the player uses a lut for calculating the target
frequency which is base_freq * (octave - 4) * 2^((note - 1)/12).

-- 

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