[MPlayer-dev-eng] [PATCH] VDPAU: add BT.709 support

[Reimar Döffinger]

On Tue, Oct 06, 2009 at 01:41:17PM +0300, Lauri Mylläri wrote:
> +    VdpStatus vdp_st;
> +    VdpCSCMatrix matrix;
> +    VdpColorStandard vdp_color;
> +    static const VdpVideoMixerAttribute attributes[] = {VDP_VIDEO_MIXER_ATTRIBUTE_CSC_MATRIX};
> +    const void *attribute_values[] = {&matrix};
> +
> +    switch (colorspace) {
> +    case 1:
> +        vdp_color = VDP_COLOR_STANDARD_ITUR_BT_601;
> +        break;
> +    case 2: 
> +        vdp_color = VDP_COLOR_STANDARD_ITUR_BT_709;
> +        break;
> +    default:
> +        /* TODO: some videos have flags indicating colorspace */
> +        if ((vid_width >= 1280) || (vid_height > 576))
> +            vdp_color = VDP_COLOR_STANDARD_ITUR_BT_709;
> +        else
> +            vdp_color = VDP_COLOR_STANDARD_ITUR_BT_601;
> +    }
> +
> +    mp_msg(MSGT_VO, MSGL_V, "[vdpau] Updating CSC matrix for %s\n",
> +           vdp_color == VDP_COLOR_STANDARD_ITUR_BT_709
> +                ? "BT.709"
> +                : ( vdp_color == VDP_COLOR_STANDARD_ITUR_BT_601 ? "BT.601"
> +                                                                : "other" ) );
> +
> +    vdp_st = vdp_generate_csc_matrix(&procamp, vdp_color,
> +                                     &matrix);
> +    CHECK_ST_WARNING("Error when generating CSC matrix")

Less code and easier to extend IMO:
static const VdpColorStandard vdp_colors[] = {0, VDP_COLOR_STANDARD_ITUR_BT_601, VDP_COLOR_STANDARD_ITUR_BT_709};
static const char * const vdp_names[] = {NULL, "BT.601", "BT.709"};
int csp = colorspace;

if (csp < 0 || csp > 2)
    csp = 1; // add a warning or error out while parsing
if (!csp)
    csp = vid_width >= 1280 || vid_height > 576 ? 2 : 1;
mp_msg(MSGT_VO, MSGL_V, "[vdpau] Updating CSC matrix for %s\n", vdp_names[csp]);
vdp_st = vdp_generate_csc_matrix(&procamp, vdp_colors[csp], &matrix);

This also restricts the vdp_generate_csc_matrix-specific code to two
arrays and two code lines, making it easier to add support for
completely custom matrices.


> +#ifdef DEBUG
> +    mp_msg(MSGT_VO, MSGL_V, "csc matrix\n");
> +    for (int y = 0; y < 3; y++) {
> +        mp_msg(MSGT_VO, MSGL_V, "  %d:", y + 1);
> +        for (int x = 0; x < 4; x++)
> +            mp_msg(MSGT_VO, MSGL_V, "\t%f", matrix[y][x]);
> +        mp_msg(MSGT_VO, MSGL_V, "\n");
> +    }
> +#endif

I think it is better to remove this, doesn't seem useful enough to
justify 7 lines of code in "production" code - particularly since
by the time someone needs it, like most debug code it probably wouldn't
even compile anymore.