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

[Brian Paul]

On 04/23/2014 09:17 AM, Jose Fonseca wrote:
> 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."

That sounds great.

-Brian


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