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

[Vitor Sessak]

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.

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

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

-Vitor