[MPlayer-dev-eng] [RFC] Updated color management patch

[Diego Biurrun]

On Tue, Jan 19, 2010 at 10:59:27AM -0500, Yue Shi Lai wrote:
>
> Attached is an updated version of the color management patch to MPlayer.  
> I have been contacted my several users of wide gamut displays regarding  
> an updated version that patches against the current SVN revision.
>
> Note that there has been modifications since the first version I posted  
> on this list. This version has been updated to allow multithreaded CLUT  
> initialization and lookup.
>
> diff -urNp mplayer/Makefile mplayer-vf_cm/Makefile

Not using 'svn diff'?  Should be much easier...

> --- mplayer/Makefile	2010-01-18 17:52:57.000000000 +0000
> +++ mplayer-vf_cm/Makefile	2010-01-18 21:11:48.000000000 +0000
> @@ -172,6 +172,7 @@ SRCS_LIBMPEG2-$(HAVE_ALTIVEC)        += 
>  SRCS_LIBMPEG2-$(HAVE_MMX)            += libmpeg2/idct_mmx.c \
>                                          libmpeg2/motion_comp_mmx.c
>  SRCS_LIBMPEG2-$(HAVE_VIS)            += libmpeg2/motion_comp_vis.c
> +SRCS_COMMON-$(LCMS)                  += libmpcodecs/vf_cm.c
>  SRCS_COMMON-$(LIBMPEG2)              += libmpcodecs/vd_libmpeg2.c \
>                                          libmpeg2/alloc.c \
>                                          libmpeg2/cpu_accel.c\

This was previously in proper alphabetical order, maintain it.

> @@ -911,6 +912,10 @@ VIDIX_OBJS = $(filter vidix/%,$(SRCS_MPL
>  
>  $(VIDIX_DEPS) $(VIDIX_OBJS): $(VIDIX_PCI_FILES)
>  
> +PROFILEDIR = $(DATADIR)/icc
> +
> +libmpcodecs/vf_cm%: CFLAGS += -DPROFILEDIR=\"$(PROFILEDIR)\"

This hack is not acceptable.  Place it in config.h if you must.

> --- mplayer/configure	2010-01-18 17:52:57.000000000 +0000
> +++ mplayer-vf_cm/configure	2010-01-18 21:11:14.000000000 +0000
> @@ -6242,6 +6247,22 @@ else
>  
> +echocheck "Little CMS"
> +if test "$_lcms" = auto ; then
> + _lcms=no
> + def_lcms='#undef LCMS'
> + cat > $TMPC <<EOF

Use two space indentation.

> --- mplayer/libmpcodecs/vf.c	2010-01-18 17:52:31.000000000 +0000
> +++ mplayer-vf_cm/libmpcodecs/vf.c	2010-01-18 21:12:21.000000000 +0000
> @@ -48,6 +48,9 @@ extern const vf_info_t vf_info_yvu9;
>  extern const vf_info_t vf_info_eq;
>  extern const vf_info_t vf_info_eq2;
> +#ifdef LCMS
> +extern const vf_info_t vf_info_cm;
> +#endif /* LCMS */

pointless #ifdef.

> --- mplayer/libmpcodecs/vf_cm.c	1970-01-01 00:00:00.000000000 +0000
> +++ mplayer-vf_cm/libmpcodecs/vf_cm.c	2010-01-18 21:10:25.000000000 +0000
> @@ -0,0 +1,2858 @@
> +
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <math.h>
> +#include <stdio.h>
> +
> +#include "config.h"
> +#include "cpudetect.h"
> +#include "mp_msg.h"
> +#include "mp_image.h"
> +#include "img_format.h"
> +#include "libavutil/avutil.h"
> +#include "cpudetect.h"
> +#include "libvo/fastmemcpy.h"
> +#include "subopt-helper.h"
> +#include "vf.h"
> +#include "vf_cm.h"
> +
> +#include <lcms.h>

Place all system headers together.

> +#ifndef PATH_MAX
> +#define PATH_MAX        4096    /* Maximum size of the file names */
> +#endif /* PATH_MAX */

This looks like some system workaround and I believe we have it
somewhere already.

> +typedef struct {
> +} lcms_abpc_half_transform_t;
> +
> +typedef struct {
> +} lcms_abpc_transform_t;

The _t namespace is reserved for POSIX, do not further pollute it.
Also, I dislike typedefs for structs.

> +static void itu_rec709_trc(LPGAMMATABLE *trc)
> +{
> +    if(*trc == NULL)

!*trc, same below

> +    for(i = 0; i < NGAMMA; i++) {
> +
> +        if(x <= 0.081 /* = 4.5 * 0.018 */)

Please use K&R style, i.e. space between if/for/while/while and (, more below.

> +static void clut_rgb2yuv(
> +    uint8_t buf_out[3], uint8_t buf_in[3], int bt_709)

Indentation is off, just

  static void clut_rgb2yuv(uint8_t buf_out[3], uint8_t buf_in[3], int bt_709)

is fine, if the line is too long break it as

  static void clut_rgb2yuv(uint8_t buf_out[3],
                           uint8_t buf_in[3], int bt_709)

or similar, more below.

> +        pclut[stride] = av_clip_uint8(
> +            (FIXPOINT_FRACTION(387, 448) * pclut[0] -

same

> +#ifdef HAVE_SSE2

#if

> diff -urNp mplayer/libvo/gl_common.c mplayer-vf_cm/libvo/gl_common.c
> --- mplayer/libvo/gl_common.c	2010-01-18 17:52:40.000000000 +0000
> +++ mplayer-vf_cm/libvo/gl_common.c	2010-01-18 21:15:45.000000000 +0000
> @@ -34,6 +34,9 @@
>  #include <math.h>
>  #include "gl_common.h"
>  #include "csputils.h"
> +#ifdef LCMS
> +#include "libmpcodecs/vf_cm.h"
> +#endif /* LCMS */

I think the #ifdef is unneeded.

> --- mplayer/libvo/vo_gl.c	2010-01-18 17:52:40.000000000 +0000
> +++ mplayer-vf_cm/libvo/vo_gl.c	2010-01-18 21:09:44.000000000 +0000
> @@ -36,6 +36,9 @@
>  #endif
>  #include "fastmemcpy.h"
>  #include "libass/ass_mp.h"
> +#ifdef LCMS
> +#include "libmpcodecs/vf_cm.h"
> +#endif /* LCMS */

same

Diego