[cairo] [PATCH] [gl] Add shader support code for GL versions < 3.0.

[Chris Wilson]

On Thu, 14 Jan 2010 11:49:04 -0600, Zach Laine <whatwasthataddress at gmail.com> wrote:
> Adds cairo_gl_shader_program_t, and functions to manipulate same.  Multiple GL
> entry points for shaders are provided -- one for the pre-GL 2.0 extenstions
> entry points, and one for GL 2.0.  This code is well tested, but currently
> unused in the GL backend.

Hi Zach, this looks good and is definitely the direction we want to be
going in, but... ;-)

I've some specific comments on style inline, but first let's look at the
general design. cairo-gl is based around the idea of classifying the
operation using its cairo_gl_composite_setup_t. This may contain 4
channels, not just source, but mask, clipmask and possibly a copy of the
destination as a texture (all depending on the operation) and a program
has to be compiled to execute that in one or more passes (depending on
hardware and available resources). In that represent, I feel the fixed
programs are too simplistic. For example, look at the approach taken by
glitz which pre-compiles all possible combinations of source and masks,
and that is still just a small fraction of the number of programs we will
need to run, for example, WebKit without falling back to a multi-pass
composite. So the interface that I'd like to see is where we can pass in a
cairo_gl_composite_setup_t (or whatever) and have the shader manager
select the right program (and caching the results). The best example of
how I think this should look is:
  http://cgit.freedesktop.org/~ickle/cairo/tree/src/drm/cairo-drm-i965-shader.c?h=wip/compositor
or
  http://cgit.freedesktop.org/~ickle/cairo/tree/src/drm/cairo-drm-i915-shader.c?h=wip/compositor

Eric will be able to point to some more apt examples for GL, in
particular his glamor includes a shader manager and has a very similar job
to the gl-compositor.

[snip]

> +void
> +init_shader_program (cairo_gl_shader_program_t *program);

The OO convention for cairo is to prefix the object name to the function,
and also internal functions should be to be marked private and with a
leading underscore:

cairo_private void
_cairo_gl_shader_program_init (cairo_gl_shader_program_t *program);

This is enforced by make check, you have been warned. ;-)

Within a source file, you are free to use whatever consistent convention
makes the code most readable.

[snip]

> diff --git a/src/cairo-gl-shader.c b/src/cairo-gl-shader.c
> new file mode 100644
> index 0000000..52f43f3
> --- /dev/null
> +++ b/src/cairo-gl-shader.c
> @@ -0,0 +1,653 @@
> +/* cairo - a vector graphics library with display and print output
> + *
> + * Copyright © 2009 T. Zachary Laine

2009,2010 now ;-)

[snip]

> +
> +/* OpenGL Core 2.0 API. */
> +static cairo_status_t
> +compile_shader_core_2_0 (GLuint *shader, GLenum type, const char *text)
> +{
> +    const char* strings[1] = { text };
> +    GLint gl_status;
> +
> +    *shader = glCreateShader (type);
> +    glShaderSource (*shader, 1, strings, 0);
> +    glCompileShader (*shader);
> +    glGetShaderiv (*shader, GL_COMPILE_STATUS, &gl_status);
> +    if (gl_status == GL_FALSE) {
> +        GLint log_size;
> +        glGetShaderiv (*shader, GL_INFO_LOG_LENGTH, &log_size);
> +        if (0 < log_size) {
> +            char *log = _cairo_malloc (log_size);
> +            GLint chars;
> +
> +            log[log_size - 1] = '\0';
> +            glGetShaderInfoLog (*shader, log_size, &chars, log);
> +            printf ("OpenGL shader compilation failed.  Shader:\n"
> +                    "%s\n"
> +                    "OpenGL compilation log:\n"
> +                    "%s\n",
> +                    text, log);
> +
> +            free (log);
> +        } else {
> +            printf ("OpenGL shader compilation failed.\n");
> +        }
> +
> +        return CAIRO_INT_STATUS_UNSUPPORTED;
> +    }
> +
> +    return CAIRO_STATUS_SUCCESS;
> +}

You are writing library code, so use of the standard file descriptors is
verboten (you have no idea what they have been redirected to). If you
must, use stderr for warnings/errors, but wrap it in a compile time
debugging feature.

If a function is simply returning a boolean success/failure, just return a
boolean. Reserve cairo_status_t for when you do actually want to return an
propagate a fatal error code.

Other than those minor quibbles, good stuff!
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre