[Mesa-dev] [PATCH 2/2] mesa/st: Fix pipe_framebuffer_state::height for PIPE_TEXTURE_1D_ARRAY.

[Brian Paul]

On 04/23/2014 07:55 AM, jfonseca at vmware.com wrote:
> From: José Fonseca <jfonseca at vmware.com>
>
> This prevents buffer overflow w/ llvmpipe when running piglit
>
>    bin/gl-3.2-layered-rendering-clear-color-all-types 1d_array single_level -fbo -auto
>
> v2: Compute the framebuffer size as the minimum size, as pointed out by
> Brian;  compacted code;  ran piglit quick test list (with no
> regressions.)
> ---
>   src/mesa/state_tracker/st_atom_framebuffer.c | 33 ++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_atom_framebuffer.c b/src/mesa/state_tracker/st_atom_framebuffer.c
> index 4c4f839..f395ec7 100644
> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
> @@ -31,6 +31,8 @@
>     *   Brian Paul
>     */
>
> +#include <limits.h>
> +
>   #include "st_context.h"
>   #include "st_atom.h"
>   #include "st_cb_bitmap.h"
> @@ -44,6 +46,24 @@
>
>
>   /**
> + * Update framebuffer size.
> + *
> + * framebuffer->width should match fb->Weight, but for PIPE_TEXTURE_1D_ARRAY

"fb->Width"


> + * textures fb->Height has the number of layers, and not the surface height.
> + */

The comment seems a bit disconnected from the code. 
update_framebuffer_size() is used to find the size which is the min of 
the attached surfaces.  The comment about 1D array textures doesn't seem 
to matter in the code.  That just seems a little confusing.


> +static void
> +update_framebuffer_size(struct pipe_framebuffer_state *framebuffer,
> +                        struct pipe_surface *surface)
> +{
> +   assert(surface);
> +   assert(surface->width  < UINT_MAX);
> +   assert(surface->height < UINT_MAX);
> +   framebuffer->width  = MIN2(framebuffer->width,  surface->width);
> +   framebuffer->height = MIN2(framebuffer->height, surface->height);
> +}
> +
> +
> +/**
>    * Update framebuffer state (color, depth, stencil, etc. buffers)
>    */
>   static void
> @@ -57,11 +77,12 @@ update_framebuffer_state( struct st_context *st )
>      st_flush_bitmap_cache(st);
>
>      st->state.fb_orientation = st_fb_orientation(fb);
> -   framebuffer->width = fb->Width;
> -   framebuffer->height = fb->Height;
>
>      /*printf("------ fb size %d x %d\n", fb->Width, fb->Height);*/
>
> +   framebuffer->width  = UINT_MAX;
> +   framebuffer->height = UINT_MAX;
> +
>      /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
>       * to determine which surfaces to draw to
>       */
> @@ -81,6 +102,7 @@ update_framebuffer_state( struct st_context *st )
>
>            if (strb->surface) {
>               pipe_surface_reference(&framebuffer->cbufs[i], strb->surface);
> +            update_framebuffer_size(framebuffer, strb->surface);
>            }
>            strb->defined = GL_TRUE; /* we'll be drawing something */
>         }
> @@ -100,12 +122,14 @@ update_framebuffer_state( struct st_context *st )
>            st_update_renderbuffer_surface(st, strb);
>         }
>         pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +      update_framebuffer_size(framebuffer, strb->surface);
>      }
>      else {
>         strb = st_renderbuffer(fb->Attachment[BUFFER_STENCIL].Renderbuffer);
>         if (strb) {
>            assert(strb->surface);
>            pipe_surface_reference(&framebuffer->zsbuf, strb->surface);
> +         update_framebuffer_size(framebuffer, strb->surface);
>         }
>         else
>            pipe_surface_reference(&framebuffer->zsbuf, NULL);
> @@ -122,6 +146,11 @@ update_framebuffer_state( struct st_context *st )
>      }
>   #endif
>
> +   /* _mesa_test_framebuffer_completeness refuses framebuffers with no
> +    * attachments, so this should never happen. */

Close */ on next line.


> +   assert(framebuffer->width  != UINT_MAX);
> +   assert(framebuffer->height != UINT_MAX);
> +
>      cso_set_framebuffer(st->cso_context, framebuffer);
>   }
>
>

Otherwise, Reviewed-by: Brian Paul <brianp at vmware.com>