[Mesa-dev] [PATCH] mesa: rewrite accum buffer support

[Jose Fonseca]

----- Original Message -----
> Implemented in terms of renderbuffer mapping/unmapping and format
> packing/unpacking functions.
> 
> The swrast and state tracker code for implementing accumulation are
> unused and will be removed in the next commit.
> 
> v2: don't use memcpy() in _mesa_clear_accum_buffer()
> ---
>  src/mesa/SConscript                      |    2 -
>  src/mesa/drivers/common/driverfuncs.c    |    3 +-
>  src/mesa/drivers/dri/intel/intel_pixel.c |    2 +-
>  src/mesa/main/accum.c                    |  364
>  +++++++++++++++++++++++++++++-
>  src/mesa/main/accum.h                    |   18 ++
>  src/mesa/sources.mak                     |    2 -
>  src/mesa/state_tracker/st_cb_clear.c     |    5 +-
>  src/mesa/state_tracker/st_cb_fbo.c       |   21 ++
>  src/mesa/state_tracker/st_context.c      |    5 +-
>  src/mesa/swrast/s_clear.c                |    5 +-
>  10 files changed, 412 insertions(+), 15 deletions(-)
> 
[...]
> +
> +   /* bounds, with scissor */
> +   x = ctx->DrawBuffer->_Xmin;
> +   y = ctx->DrawBuffer->_Ymin;
> +   width = ctx->DrawBuffer->_Xmax - ctx->DrawBuffer->_Xmin;
> +   height = ctx->DrawBuffer->_Ymax - ctx->DrawBuffer->_Ymin;
> +
> +   ctx->Driver.MapRenderbuffer(ctx, accRb, x, y, width, height,
> +                               GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
> +                               &accMap, &accRowStride);

I suppose that GL_MAP_READ_BIT could be skipped when scissor is not set, but the fact that nobody bothered with writing a hardware accelerated version of this till date means it doesn't really matter in practice.

> +   if (!accMap) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glAccum");
> +      return;
> +   }
> +
> +   if (accRb->Format == MESA_FORMAT_SIGNED_RGBA_16) {
> +      const GLshort clearR =
> FLOAT_TO_SHORT(ctx->Accum.ClearColor[0]);
> +      const GLshort clearG =
> FLOAT_TO_SHORT(ctx->Accum.ClearColor[1]);
> +      const GLshort clearB =
> FLOAT_TO_SHORT(ctx->Accum.ClearColor[2]);
> +      const GLshort clearA =
> FLOAT_TO_SHORT(ctx->Accum.ClearColor[3]);
> +      GLuint i, j;
> +
> +      for (j = 0; j < height; j++) {
> +         GLshort *row = (GLshort *) accMap;
> +
> +         for (i = 0; i < width; i++) {
> +            row[i * 4 + 0] = clearR;
> +            row[i * 4 + 1] = clearG;
> +            row[i * 4 + 2] = clearB;
> +            row[i * 4 + 3] = clearA;
> +         }
> +         accMap += accRowStride;
> +      }
> +   }
> +   else {
> +      /* other types someday? */
> +      _mesa_warning(ctx, "unexpected accum buffer type");
> +   }
> +
> +   ctx->Driver.UnmapRenderbuffer(ctx, accRb);
> +}
> +
> +
> +/**
> + * if (bias)
> + *    Accum += value
> + * else
> + *    Accum *= value
> + */
> +static void
> +accum_scale_or_bias(struct gl_context *ctx, GLfloat value,
> +                    GLint xpos, GLint ypos, GLint width, GLint
> height,
> +                    GLboolean bias)
> +{
> +   struct gl_renderbuffer *accRb =
> +      ctx->DrawBuffer->Attachment[BUFFER_ACCUM].Renderbuffer;
> +   GLubyte *accMap;
> +   GLint accRowStride;
> +
> +   assert(accRb);
> +
> +   ctx->Driver.MapRenderbuffer(ctx, accRb, xpos, ypos, width,
> height,
> +                               GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
> +                               &accMap, &accRowStride);
> +
> +   if (!accMap) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glAccum");
> +      return;
> +   }
> +
> +   if (accRb->Format == MESA_FORMAT_SIGNED_RGBA_16) {
> +      const GLshort incr = (GLshort) (value * 32767.0f);
> +      GLuint i, j;
> +      if (bias) {
> +         for (j = 0; j < height; j++) {
> +            GLshort *acc = (GLshort *) accMap;
> +            for (i = 0; i < 4 * width; i++) {
> +               acc[i] += incr;
> +            }
> +            accMap += accRowStride;
> +         }
> +      }
> +      else {
> +         /* scale */
> +         for (j = 0; j < height; j++) {
> +            GLshort *acc = (GLshort *) accMap;
> +            for (i = 0; i < 4 * width; i++) {
> +               acc[i] = (GLshort) (acc[i] * value);
> +            }
> +            accMap += accRowStride;
> +         }
> +      }
> +   }
> +   else {
> +      /* other types someday? */
> +   }
> +
> +   ctx->Driver.UnmapRenderbuffer(ctx, accRb);
> +}
> +
> +
> +/**
> + * if (load)
> + *    Accum = ColorBuf * value
> + * else
> + *    Accum += ColorBuf * value
> + */
> +static void
> +accum_or_load(struct gl_context *ctx, GLfloat value,
> +              GLint xpos, GLint ypos, GLint width, GLint height,
> +              GLboolean load)
> +{
> +   struct gl_renderbuffer *accRb =
> +      ctx->DrawBuffer->Attachment[BUFFER_ACCUM].Renderbuffer;
> +   struct gl_renderbuffer *colorRb =
> ctx->ReadBuffer->_ColorReadBuffer;
> +   GLubyte *accMap, *colorMap;
> +   GLint accRowStride, colorRowStride;
> +
> +   if (!colorRb) {
> +      /* no read buffer - OK */
> +      return;
> +   }
> +
> +   assert(accRb);
> +
> +   /* Map accum buffer */
> +   ctx->Driver.MapRenderbuffer(ctx, accRb, xpos, ypos, width,
> height,
> +                               GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
> +                               &accMap, &accRowStride);
> +   if (!accMap) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glAccum");
> +      return;
> +   }
> +
> +   /* Map color buffer */
> +   ctx->Driver.MapRenderbuffer(ctx, colorRb, xpos, ypos, width,
> height,
> +                               GL_MAP_READ_BIT,
> +                               &colorMap, &colorRowStride);
> +   if (!colorMap) {
> +      ctx->Driver.UnmapRenderbuffer(ctx, accRb);
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, "glAccum");
> +      return;
> +   }
> +
> +   if (accRb->Format == MESA_FORMAT_SIGNED_RGBA_16) {
> +      const GLfloat scale = value * 32767.0f;
> +      GLuint i, j;
> +
> +      for (j = 0; j < height; j++) {
> +         GLshort *acc = (GLshort *) accMap;
> +         GLfloat rgba[MAX_WIDTH][4];

We really should not allocate these huge arrays from the stack, as they will potentially cause stack overflow on Windows and other systems with smaller stack. Reportedly Windows processes have a 1MB stack by default, instead of 10MB of Linux.

By the current definition of MAX_WIDTH, this is a 256KB array. 64k on windows thanks to an hack precisely to mitigate this issue -- #define MAX_WIDTH 4K instead of 16K. Still there's no way to tell how much stack the app will leave to us, so we should be modest in our stack consumption for the sake of portability.

It's better to use malloc everywhere a MAX_WIDTH arrays is involved. At very least for new code.

The rest looks good to me.

Jose