[Cogl] [PATCH 2/6] framebuffer: Bind the framebuffer before querying the bits
Robert Bragg
robert at sixbynine.org
Wed Jan 16 06:02:37 PST 2013
This looks good to land to me:
Reviewed-by: Robert Bragg <robert at linux.intel.com>
thanks,
- Robert
On Fri, Dec 14, 2012 at 11:43 AM, Neil Roberts <neil at linux.intel.com> wrote:
> The GL framebuffer driver now makes sure to bind the framebuffer
> before counting the number of bits. Previously it would just query the
> number of bits for whatever framebuffer happened to be used last.
>
> In addition the virtual for querying the framebuffer bits has been
> modified to take a pointer to a structure instead of a separate
> pointer to each component. This should make it slightly more efficient
> and easier to maintain.
> ---
> cogl/cogl-driver.h | 5 +-
> cogl/cogl-framebuffer-private.h | 13 ++--
> cogl/cogl-framebuffer.c | 29 ++++-----
> cogl/driver/gl/cogl-framebuffer-gl-private.h | 5 +-
> cogl/driver/gl/cogl-framebuffer-gl.c | 90 ++++++++++++--------------
> cogl/driver/nop/cogl-framebuffer-nop-private.h | 5 +-
> cogl/driver/nop/cogl-framebuffer-nop.c | 12 +---
> tests/conform/test-conform-main.c | 4 +-
> 8 files changed, 71 insertions(+), 92 deletions(-)
>
> diff --git a/cogl/cogl-driver.h b/cogl/cogl-driver.h
> index ba6e030..ab2274f 100644
> --- a/cogl/cogl-driver.h
> +++ b/cogl/cogl-driver.h
> @@ -75,10 +75,7 @@ struct _CoglDriverVtable
>
> void
> (* framebuffer_query_bits) (CoglFramebuffer *framebuffer,
> - int *red,
> - int *green,
> - int *blue,
> - int *alpha);
> + CoglFramebufferBits *bits);
>
> void
> (* framebuffer_finish) (CoglFramebuffer *framebuffer);
> diff --git a/cogl/cogl-framebuffer-private.h b/cogl/cogl-framebuffer-private.h
> index 457b3dd..25f183b 100644
> --- a/cogl/cogl-framebuffer-private.h
> +++ b/cogl/cogl-framebuffer-private.h
> @@ -105,6 +105,14 @@ typedef enum
> COGL_READ_PIXELS_NO_FLIP = 1L << 30
> } CoglPrivateReadPixelsFlags;
>
> +typedef struct
> +{
> + int red;
> + int blue;
> + int green;
> + int alpha;
> +} CoglFramebufferBits;
> +
> struct _CoglFramebuffer
> {
> CoglObject _parent;
> @@ -162,10 +170,7 @@ struct _CoglFramebuffer
>
> /* driver specific */
> CoglBool dirty_bitmasks;
> - int red_bits;
> - int blue_bits;
> - int green_bits;
> - int alpha_bits;
> + CoglFramebufferBits bits;
>
> int samples_per_pixel;
> };
> diff --git a/cogl/cogl-framebuffer.c b/cogl/cogl-framebuffer.c
> index dd81001..06dd388 100644
> --- a/cogl/cogl-framebuffer.c
> +++ b/cogl/cogl-framebuffer.c
> @@ -847,47 +847,44 @@ int
> cogl_framebuffer_get_red_bits (CoglFramebuffer *framebuffer)
> {
> CoglContext *ctx = framebuffer->context;
> - int red, green, blue, alpha;
> + CoglFramebufferBits bits;
>
> - ctx->driver_vtable->framebuffer_query_bits (framebuffer,
> - &red, &green, &blue, &alpha);
> + ctx->driver_vtable->framebuffer_query_bits (framebuffer, &bits);
>
> - return red;
> + return bits.red;
> }
> +
> int
> cogl_framebuffer_get_green_bits (CoglFramebuffer *framebuffer)
> {
> CoglContext *ctx = framebuffer->context;
> - int red, green, blue, alpha;
> + CoglFramebufferBits bits;
>
> - ctx->driver_vtable->framebuffer_query_bits (framebuffer,
> - &red, &green, &blue, &alpha);
> + ctx->driver_vtable->framebuffer_query_bits (framebuffer, &bits);
>
> - return green;
> + return bits.green;
> }
>
> int
> cogl_framebuffer_get_blue_bits (CoglFramebuffer *framebuffer)
> {
> CoglContext *ctx = framebuffer->context;
> - int red, green, blue, alpha;
> + CoglFramebufferBits bits;
>
> - ctx->driver_vtable->framebuffer_query_bits (framebuffer,
> - &red, &green, &blue, &alpha);
> + ctx->driver_vtable->framebuffer_query_bits (framebuffer, &bits);
>
> - return blue;
> + return bits.blue;
> }
>
> int
> cogl_framebuffer_get_alpha_bits (CoglFramebuffer *framebuffer)
> {
> CoglContext *ctx = framebuffer->context;
> - int red, green, blue, alpha;
> + CoglFramebufferBits bits;
>
> - ctx->driver_vtable->framebuffer_query_bits (framebuffer,
> - &red, &green, &blue, &alpha);
> + ctx->driver_vtable->framebuffer_query_bits (framebuffer, &bits);
>
> - return alpha;
> + return bits.alpha;
> }
>
> CoglColorMask
> diff --git a/cogl/driver/gl/cogl-framebuffer-gl-private.h b/cogl/driver/gl/cogl-framebuffer-gl-private.h
> index 5ab3817..2e0c97a 100644
> --- a/cogl/driver/gl/cogl-framebuffer-gl-private.h
> +++ b/cogl/driver/gl/cogl-framebuffer-gl-private.h
> @@ -50,10 +50,7 @@ _cogl_framebuffer_gl_clear (CoglFramebuffer *framebuffer,
>
> void
> _cogl_framebuffer_gl_query_bits (CoglFramebuffer *framebuffer,
> - int *red,
> - int *green,
> - int *blue,
> - int *alpha);
> + CoglFramebufferBits *bits);
>
> void
> _cogl_framebuffer_gl_finish (CoglFramebuffer *framebuffer);
> diff --git a/cogl/driver/gl/cogl-framebuffer-gl.c b/cogl/driver/gl/cogl-framebuffer-gl.c
> index 23cf1a7..e622d91 100644
> --- a/cogl/driver/gl/cogl-framebuffer-gl.c
> +++ b/cogl/driver/gl/cogl-framebuffer-gl.c
> @@ -891,54 +891,54 @@ _cogl_framebuffer_init_bits (CoglFramebuffer *framebuffer)
> {
> CoglContext *ctx = framebuffer->context;
>
> - cogl_framebuffer_allocate (framebuffer, NULL);
> -
> if (G_LIKELY (!framebuffer->dirty_bitmasks))
> return;
>
> + cogl_framebuffer_allocate (framebuffer, NULL);
> +
> + _cogl_framebuffer_flush_state (framebuffer,
> + framebuffer,
> + COGL_FRAMEBUFFER_STATE_BIND);
> +
> #ifdef HAVE_COGL_GL
> if ((ctx->private_feature_flags &
> COGL_PRIVATE_FEATURE_QUERY_FRAMEBUFFER_BITS) &&
> framebuffer->type == COGL_FRAMEBUFFER_TYPE_OFFSCREEN)
> {
> - GLenum attachment, pname;
> -
> - attachment = GL_COLOR_ATTACHMENT0;
> -
> - pname = GL_FRAMEBUFFER_ATTACHMENT_RED_SIZE;
> - GE( ctx, glGetFramebufferAttachmentParameteriv (GL_FRAMEBUFFER,
> - attachment,
> - pname,
> - &framebuffer->red_bits) );
> -
> - pname = GL_FRAMEBUFFER_ATTACHMENT_GREEN_SIZE;
> - GE( ctx, glGetFramebufferAttachmentParameteriv (GL_FRAMEBUFFER,
> - attachment,
> - pname,
> - &framebuffer->green_bits)
> - );
> -
> - pname = GL_FRAMEBUFFER_ATTACHMENT_BLUE_SIZE;
> - GE( ctx, glGetFramebufferAttachmentParameteriv (GL_FRAMEBUFFER,
> - attachment,
> - pname,
> - &framebuffer->blue_bits)
> - );
> -
> - pname = GL_FRAMEBUFFER_ATTACHMENT_ALPHA_SIZE;
> - GE( ctx, glGetFramebufferAttachmentParameteriv (GL_FRAMEBUFFER,
> - attachment,
> - pname,
> - &framebuffer->alpha_bits)
> - );
> + static const struct
> + {
> + GLenum attachment, pname;
> + size_t offset;
> + } params[] =
> + {
> + { GL_COLOR_ATTACHMENT0, GL_FRAMEBUFFER_ATTACHMENT_RED_SIZE,
> + offsetof (CoglFramebufferBits, red) },
> + { GL_COLOR_ATTACHMENT0, GL_FRAMEBUFFER_ATTACHMENT_GREEN_SIZE,
> + offsetof (CoglFramebufferBits, green) },
> + { GL_COLOR_ATTACHMENT0, GL_FRAMEBUFFER_ATTACHMENT_BLUE_SIZE,
> + offsetof (CoglFramebufferBits, blue) },
> + { GL_COLOR_ATTACHMENT0, GL_FRAMEBUFFER_ATTACHMENT_ALPHA_SIZE,
> + offsetof (CoglFramebufferBits, alpha) },
> + };
> + int i;
> +
> + for (i = 0; i < G_N_ELEMENTS (params); i++)
> + {
> + int *value =
> + (int *) ((uint8_t *) &framebuffer->bits + params[i].offset);
> + GE( ctx, glGetFramebufferAttachmentParameteriv (GL_FRAMEBUFFER,
> + params[i].attachment,
> + params[i].pname,
> + value) );
> + }
> }
> else
> #endif /* HAVE_COGL_GL */
> {
> - GE( ctx, glGetIntegerv (GL_RED_BITS, &framebuffer->red_bits) );
> - GE( ctx, glGetIntegerv (GL_GREEN_BITS, &framebuffer->green_bits) );
> - GE( ctx, glGetIntegerv (GL_BLUE_BITS, &framebuffer->blue_bits) );
> - GE( ctx, glGetIntegerv (GL_ALPHA_BITS, &framebuffer->alpha_bits) );
> + GE( ctx, glGetIntegerv (GL_RED_BITS, &framebuffer->bits.red) );
> + GE( ctx, glGetIntegerv (GL_GREEN_BITS, &framebuffer->bits.green) );
> + GE( ctx, glGetIntegerv (GL_BLUE_BITS, &framebuffer->bits.blue) );
> + GE( ctx, glGetIntegerv (GL_ALPHA_BITS, &framebuffer->bits.alpha) );
> }
>
>
> @@ -948,29 +948,23 @@ _cogl_framebuffer_init_bits (CoglFramebuffer *framebuffer)
> framebuffer->type == COGL_FRAMEBUFFER_TYPE_OFFSCREEN
> ? "offscreen"
> : "onscreen",
> - framebuffer->red_bits,
> - framebuffer->blue_bits,
> - framebuffer->green_bits,
> - framebuffer->alpha_bits);
> + framebuffer->bits.red,
> + framebuffer->bits.blue,
> + framebuffer->bits.green,
> + framebuffer->bits.alpha);
>
> framebuffer->dirty_bitmasks = FALSE;
> }
>
> void
> _cogl_framebuffer_gl_query_bits (CoglFramebuffer *framebuffer,
> - int *red,
> - int *green,
> - int *blue,
> - int *alpha)
> + CoglFramebufferBits *bits)
> {
> _cogl_framebuffer_init_bits (framebuffer);
>
> /* TODO: cache these in some driver specific location not
> * directly as part of CoglFramebuffer. */
> - *red = framebuffer->red_bits;
> - *green = framebuffer->green_bits;
> - *blue = framebuffer->blue_bits;
> - *alpha = framebuffer->alpha_bits;
> + *bits = framebuffer->bits;
> }
>
> void
> diff --git a/cogl/driver/nop/cogl-framebuffer-nop-private.h b/cogl/driver/nop/cogl-framebuffer-nop-private.h
> index 568cdea..9cec46b 100644
> --- a/cogl/driver/nop/cogl-framebuffer-nop-private.h
> +++ b/cogl/driver/nop/cogl-framebuffer-nop-private.h
> @@ -53,10 +53,7 @@ _cogl_framebuffer_nop_clear (CoglFramebuffer *framebuffer,
>
> void
> _cogl_framebuffer_nop_query_bits (CoglFramebuffer *framebuffer,
> - int *red,
> - int *green,
> - int *blue,
> - int *alpha);
> + CoglFramebufferBits *bits);
>
> void
> _cogl_framebuffer_nop_finish (CoglFramebuffer *framebuffer);
> diff --git a/cogl/driver/nop/cogl-framebuffer-nop.c b/cogl/driver/nop/cogl-framebuffer-nop.c
> index c4c1348..a7938ad 100644
> --- a/cogl/driver/nop/cogl-framebuffer-nop.c
> +++ b/cogl/driver/nop/cogl-framebuffer-nop.c
> @@ -29,7 +29,7 @@
> #include "cogl-framebuffer-nop-private.h"
>
> #include <glib.h>
> -
> +#include <string.h>
>
> void
> _cogl_framebuffer_nop_flush_state (CoglFramebuffer *draw_buffer,
> @@ -62,15 +62,9 @@ _cogl_framebuffer_nop_clear (CoglFramebuffer *framebuffer,
>
> void
> _cogl_framebuffer_nop_query_bits (CoglFramebuffer *framebuffer,
> - int *red,
> - int *green,
> - int *blue,
> - int *alpha)
> + CoglFramebufferBits *bits)
> {
> - *red = 0;
> - *green = 0;
> - *blue = 0;
> - *alpha = 0;
> + memset (bits, 0, sizeof (CoglFramebufferBits));
> }
>
> void
> diff --git a/tests/conform/test-conform-main.c b/tests/conform/test-conform-main.c
> index cbee5b2..8f2f2d5 100644
> --- a/tests/conform/test-conform-main.c
> +++ b/tests/conform/test-conform-main.c
> @@ -98,9 +98,7 @@ main (int argc, char **argv)
> ADD_TEST (test_bitmask, 0, 0);
>
> ADD_TEST (test_offscreen, 0, 0);
> - ADD_TEST (test_framebuffer_get_bits,
> - TEST_REQUIREMENT_OFFSCREEN,
> - TEST_KNOWN_FAILURE);
> + ADD_TEST (test_framebuffer_get_bits, TEST_REQUIREMENT_OFFSCREEN, 0);
>
> ADD_TEST (test_point_size, 0, 0);
> ADD_TEST (test_point_sprite,
> --
> 1.7.11.3.g3c3efa5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
More information about the Cogl
mailing list