[FFmpeg-devel] FFmpeg source code is no longer C99 because of GNUism called case ranges

[Måns Rullgård]

Roman Shaposhnik <rvs at sun.com> writes:

> On Mon, 2007-06-25 at 09:09 +0100, M?ns Rullg?rd wrote:
>> > My question, of course, is -- would it be acceptable to make code C99
>> > compliant again at an expense of making it slightly less regular?
>> 
>> The main code should be C99 compliant, extensions only allowed under
>> appropriate #ifdefs and always with a proper fallback (think asm()).
>> 
>> Patches, clean of course, to fix issues like the ones you mention are
>> always welcome.
>
>   Ok. Looks like there's no much disagreement on this one. Thus I'm
> attaching my first cut at making the code C99 compliant again.
>
>   Michael, please take a look. Does this seem acceptable or would you
> want something fancier?
>
> Thanks,
> Roman.
>
> Index: ffmpeg/libavcodec/rv10.c
> ===================================================================
> --- ffmpeg/libavcodec/rv10.c	(revision 9470)
> +++ ffmpeg/libavcodec/rv10.c	(working copy)
> @@ -539,43 +539,38 @@
>      s->h263_long_vectors= ((uint8_t*)avctx->extradata)[3] & 1;
>      avctx->sub_id= AV_RB32((uint8_t*)avctx->extradata + 4);
>  
> -    switch(avctx->sub_id){
> -    case 0x10000000:
> -        s->rv10_version= 0;
> +    if (avctx->sub_id < 0x20000000) {
> +        switch(avctx->sub_id){
> +        case 0x10000000:
> +            s->rv10_version= 0;
> +            s->low_delay=1;
> +            break;
> +        case 0x10002000:
> +            s->rv10_version= 3;
> +            s->low_delay=1;
> +            s->obmc=1;
> +            break;
> +        case 0x10003000:
> +        case 0x10003001:
> +            s->rv10_version= 3;
> +            s->low_delay=1;
> +            break;
> +        default:
> +            av_log(s->avctx, AV_LOG_ERROR, "unknown header %X\n", avctx->sub_id);
> +        }
> +    }
> +    else if (avctx->sub_id == 0x20001000 ||
> +             (avctx->sub_id >= 0x20100000 && avctx->sub_id < 0x201a0000)) {
>          s->low_delay=1;
> -        break;
> -    case 0x10002000:
> -        s->rv10_version= 3;
> -        s->low_delay=1;
> -        s->obmc=1;
> -        break;
> -    case 0x10003000:
> -        s->rv10_version= 3;
> -        s->low_delay=1;
> -        break;
> -    case 0x10003001:
> -        s->rv10_version= 3;
> -        s->low_delay=1;
> -        break;
> -    case 0x20001000: /* real rv20 decoder fail on this id */
> -    /*case 0x20100001:
> -    case 0x20101001:
> -    case 0x20103001:*/
> -    case 0x20100000 ... 0x2019ffff:
> -        s->low_delay=1;
> -        break;
> -    /*case 0x20200002:
> -    case 0x20201002:
> -    case 0x20203002:*/
> -    case 0x20200002 ... 0x202fffff:
> -    case 0x30202002:
> -    case 0x30203002:
> +    }
> +    else if (avctx->sub_id == 0x30202002 ||
> +             avctx->sub_id == 0x30203002 ||
> +             (avctx->sub_id >= 0x20200002 && avctx->sub_id < 0x20300000)) {
>          s->low_delay=0;
>          s->avctx->has_b_frames=1;
> -        break;
> -    default:
> -        av_log(s->avctx, AV_LOG_ERROR, "unknown header %X\n", avctx->sub_id);
>      }
> +    else
> +        av_log(s->avctx, AV_LOG_ERROR, "unknown header %X\n", avctx->sub_id);
>  
>      if(avctx->debug & FF_DEBUG_PICT_INFO){
>          av_log(avctx, AV_LOG_DEBUG, "ver:%X ver0:%X\n", avctx->sub_id, avctx->extradata_size >= 4 ? ((uint32_t*)avctx->extradata)[0] : -1);

This has the appearance of something that could possibly be done with
some bit masking and arithmetic instead of listing all the
possibilities.

> Index: ffmpeg/libavformat/mp3.c
> ===================================================================
> --- ffmpeg/libavformat/mp3.c	(revision 9470)
> +++ ffmpeg/libavformat/mp3.c	(working copy)
> @@ -234,7 +234,8 @@
>          taghdrlen = 6;
>          break;
>  
> -    case 3 ... 4:
> +    case 3:
> +    case 4:
>          isv34 = 1;
>          taghdrlen = 10;
>          break;

Looks good to me.  I think you can safely apply this.  Michael, beat
if you disagree.

> Index: ffmpeg/libavformat/raw.c
> ===================================================================
> --- ffmpeg/libavformat/raw.c	(revision 9470)
> +++ ffmpeg/libavformat/raw.c	(working copy)
> @@ -355,28 +355,32 @@
>      return 0;
>  }
>  
> -#define VISUAL_OBJECT_START_CODE       0x000001b5
> -#define VOP_START_CODE                 0x000001b6
> +#define VISUAL_OBJECT_START_CODE       0xb5
> +#define VOP_START_CODE                 0xb6
>  
>  static int mpeg4video_probe(AVProbeData *probe_packet)
>  {
>      uint32_t temp_buffer= -1;
> +    uint8_t b;
>      int VO=0, VOL=0, VOP = 0, VISO = 0, res=0;
>      int i;
>  
>      for(i=0; i<probe_packet->buf_size; i++){
>          temp_buffer = (temp_buffer<<8) + probe_packet->buf[i];
> -        if ((temp_buffer & 0xffffff00) == 0x100) {
> -            switch(temp_buffer){
> -            case VOP_START_CODE:             VOP++; break;
> -            case VISUAL_OBJECT_START_CODE:  VISO++; break;
> -            case 0x100 ... 0x11F:             VO++; break;
> -            case 0x120 ... 0x12F:            VOL++; break;
> -            case 0x130 ... 0x1AF:
> -            case 0x1B7 ... 0x1B9:
> -            case 0x1C4 ... 0x1FF:            res++; break;
> -            }
> -        }
> +        if ((temp_buffer & 0xffffff00) != 0x100)
> +            continue;
> +
> +        b = probe_packet->buf[i];
> +        if (b == VOP_START_CODE)
> +            VOP++;
> +        else if (b == VISUAL_OBJECT_START_CODE)
> +            VISO++;
> +        else if (b < 0x20)
> +            VO++;
> +        else if (b < 0x30)
> +            VOL++;
> +        else
> +            res += !((b > 0xAF && b < 0xB7) || (b > 0xB9 && b < 0xC4));
>      }
>  
>      if ( VOP >= VISO && VOP >= VOL && VO >= VOL && VOL > 0 && res==0)

Why the new variable?  The logic looks correct though.

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