[Ffmpeg-devel] Some possible libswscale cleanups?

[Michael Niedermayer]

Hi

On Thu, Dec 21, 2006 at 04:05:53PM +0100, Luca Abeni wrote:
> Hi all,
> 
> as people might already have noticed, building libswscale in the ffmpeg
> tree generates a lot of warnings. Some of them, such as
> swscale.c: In function `yuv2packedXinC':
> swscale.c:661: warning: pointer of type `void *' used in arithmetic
> swscale.c:667: warning: pointer of type `void *' used in arithmetic
> swscale.c:678: warning: pointer of type `void *' used in arithmetic
> swscale.c:697: warning: pointer of type `void *' used in arithmetic
> [...]
> are due to the fact that the table_rV, table_gU, and table_bU fields in
> SwsContext are "void *". I do not know if these warnings justify a
> change from "void *" to "uint8_t *", but in case such change is ok the
> attached remove_void_pointers_arithmetic.diff removes the warnings.

f*cking gcc devels, arithemtic with void should not trigger a warning
its invalid, this is not meaningfull in c gcc shouldnt compile this


> 
> On one of my test machines I also see this:
> gcc -DHAVE_AV_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_ISOC9X_SOURCE -I"/tmp/ffmpeg" -I"/tmp/ffmpeg" -I"/tmp/ffmpeg"/libavutil -fomit-frame-pointer -g -Wall -Wno-switch -Wpointer-arith -Wredundant-decls -O3  -c -o swscale.o swscale.c
> In file included from swscale.c:65:
> /usr/include/malloc.h:118: warning: redundant redeclaration of `malloc' in same scope
> /usr/include/stdlib.h:556: warning: previous declaration of `malloc'
> /usr/include/malloc.h:122: warning: redundant redeclaration of `calloc' in same scope
> /usr/include/stdlib.h:559: warning: previous declaration of `calloc'
> In file included from /tmp/ffmpeg/libavutil/common.h:35,
>                  from /tmp/ffmpeg/libavutil/avutil.h:44,
>                  from swscale_internal.h:28,
>                  from swscale.c:76:
> [...]
> I think "#include <malloc.h>" is not needed now (it was needed when
> libswscale used memalign()), so the attached remove_useless_include.diff
> removes it, eliminating the warning.

ok


> 
> After applying the two attached patches, the only remaining warnings are
> gcc -DHAVE_AV_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_ISOC9X_SOURCE -I"/tmp/ffmpeg" -I"/tmp/ffmpeg" -I"/tmp/ffmpeg"/libavutil -fomit-frame-pointer -g -Wall -Wno-switch -Wpointer-arith -Wredundant-decls -O3  -c -o swscale.o swscale.c
> swscale.c: In function `yuv2packedXinC':
> swscale.c:780: warning: unused variable `g'
> swscale.c:780: warning: unused variable `b'
> swscale.c:780: warning: unused variable `r'
> swscale.c:788: warning: unused variable `g'
> swscale.c:788: warning: unused variable `b'
> swscale.c:788: warning: unused variable `r'
> (and I see no easy way to remove them).

attribute_unused ?


> 
> I do not know if the two patches are ok, if there are better ways to
> address these warnings, or if it is not worth removing these warnings.
> Let me know, and I'll rework the patches as needed.
> 
> 
> I also noticed that swscale.c contains some 
> FFMIN(FFMAX(..., 0), 255);
> Maybe it would be good to change them in clip_uint8()? If yes, let me
> know and I'll provide a patch.

yes

[...]

> Index: swscale.c
> ===================================================================
> --- swscale.c.orig	2006-12-21 15:06:30.000000000 +0100
> +++ swscale.c	2006-12-21 15:07:02.000000000 +0100
> @@ -422,9 +422,9 @@
>                          
>  #define YSCALE_YUV_2_RGBX_C(type) \
>  			YSCALE_YUV_2_PACKEDX_C(type)\
> -			r = c->table_rV[V];\
> -			g = c->table_gU[U] + c->table_gV[V];\
> -			b = c->table_bU[U];\
> +			r = (type *)c->table_rV[V];\
> +			g = (type *)c->table_gU[U] + c->table_gV[V];\
> +			b = (type *)c->table_bU[U];\

void* + int != random_type* + int
or did i miss something (iam too lazy too look at the code ATM ...)


[...]
> @@ -1786,8 +1786,8 @@
>  	int y=      srcSliceY;
>  	int height= srcSliceH;
>  	int i, j;
> -	uint16_t *srcPtr= src[0];
> -	uint16_t *dstPtr= dst[0] + dstStride[0]*y/2;
> +	uint16_t *srcPtr= (uint16_t *)src[0];
> +	uint16_t *dstPtr= (uint16_t *)dst[0] + dstStride[0]*y/2;

this also doesnt look correct for the same reason (/2 vs. 16bits)


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes