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

[Reimar Döffinger]

Sorry for not reviewing properly, it's just too large for that...

On Tue, Jan 19, 2010 at 10:59:27AM -0500, Yue Shi Lai wrote:
> @@ -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)\"

I suspect this belongs in config.h

> +echocheck "Little CMS"
> +if test "$_lcms" = auto ; then
> + _lcms=no
> + def_lcms='#undef LCMS'

Doesn't set def_lcms when --disable-lcms is used.

> +#include <lcms.h>
> +int main(void) { return 0; }
> +EOF
> + cc_check -llcms && _lcms=yes

No reliable, you should actually use a symbol from liblcms

> --- 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_lavcdeint;
>  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 */

Use less ifdef


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

I don't think that comment line fits quite there.

> +#define NGAMMA          1024    /* Sampling points for tone
> +                                   reproduction curve (TRC) */
> +#define CMS_PIXELFMT    TYPE_RGB_8  /* Pixel format used in CMS */
> +#define CMS_STRIDE      3       /* Size of CMS_PIXELFMT, >= 3 */
> +#define CLUT_STRIDE     3       /* Size per CLUT pixel, >= 3 */
> +/* The default CLUT used in this filter is an 65 point (per channel),
> +   which is twice the (linear) size typically used by CMM */
> +#define CLUT_DECIMATION 4       /* Inexact CLUT spline decimation */
> +#define GAMUT_WARN_GRAY 153     /* Default gamut warning gray */

And since you already have comments you might as well make the doxygen-compatible.
E.g. by using ///<

> + * @param[out] trc the tone reproduction curve in Little CMS format
> + */
> +static void itu_rec709_trc(LPGAMMATABLE *trc)

Makes more sense to make this the return value than an argument.

> +        pclut[stride] = av_clip_uint8(
> +            (33 * pclut[0] +
> +             28 * pclut[4 * stride] +
> +              3 * pclut[8 * stride]) >> 6);
> +        pclut[2 * stride] = av_clip_uint8(
> +            (pclut[0] + 3 * pclut[4 * stride]) >> 2);
> +        pclut[3 * stride] = av_clip_uint8(
> +            ( 7 * pclut[0] +
> +             60 * pclut[4 * stride] -
> +              3 * pclut[8 * stride]) >> 6);

A few intermediate variables would probably help readability a lot.

> +    /* Aligned body, 1st pass */
> +    asm (

Better use
__asm__

> +        /* End of loop */
> +        "loop      1b"

Isn't loop generally considered one of the worst instructions to use?

> +        : "=m" (factor[0]), "=m" (factor[1]), "=m" (factor[2]),
> +          /* 3 */
> +          "=m" (factor[3]), "=m" (factor[4])
> +        : /* 5 */
> +          "m" (factor[0]), "m" (factor[1]), "m" (factor[2]),
> +          /* 8 */
> +          "m" (factor[3]), "m" (factor[4]),
> +          /* 10 */
> +          "g" (pclut), "r" (stride), "g" (count)
> +        : "%" REG_c, "%" REG_d, "%" REG_D,
> +          "%xmm0", "%xmm1", "%xmm2", "%xmm3",
> +          "%xmm4", "%xmm5", "%xmm6", "%xmm7");

I suspect you really need to hope for gcc to have a good day to be
able to compile this.
However you are specifying the same variable multiple time, even if
that's valid I doubt whether it means what you want it to.

I suspect something like
> +        : "+&m" (factor[0]), "+&m" (factor[1]), "+&m" (factor[2]),
> +          "+&m" (factor[3]), "+&m" (factor[4])
> +        : "g" (pclut), "r" (stride), "g" (count)

or the for x86 needlessly verbose variant of

> +        : "=m" (factor[0]), "=m" (factor[1]), "=m" (factor[2]),
> +          /* 3 */
> +          "=m" (factor[3]), "=m" (factor[4])
> +        : /* 5 */
> +          "0" (factor[0]), "1" (factor[1]), "2" (factor[2]),
> +          /* 8 */
> +          "3" (factor[3]), "4" (factor[4]),
> +          /* 10 */
> +          "g" (pclut), "r" (stride), "g" (count)

Are at least more correct.

> +        : "%" REG_a, "%" REG_d, "%" REG_S, "%" REG_D,

And temporary variables with "=&r" contraints give more flexibilty
on 64 bit systems.

> +          "g" (pclut), "r" (stride), "g" (count)
> +        : "%" REG_a, "%" REG_c, "%" REG_d, "%" REG_S, "%" REG_D,

Has no chance to compile on x86 systems with PIC enabled.
While we don't really support them, breaking them is not so great still.

> +    const size_t alignment = (size_t)clut & 7;
> +    const unsigned int head = alignment == 0 ? 0 : 8 - alignment;

(8 - alignment) & 7
or quick and ugly
-clut & 7

> +static const char *trc_name(const unsigned int trc_type)

I consider that const kind of const for the argument a hindrance at
best, not that I mind much though.
However using a enum for the type might make sense...

> +    /* Clip the source black point */
> +    if(source_black_point.L > 50)
> +        source_black_point.L = 50;

FFMIN?

> +    a = sum_x * (sum_x3 * sum_y + sum_x2 * sum_x_y -
> +                 sum_x * sum_x2_y) +
> +        n * (sum_x2 * sum_x2_y - sum_x3 * sum_x_y) -
> +        sum_x2 * sum_x2 * sum_y;
> +    b = sum_x * (sum_x2 * sum_x2_y - sum_x4 * sum_y) +
> +        n * (sum_x4 * sum_x_y - sum_x3 * sum_x2_y) +
> +        sum_x2 * (sum_x3 * sum_y - sum_x2 * sum_x_y);
> +    c = sum_x3 * (sum_x * sum_x2_y + sum_x2 * sum_x_y -
> +                   sum_x3 * sum_y) +
> +        sum_x2 * (sum_x4 * sum_y - sum_x2 * sum_x2_y) -
> +        sum_x * sum_x4 * sum_x_y;

I'd suspect there might be a more human-readable way to write that.

