[MPlayer-dev-eng] [PATCH] VF Overlay

[Diego Biurrun]

On Tue, Jul 28, 2009 at 10:30:00PM +0200, Benjamin Zores wrote:
> Diego Biurrun wrote:
> >On Mon, Jul 27, 2009 at 09:26:21PM +0200, Benjamin Zores wrote:
> >>I intend to apply it within the week unless someone's complain about it,
> >>as it shouldn't harm any other part of MPlayer.
> >
> >This has lots of issues, apart from ugly formatting.  Don't apply it
> >as-is.
> >
> >I'm in a hurry now, more substantial comments later.  But you could
> >really review it yourself first, I'm convinced you'll find a few things.
> >At least this could be formatted as proper K&R.
> 
> Attached an updated version with format changes, doc update and a test 
> tool as a bonus.

I haven't had time to review the docs yet, but here are some comments.

> --- libmpcodecs/vf_overlay.c	(revision 0)
> +++ libmpcodecs/vf_overlay.c	(revision 0)
> @@ -0,0 +1,1422 @@
> +/*
> + * Copyright 2007 Jason Tackaberry <tack at urandom.ca>

Place this below the short description.

> +#ifdef HAVE_MALLOC_H
> +#include <malloc.h>
> +#endif
> +#ifdef HAVE_SYS_MMAN_H
> +#include <sys/mman.h>
> +#endif

#if, these are 0/1 definitions now that are always available.

> +//#define STOPWATCH 8     /* Useful for profiling. */
> +//#undef HAVE_MMX         /* Turn off MMX for debugging. */
> +
> +#define C64(x) ((uint64_t)((x)|(x)<<16))<<32|(uint64_t)(x)|(uint64_t)(x)<<16
> +#define clamp(a,min,max) (((a)>(max))?(max):(((a)<(min))?(min):(a)))

Geez, this must come from a land where the use of the space key was
forbidden...

> +static void premultiply_alpha_byte_8_MMX(uint8_t *byte, uint8_t *alpha,
> +                                         uint8_t *dst_byte,
> +                                         uint8_t *dst_alpha,
> +                                         int global_alpha)
> +{
> +    asm volatile(

Extra good karma for prettyprinting the asm, but no, I won't insist.

> --- libmpcodecs/vf.c	(revision 29447)
> +++ libmpcodecs/vf.c	(working copy)
> @@ -100,6 +100,9 @@
>  extern const vf_info_t vf_info_ow;
> +#ifdef HAVE_SHM
> +extern vf_info_t vf_info_overlay;
> +#endif

pointless #ifdef around extern declaration

> --- Makefile	(revision 29447)
> +++ Makefile	(working copy)
> @@ -521,6 +521,7 @@
>  
> +SRCS_COMMON-$(NEED_SHMEM)    += libmpcodecs/vf_overlay.c

This cannot be the right condition.

> --- TOOLS/overlay-test.c	(revision 0)
> +++ TOOLS/overlay-test.c	(revision 0)
> @@ -0,0 +1,79 @@
> +/* Tool to test vf_overlay filter */
> +/* Compile with: gcc -Wall overlay.c -o overlay -lSDL -lSDL_image -lrt */

Please add a proper license header and hook this up in the Makefile.
Also, new files should follow K&R style.

Diego