[FFmpeg-devel] Various small fixes

[Måns Rullgård]

"Anders Gr?nberg" <galileo.m2 at gmail.com> writes:

> On Sat, Dec 13, 2008 at 3:52 PM, M?ns Rullg?rd <mans at mansr.com> wrote:
>> "Anders Gr?nberg" <galileo.m2 at gmail.com> writes:
>>
>>> I have been experimenting with FFmpeg and this patch fixes some small
>>> issues I found in the code base that should be beneficial to other
>>> FFmpeg developers.
>>>
>>> Index: libavcodec/dsputil.c
>>> ===================================================================
>>> --- libavcodec/dsputil.c      (revision 16089)
>>> +++ libavcodec/dsputil.c      (working copy)
>>> @@ -3458,7 +3458,7 @@
>>>  static void diff_bytes_c(uint8_t *dst, uint8_t *src1, uint8_t *src2, int w){
>>>      long i;
>>>  #ifndef HAVE_FAST_UNALIGNED
>>> -    if((long)src2 & (sizeof(long)-1)){
>>> +    if((intptr_t)src2 & (sizeof(src2)-1)){
>>>          for(i=0; i+7<w; i+=8){
>>>              dst[i+0] = src1[i+0]-src2[i+0];
>>>              dst[i+1] = src1[i+1]-src2[i+1];
>>> @@ -4186,7 +4186,7 @@
>>>      static int did_fail=0;
>>>      DECLARE_ALIGNED_16(int, aligned);
>>>
>>> -    if((long)&aligned & 15){
>>> +    if((intptr_t)&aligned & 15){
>>>          if(!did_fail){
>>>  #if defined(HAVE_MMX) || defined(HAVE_ALTIVEC)
>>>              av_log(NULL, AV_LOG_ERROR,
>>
>> There was some system that didn't have intptr_t (although it is
>> required), so using long (or int) here is more portable.  What do you
>> suppose this change fixes?
> intptr_t is not required by the c99 standard but provided by most

Right, they are optional.  I must have confused it with something
else.  Anyhow, that only strengthens the argument against using it
when not strictly necessary.

> compiler and its used in FFmpeg.

The places it is used are conditionally compiled, and also need to
preserve all bits from the pointer.

> I change it because I think the intptr_t is slightly more correct.

What makes you say that?  Using an unsigned type would arguably be
safer.  However, when only the four least significant bits are
interesting, the width of the type used is irrelevant.

>>> Index: libavcodec/dsputil.h
>>> ===================================================================
>>> --- libavcodec/dsputil.h      (revision 16089)
>>> +++ libavcodec/dsputil.h      (working copy)
>>> @@ -712,11 +712,11 @@
>>>   * @param   n       size of half window
>>>   */
>>>  void ff_sine_window_init(float *window, int n);
>>> -extern float ff_sine_128 [ 128];
>>> -extern float ff_sine_256 [ 256];
>>> -extern float ff_sine_512 [ 512];
>>> -extern float ff_sine_1024[1024];
>>> -extern float ff_sine_2048[2048];
>>> +DECLARE_ALIGNED(16, extern float, ff_sine_128 [ 128]);
>>> +DECLARE_ALIGNED(16, extern float, ff_sine_256 [ 256]);
>>> +DECLARE_ALIGNED(16, extern float, ff_sine_512 [ 512]);
>>> +DECLARE_ALIGNED(16, extern float, ff_sine_1024[1024]);
>>> +DECLARE_ALIGNED(16, extern float, ff_sine_2048[2048]);
>>>  extern float *ff_sine_windows[5];
>>>
>>>  int ff_mdct_init(MDCTContext *s, int nbits, int inverse);
>>
>> These are extern declarations of data.  Using DECLARE_ALIGNED here
>> will not make change anything.
> The compiler could use the alignment information to generate faster

I don't think so.  Those arrays are used almost exclusively as
arguments to functions, whose arguments the compiler cannot know
anything about.  Besides, those functions are already written to take
advantage of alignment where possible.

> code and similar code already exists in aactab.h.

That code could quite possibly be wrong.

>>> Index: libavformat/mov.c
>>> ===================================================================
>>> --- libavformat/mov.c (revision 16089)
>>> +++ libavformat/mov.c (working copy)
>>> @@ -166,7 +166,7 @@
>>>      int (*parse)(MOVContext *ctx, ByteIOContext *pb, MOVAtom atom);
>>>  } MOVParseTableEntry;
>>>
>>> -static const MOVParseTableEntry mov_default_parse_table[];
>>> +const MOVParseTableEntry mov_default_parse_table[];
>>>
>>>  static int mov_read_default(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
>>>  {
>>
>> Why?
> The c99 standard says that a forward declaration of an array of
> unknown size should not contain the static keyword, it should only be
> present when the finale size is known. A static forward declaration of
> an array of unknown size is undefined behavior.

Then the code should be rearranged to avoid the need for this
declaration.

> Although if it doesn't compile on gcc it should be left as it is.

So you didn't test this at all?

>>> Index: libavutil/common.h
>>> ===================================================================
>>> --- libavutil/common.h        (revision 16089)
>>> +++ libavutil/common.h        (working copy)
>>> @@ -108,7 +108,7 @@
>>>  /* assume b>0 */
>>>  #define ROUNDED_DIV(a,b) (((a)>0 ? (a) + ((b)>>1) : (a) - ((b)>>1))/(b))
>>>  #define FFABS(a) ((a) >= 0 ? (a) : (-(a)))
>>> -#define FFSIGN(a) ((a) > 0 ? 1 : -1)
>>> +#define FFSIGN(a) ((a) >= 0 ? 1 : -1)
>>>
>>>  #define FFMAX(a,b) ((a) > (b) ? (a) : (b))
>>>  #define FFMAX3(a,b,c) FFMAX(FFMAX(a,b),c)
>>
>> Did this fix something?  Are you sure it didn't break something?
> Most specifications I have read assumes that 0 == -0 and defines 0
> as positive.  If the current behavior is correct then maybe the
> macro should be renamed.

The number zero is neither positive nor negative.  If it works as is,
why do you want to change it?

I get the impression you are running some kind of lint-like tool and
blindly "fixing" things it complains about.  In my experience, this
usually does more harm than good.

-- 
M?ns Rullg?rd
mans at mansr.com