> +    static const char *suffix[] = {

static const char * const suffix[] = {

> +    static const char *user_directory[] = {

static const char * const user_directory[] = {

> +    char *path[] = {
> +        directory[0], directory[1], directory[2],

const char *
and use directory[i] where you write then instead path[i]

> +        snprintf(path[i], PATH_MAX, "%s/%s",
> +                 home_directory, user_directory[i]);
> +        /* Enforce path string termination */
> +        path[i][PATH_MAX - 1] = '\0';

snprintf does guarantee that already.

> +    /* Wee need to intercept inaccessible ICC files now, or Little CMS
> +       might terminate MPlayer abruptly, and corrupting the terminal
> +       line. */
> +    if(fp == NULL) {

That's a rather serious bug for a library and a workaround does not really belong
in MPlayer (a corrupted terminal is not exactly the end of the world).

> +    default:
> +        /* This should never happen */

The assert(0) is the most appropriate code for it.

> +    if(p->lcms_src_profile == NULL)
> +        if(!create_profile(&p->lcms_src_profile, p->src_prim,
> +                           p->src_trc_type, p->src_gamma, "Source"))

if(!p->lcms_src_profile)
would be more consitent

> +    /* Print out the ICC intent of the transformation, see also
> +       http://www.color.org/profile.html */
> +    mp_msg(MSGT_VFILTER, MSGL_INFO, "[cm] "
> +           "Using %s intent\n",
> +           p->intent == INTENT_PERCEPTUAL ? "perceptual" :
> +           p->intent == INTENT_RELATIVE_COLORIMETRIC ?
> +           "media-relative colorimetric" :
> +           p->intent == INTENT_SATURATION ? "saturation" :
> +           p->intent == INTENT_ABSOLUTE_COLORIMETRIC ?
> +           "ICC-absolute colorimetric" : "unknown");

Ugh, unreadable. Use a funtion, possibly with a lookup table or something
to get the string.

> +    /* Compute the CLUT */
> +    p->clut = (uint8_t *)malloc(

useless cast.

> +        p->clut_resolution * p->clut_resolution * p->clut_resolution *
> +        CLUT_STRIDE * sizeof(uint8_t));

* sizeof(uint8_t) is quite pointless, sizeof(*p->clut) might make sense.

> +                /* For efficiency, generate one plane each time and
> +                   perform the CMM transform on the whole plane. */
> +                uint8_t buf_in_rgb[planesize * CMS_STRIDE];
> +                uint8_t buf_out_rgb[planesize * CMS_STRIDE];

I don't think that large arrays should be placed on the stack.

> +                buf_pixel[1] = cb <= 255 ? cb : 255;

FFMIN(cb, 255)
And there are many like those.

> +#if 0
> +    {
> +        /* Test code: write out the CLUT */

#define DUMP_CLUT 0
if (DUMP_CLUT) {
...

will ensure that debug code will still compile by the time someone tries to use it.

> +        uint16_t y_avg =
> +            ((uint16_t)py_in[0] + (uint16_t)py_in[1] +
> +             (uint16_t)py_in1[0] + (uint16_t)py_in1[1] + 2) >> 2;

What is supposed to be the point of those casts?

> +        (*g_cm.clut_src_pixfmt == IMGFMT_YV12 ||
> +         *g_cm.clut_src_pixfmt == IMGFMT_I420 ||
> +         *g_cm.clut_src_pixfmt == IMGFMT_IYUV) &&

> +        const uint8_t uint8_y =
> +            av_clip_uint8(y * *g_cm.clut_resolution);
> +        const uint8_t uint8_cb =
> +            av_clip_uint8(cb * *g_cm.clut_resolution);
> +        const uint8_t uint8_cr =
> +            av_clip_uint8(cr * *g_cm.clut_resolution);
> +        const size_t offset =
> +            ((uint8_cb * *g_cm.clut_resolution +
> +              uint8_cr) * *g_cm.clut_resolution +
> +             uint8_y) * g_cm.clut_stride;

A local variable for those often-used values shoudl improve
readability.
And those uint8_cb etc. names make it really hard to read since
types and names are hardly distinguishable.

> +    static opt_t opt[] = {

static const nowadays

> +    if(optval.src_prim) {
> +        if(optval.src_prim[0] != '\0') {
> +    if(optval.src_trc) {
> +        if(optval.src_trc[0] != '\0') {
> +    if(optval.dest_prim) {
> +        if(optval.dest_prim[0] != '\0') {
> +    if(optval.dest_trc) {
> +        if(optval.dest_trc[0] != '\0') {
> +    if(optval.intent) {
> +        if(optval.intent[0] != '\0') {

Merge the ifs, you can call free unconditionally.

> +        vf->priv = malloc(sizeof(struct vf_priv_s));

calloc should make some of the explicit initializations unnecessary.

> @@ -1084,6 +1113,11 @@ static void create_conv_textures(gl_conv
>          texs[0] = (*texu)++;
>          ActiveTexture(GL_TEXTURE0 + texs[0]);
>          lookup_data = malloc(3 * sz * sz * sz);
> +#ifdef LCMS
> +        if(vf_cm_clut_is_ycbcr_to_rgb())
> +            gen_yuv2rgb_map_cm(params, lookup_data, LOOKUP_3DRES);
> +        else
> +#endif /* LCMS */
>          mp_gen_yuv2rgb_map(&params->csp_params, lookup_data, LOOKUP_3DRES);

I think it would be better if mp_gen_yuv2rgb_map used gen_yuv2rgb_map_cm when requested.

> diff -urNp mplayer/libvo/vo_gl.c mplayer-vf_cm/libvo/vo_gl.c
> --- 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 */
>  
>  static const vo_info_t info =
>  {
> @@ -1160,6 +1163,9 @@ static int preinit(const char *arg)
>      mp_msg(MSGT_VO, MSGL_V, "[gl] Using %d as slice height "
>               "(0 means image height).\n", slice_height);
>      if (!init_mpglcontext(&glctx, gltype)) return -1;
> +#ifdef LCMS
> +    vf_cm_set_vo_clut_lookup();
> +#endif /* LCMS */

Can't that somehow be handled by gen_yuv2rgb_map_cm?