[Ffmpeg-devel] [PATCH] from DivX, Part 7: MSVC fixes

[Måns Rullgård]

Michael Niedermayer said:
> Hi
>
> On Fri, Dec 16, 2005 at 01:49:16PM -1000, Steve Lhomme wrote:
>> These are only the "soft" part of the MSVC fixes. That means I didn't
>> include parts I know wouldn't make it, like the named fields in structures.
>
> [...]
>>  #else //TRACE
>> -#define tprintf(...) {}
>> + #if __STDC_VERSION__ >= 199901L
>> +  #define tprintf(...) {}
>> + #else
>> +  inline void tprintf(char *x, ...) {}
>> + #endif
>>  #endif
>
> hmm, are you sure that 199901L is the oldest which supports this? and even
> if so, isnt it better to check for the compiler instead of the standard?
> a compiler might support 98% of C99 but might not set
> __STDC_VERSION__ == 199901L as it doesnt support 100%
> this comment applies to all __STDC_VERSION__ changes

With some proper care, a portable way of disabling tprintf is

#define tprintf if(0)

>> +#if __STDC_VERSION__ >= 199901L
>>      uint8_t fixed[s->mb_stride * s->mb_height];
>> +#else
>> +    uint8_t *fixed=(uint8_t*)alloca(s->mb_stride * s->mb_height);
>> +#endif
>
> rejected
>
> #define A(type, name, size) type name[size];
> #define A(type, name, size) type *name= alloca(size * sizeof(type));
>
> would be cleaner but even then i would say that needs some disscussion and
> should be a seperate patch

This is a somewhat dangerous thing to do, since it makes sizeof(name)
unpredictable.

>> Index: libavcodec/mjpeg.c
>> ===================================================================
>> RCS file: /cvsroot/ffmpeg/ffmpeg/libavcodec/mjpeg.c,v
>> retrieving revision 1.114
>> diff -u -r1.114 mjpeg.c
>> --- libavcodec/mjpeg.c	18 Sep 2005 21:21:01 -0000	1.114
>> +++ libavcodec/mjpeg.c	16 Dec 2005 23:16:01 -0000
>> @@ -495,7 +495,7 @@
>>      int size= put_bits_count(&s->pb) - start*8;
>>      int i, ff_count;
>>      uint8_t *buf= s->pb.buf + start;
>> -    int align= (-(size_t)(buf))&3;
>> +    int align= (-(int)(buf))&3;
>
> this is going to trigger warnings on some compilers i think ...

Yes, on 64-bit targets.  Casting to intptr_t should be ok, as should long.
(The Linux kernel depends on long being compatible with pointers.)

>> +    if (avctx->sub_id >= 0x20100000 && avctx->sub_id <= 0x2019ffff)
>> +        s->low_delay=1;
>> +    else if (avctx->sub_id >= 0x20200002 && avctx->sub_id <= 0x202fffff)
>> +    {
>> +        s->low_delay=0;
>> +        s->avctx->has_b_frames=1;
>> +    }
>> +    else
>>      switch(avctx->sub_id){
>>      case 0x10000000:
>>          s->rv10_version= 0;
>> @@ -541,13 +549,11 @@
>>      /*case 0x20100001:
>>      case 0x20101001:
>>      case 0x20103001:*/
>> -    case 0x20100000 ... 0x2019ffff:
>>          s->low_delay=1;
>>          break;
>>      /*case 0x20200002:
>>      case 0x20201002:
>>      case 0x20203002:*/
>> -    case 0x20200002 ... 0x202fffff:
>
> rejected, dont replace clean code with such a mess

Non-portable mess to boot.  It is certainly not allowed in C89.  Not sure
about C99.

>> Index: libavformat/nsvdec.c
>> ===================================================================
>> RCS file: /cvsroot/ffmpeg/ffmpeg/libavformat/nsvdec.c,v
>> retrieving revision 1.11
>> diff -u -r1.11 nsvdec.c
>> --- libavformat/nsvdec.c	12 Dec 2005 01:56:46 -0000	1.11
>> +++ libavformat/nsvdec.c	16 Dec 2005 23:32:26 -0000
>> @@ -362,7 +362,7 @@
>>          if((unsigned)table_entries >= UINT_MAX / sizeof(uint32_t))
>>              return -1;
>>          nsv->nsvf_index_data = av_malloc(table_entries * sizeof(uint32_t));
>> -#warning "FIXME: Byteswap buffer as needed"
>> +/* #warning "FIXME: Byteswap buffer as needed" */
>
> sorry no, there is #ifdef for that

Why not fix the real problem instead?  That code fails on big endian machines.

-- 
M?ns Rullg?rd
mru at inprovide.com