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

[Brian Paul]

On 12/06/2011 12:27 PM, Eric Anholt wrote:
> On Mon,  5 Dec 2011 20:41:25 -0700, Brian Paul<brianp at vmware.com>  wrote:
>> 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()
>> v3: don't allocate MAX_WIDTH arrays, be more careful with mapping flags
>
> Minor comments, then:
>
> Reviewed-by: Eric Anholt<eric at anholt.net>
>
>> ---
>>   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                    |  388 +++++++++++++++++++++++++++++-
>>   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, 436 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/SConscript b/src/mesa/SConscript
>> index 5c50189..633cb9c 100644
>> --- a/src/mesa/SConscript
>> +++ b/src/mesa/SConscript
>> @@ -153,7 +153,6 @@ math_sources = [
>>   swrast_sources = [
>>       'swrast/s_aaline.c',
>>       'swrast/s_aatriangle.c',
>> -    'swrast/s_accum.c',
>>       'swrast/s_alpha.c',
>>       'swrast/s_atifragshader.c',
>>       'swrast/s_bitmap.c',
>> @@ -241,7 +240,6 @@ statetracker_sources = [
>>       'state_tracker/st_atom_stipple.c',
>>       'state_tracker/st_atom_texture.c',
>>       'state_tracker/st_atom_viewport.c',
>> -    'state_tracker/st_cb_accum.c',
>>       'state_tracker/st_cb_bitmap.c',
>>       'state_tracker/st_cb_blit.c',
>>       'state_tracker/st_cb_bufferobjects.c',
>> diff --git a/src/mesa/drivers/common/driverfuncs.c b/src/mesa/drivers/common/driverfuncs.c
>> index 50fcede..0785cff 100644
>> --- a/src/mesa/drivers/common/driverfuncs.c
>> +++ b/src/mesa/drivers/common/driverfuncs.c
>> @@ -25,6 +25,7 @@
>>
>>   #include "main/glheader.h"
>>   #include "main/imports.h"
>> +#include "main/accum.h"
>>   #include "main/arrayobj.h"
>>   #include "main/context.h"
>>   #include "main/framebuffer.h"
>> @@ -80,7 +81,7 @@ _mesa_init_driver_functions(struct dd_function_table *driver)
>>
>>      /* framebuffer/image functions */
>>      driver->Clear = _swrast_Clear;
>> -   driver->Accum = _swrast_Accum;
>> +   driver->Accum = _mesa_accum;
>>      driver->RasterPos = _tnl_RasterPos;
>>      driver->DrawPixels = _swrast_DrawPixels;
>>      driver->ReadPixels = _mesa_readpixels;
>> diff --git a/src/mesa/drivers/dri/intel/intel_pixel.c b/src/mesa/drivers/dri/intel/intel_pixel.c
>> index ae2ac05..4f665a7 100644
>> --- a/src/mesa/drivers/dri/intel/intel_pixel.c
>> +++ b/src/mesa/drivers/dri/intel/intel_pixel.c
>> @@ -157,7 +157,7 @@ intel_check_blit_format(struct intel_region * region,
>>   void
>>   intelInitPixelFuncs(struct dd_function_table *functions)
>>   {
>> -   functions->Accum = _swrast_Accum;
>> +   functions->Accum = _mesa_accum;
>>      if (!getenv("INTEL_NO_BLIT")) {
>>         functions->Bitmap = intelBitmap;
>>         functions->CopyPixels = intelCopyPixels;
>> diff --git a/src/mesa/main/accum.c b/src/mesa/main/accum.c
>> index d7ed3a8..5fe8dc6 100644
>> --- a/src/mesa/main/accum.c
>> +++ b/src/mesa/main/accum.c
>> @@ -24,7 +24,10 @@
>>
>>   #include "glheader.h"
>>   #include "accum.h"
>> +#include "condrender.h"
>>   #include "context.h"
>> +#include "format_unpack.h"
>> +#include "format_pack.h"
>>   #include "imports.h"
>>   #include "macros.h"
>>   #include "mfeatures.h"
>> @@ -101,7 +104,7 @@ _mesa_Accum( GLenum op, GLfloat value )
>>         return;
>>
>>      if (ctx->RenderMode == GL_RENDER) {
>> -      ctx->Driver.Accum(ctx, op, value);
>> +      _mesa_accum(ctx, op, value);
>>      }
>
> If we're going to hard-wire this, we should just ditch the
> ctx->Driver.Accum hook, right?
>
> (I'm neutral on it -- it seems like something where metaops could be
> built, I just don't see anyone I know caring enough to ever do it).

A hardware implementation is certainly possible nowadays (ahem, ~20 
years after SGI did it in HW) and it would be kind of neat to see some 
of the old accumulation buffer motion-blur demos running at a 
reasonable speed, but I doubt anyone would bother implementing it.  We 
can remove the hook someday down the road, but it's a pretty small thing.


>> +         if (rgba)
>> +            free(rgba);
>> +         if (dest)
>> +            free(dest);
>
> free() happily takes NULL pointers.

Just being paranoid I guess, I'll fix that.

-Brian