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

[Diego Biurrun]

On Thu, Aug 06, 2009 at 11:51:57PM +0200, Benjamin Zores wrote:
> 
> Thx for the other comments, applied the changes in this newly attached 
> patch.
> 
> --- libmpcodecs/vf_overlay.c	(revision 0)
> +++ libmpcodecs/vf_overlay.c	(revision 0)
> @@ -0,0 +1,1323 @@
> +
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#if HAVE_MALLOC_H
> +#include <malloc.h>
> +#endif
> +#include <string.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/param.h>
> +#include <sys/time.h>
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +#if HAVE_SYS_MMAN_H
> +#include <sys/mman.h>
> +#endif
> +#include <inttypes.h>
> +
> +#include "mp_msg.h"
> +#include "libvo/fastmemcpy.h"
> +#include "libvo/video_out.h"
> +#include "libswscale/swscale.h"
> +#include "input/input.h"
> +#include "osdep/timer.h"
> +#include "cpudetect.h"
> +#include "mangle.h"
> +
> +#include "mp_image.h"
> +#include "vf.h"
> +#include "img_format.h"
> +#include "libavutil/avutil.h"
> +#include "vf_scale.h"

Are all those headers really needed?

> +//#define HAVE_MMX 0         /* Turn off MMX for debugging. */

debug cruft

> +    uint8_t *y;       ///< Luma plane
> +    uint8_t *u;       ///< Chroma (Cb) plane
> +    uint8_t *v;       ///< Chroma (Cr) plane
> +    uint8_t *a;       ///< Alpha plane for luma channel
> +    uint8_t *uva;     ///< Alpha plane for chroma channel
> +    uint8_t *pre_y;   ///< Luma plane with pre-alpha-multiplied pixels
> +    uint8_t *pre_u;   ///< Chroma (Cb) plane with pre-alpha-multiplied pixels
> +    uint8_t *pre_v;   ///< Chroma (Cr) plane with pre-alpha-multiplied pixels
> +    uint8_t *pre_a;   /**< Alpha plane for luma channel where pixels are
> +                         averaged with global alpha */
> +    uint8_t *pre_uva; /**< Alpha plane for chroma channel where pixels are
> +                         averaged with global alpha */

I wouldn't capitalize all those non-sentences, same below.

> +    /// The shared memory id as gotten from shm_get().

ID, s/gotten/received/

> +#if HAVE_MMX
> +static uint64_t attribute_used __attribute__((aligned(8))) MM_global_alpha;
> +static uint64_t attribute_used __attribute__((aligned(8))) MM_ROUND = C64(0x80);
> +#endif

I think you can move this down where it is used, will save one #ifdef.

> +    /* Round sizes up to multiples of 2. */
> +    r->w = (w + 1) & ~1;
> +    r->h = (h + 1) & ~1;
> +    r->type = type;
> +    r->next = NULL;

Align all 4 lines.

> +    asm volatile(

extra good karma for prettyprinting the asm..

> --- Makefile	(revision 29478)
> +++ Makefile	(working copy)
> @@ -521,6 +521,7 @@
>                stream/url.c \
>                $(SRCS_COMMON-yes)
>  
> +SRCS_COMMON-$(HAVE_SHM)      += libmpcodecs/vf_overlay.c
>  
>  SRCS_MPLAYER-$(3DFX)         += libvo/vo_3dfx.c
>  SRCS_MPLAYER-$(AA)           += libvo/vo_aa.c

This looks misplaced, put it in proper alphabetical order.

> --- DOCS/tech/vf_overlay.txt	(revision 0)
> +++ DOCS/tech/vf_overlay.txt	(revision 0)
> @@ -0,0 +1,225 @@
> +
> +   * Compositing is MMX accelerated and performs quite well. On a 2GHz

MMX-accelerated

> --- DOCS/tech/slave.txt	(revision 29478)
> +++ DOCS/tech/slave.txt	(working copy)
> @@ -201,6 +201,10 @@
>  
> +overlay <command>
> +    Manipulate the overlay filter.  See DOCS/tech/vf_overlay.txt for a

nit: s/  / /

> --- DOCS/man/en/mplayer.1	(revision 29478)
> +++ DOCS/man/en/mplayer.1	(working copy)
> @@ -7182,6 +7182,34 @@
>  .
>  .TP
> +.B overlay=shmkey
> +Provides an overlay image buffer that can be accessed via shared memory.
> +This filter can be used by applications controlling MPlayer to provide a
> +custom on-screen display.
> +The overlay image is composited over the running video and supports global 
> +and per-pixel alpha blending.
> +Pixels are specified in BGRA format, and the size of the overlay image is the
> +video display size.

Pixels are specified in BGRA format.
The size of the overlay image is the video display size.

Please add a small note about TOOLS/overlay-test.c to TOOLS/README.

Diego