[FFmpeg-devel] FFmpeg source code is no longer C99 because of GNUism called case ranges
Måns Rullgård
mans
Thu Jul 5 09:02:36 CEST 2007
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
More information about the ffmpeg-devel
mailing list