[FFmpeg-soc] [soc] libavsequencer [PATCH 01/08] Music module public API header file.

[Vitor Sessak]

On 07/15/2010 02:28 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/15/2010 12:18 AM, Sebastian Vater wrote:
>>>
>>> So here is it, I merged them all into one to avoid flooding the ML.
>>> They're getting small enough now, to merge them all into one, I think!
>>
>> This is not a patch against latest SVN, this is a patch against your
>> SoC tree and thus unreadable (your patch should contain all the lines
>> of libavsequencer/* since none of this files exists now in main svn).
>
> Fixed.
>
>>
>>> diff --git a/libavsequencer/track.h b/libavsequencer/track.h
>>> index c1370ed..33095d9 100755
>>> --- a/libavsequencer/track.h
>>> +++ b/libavsequencer/track.h
>>> @@ -34,12 +34,18 @@
>>>    * version bump.
>>>    */
>>
>> [...]
>>
>>>       /** 0x20 - Set volume:
>>
>> Again, is saying "0x20" in the comment relevant? If this value for the
>> set volume command is format-dependent (i.e., it is only 0x20 in MOD
>> but 0x55 in XM), this is misleading.
>
> The demuxer/decoder will convert this to effect number 0x20 anyway, in
> MOD the set volume command is C (internally = 0x02). The goal is to have
> a unique command set independent from the origin file.
>
>>
>>>          Data word consists of two 8-bit pairs named xx and yy
>>>          where xx is the volume level (ranging either from
>>> @@ -756,7 +729,8 @@ typedef struct AVSequencerTrackEffect {
>>>          means that the tremolo envelope values will be inverted.  */
>>>   #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE  0x2F
>>
>> Wasn't you going to change these (and many other) to a enum? I
>> remember you said you would change it soon, but being afraid of
>> breaking other files that uses this header, but what code exactly does
>> replacing
>
> Yes, will do this tonight.
>
>>
>> #define AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE  0x2F
>> #define AVSEQ_TRACK_EFFECT_CMD_STOP_FX       0x1D
>>
>>
>> by
>>
>> enum Whatever {
>>      AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F,
>>      AVSEQ_TRACK_EFFECT_CMD_STOP_FX      = 0x1D,
>> }
>>
>> breaks?
>>
>
> I access them like:
> if (flags&  AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
>
> Won't have change that to:
> if (flags&  Whatever.AVSEQ_TRACK_EFFECT_CMD_STOP_FX) { .. }
>
> then?

No, at least in my box the following compiles fine:

> enum Whatever {
>      AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE = 0x2F,
>      AVSEQ_TRACK_EFFECT_CMD_STOP_FX      = 0x1D,
> };
>
> int f(int flags)
> {
>     if (flags & AVSEQ_TRACK_EFFECT_CMD_T_TREMO_ONCE)
>         return 1;
>     else
>         return 0;
> }

Finally, wasn't you going to remove all the defines for the default 
values by creating a function that init them? Can you send a new patch 
after applying these two major changes (ie, using enum instead of 
defines and avoiding the defines for the default values)? Ideally, you 
should send a new patch after all the feedback was already taken into 
account...

-Vitor