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

[Jose Fonseca]

Thanks for the review.

----- Original Message -----
> 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.

Yes, the update_framebuffer_size finds the min size, which I think is obvious.  This comment here was supposed to explain why we do it when gl_framebuffer has similar info, ie., the less obvious bit.

But I agree that the comment could be better phrased.  What about this?

   "We need to derive pipe_framebuffer size from the bound pipe_surfaces here instead of copying gl_framebuffer size because for certain target types (like PIPE_TEXTURE_1D_ARRAY) gl_framebuffer::Height has the number of layers instead of 1."

Jose



> > +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>
>