[Mesa-dev] [PATCH 3/3] swrast: map/unmap attachments, not renderbuffers in glReadPixels

[Eric Anholt]

On Sat,  5 Nov 2011 13:15:41 -0600, Brian Paul <brianp at vmware.com> wrote:
> Most drivers have been creating fake/wrapper renderbuffers when a
> texture image is attached to an FBO.  But that's not a requirement
> of core Mesa.
> 
> So an FBO attachment that points into a texture may not have a
> (fake) renderbuffer.  This caused the new glReadPixels code to fail
> when reading from a buffer that's actually a texture.
> 
> Now, we check the attachment point to see if it's a real renderbuffer
> or a texture image and call the corresponding MapRenderbuffer() or
> MapTextureImage() function.
> 
> This fixes a bunch of piglit fbo failures when using the non-DRI
> swrast driver.

I'm not excited about needing getters for the attachment's format -- I
wish we just had one structure with the Format in it we could reference
when talking about an attachment, since we need that field so often.

On the other hand, I like the idea of forcing using a helper function
for doing the mappings, instead of going straight to ctx->Driver.  With
all the hardware drivers needing to privately store x/y/w/h/mode/some
other pointer, it means that we can't recursively map a buffer (which
would be easy to accidentally do when doing packed depth/stencil or
CopyPixels).  I've been thinking it would be nice to make MTI/MRB
helpers that stored those right in the GL struct, and used those fields
to assert that someone testing on a software driver doesn't produce
recursive mappings.

> +
> +/**
> + * Map a framebuffer attachment.  An attachment may either be a renderbuffer
> + * or a texture image.  Check the attachment type and map the corresponding
> + * image.
> + * Note:  This might be moved out of swrast someday.
> + */
> +static void
> +map_attachment(struct gl_context *ctx,
> +               struct gl_renderbuffer_attachment *att,
> +               GLuint x, GLuint y, GLuint w, GLuint h,
> +               GLbitfield mode,
> +               GLubyte **mapOut, GLint *rowStrideOut)
> +{
> +   if (att->Type == GL_TEXTURE) {
> +      struct gl_texture_image *texImage = _mesa_get_attachment_teximage(att);
> +      assert(texImage);
> +      ctx->Driver.MapTextureImage(ctx, texImage,
> +                                  att->Zoffset,
> +                                  x, y, w, h, mode,
> +                                  mapOut, rowStrideOut);
> +   }
> +   else if (att->Type == GL_RENDERBUFFER) {
> +      assert(att->Renderbuffer);
> +      ctx->Driver.MapRenderbuffer(ctx, att->Renderbuffer,
> +                                  x, y, w, h, mode,
> +                                  mapOut, rowStrideOut);
> +   }
> +   else {
> +      *mapOut = NULL;
> +      *rowStrideOut = 0;
> +      _mesa_warning(NULL, "invalid attachment in map_attachment()");
> +   }
> +}
> +
> +/**
> + * Unmap a framebuffer attachment.
> + * Note:  This might be moved out of swrast someday.
> + */
> +static void
> +unmap_attachment(struct gl_context *ctx,
> +                 struct gl_renderbuffer_attachment *att)
> +{
> +   if (att->Type == GL_TEXTURE) {
> +      struct gl_texture_image *texImage = _mesa_get_attachment_teximage(att);
> +      assert(texImage);
> +      ctx->Driver.UnmapTextureImage(ctx, texImage, att->Zoffset);
> +   }
> +   else if (att->Type == GL_RENDERBUFFER) {
> +      assert(att->Renderbuffer);
> +      ctx->Driver.UnmapRenderbuffer(ctx, att->Renderbuffer);
> +   }
> +   else {
> +      _mesa_warning(NULL, "invalid attachment in unmap_attachment()");
> +   }
> +}

I'd like to see this moved to core up front -- if nothing else, I'm
going to need to use it elsewhere in swrast this week or so.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111106/765afe84/attachment-0001.pgp>