[Piglit] [PATCH] arb_fbo: Test glBlitFramebuffer on depth textures with nonzero miplevels

[Paul Berry]

On 15 August 2012 16:47, Chad Versace <chad.versace at linux.intel.com> wrote:

> On 08/15/2012 02:17 PM, Paul Berry wrote:
> > On 15 August 2012 12:51, Chad Versace <chad.versace at linux.intel.com
> > <mailto:chad.versace at linux.intel.com>> wrote:
> >
> >     Extend the framebuffer-blit-levels test to verify that
> >     glBlitFramebuffer works when blitting to and from miplevels other
> than
> >     zero for depth textures.
> >
> >     CC: Paul Berry <stereotype441 at gmail.com <mailto:
> stereotype441 at gmail.com>>
> >     ---
> >      .../framebuffer-blit-levels.c                      | 125
> ++++++++++++++++-----
> >      1 file changed, 94 insertions(+), 31 deletions(-)
> >
> >     diff --git
> a/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c
> >     b/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c
> >     index 31e0cf2..65bc497 100644
> >     --- a/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c
> >     +++ b/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c
> >     @@ -70,6 +70,14 @@ GLuint aux_framebuffer;
> >      GLuint test_texture;
> >      GLuint aux_texture;
> >
> >     +GLenum texture_internal_format;
> >     +GLenum texture_format;
> >     +GLenum texture_type;
> >
> >
> > Is texture_type really necessary?  It looks like we always use GL_FLOAT.
> >
> > By the same token, it looks like we always use the same values for
> > texture_internal_format and texture_format.
> >
> > I won't push you on the subject, though, because I realize that this
> change
> > makes the code more self-explanatory :)
>
> You're right. These variables aren't strictly necessary. But, if we ever
> extend
> the test for more formats, such as for GL_STENCIL_INDEX/GL_BYTE, then these
> variables will become useful.
>

Good point.  BTW, I have started investigating what it will take to fix
this test on Mesa/i965, and it looks like the code is going to have sharp
corners for stencil buffers.  So extending the test for
GL_STENCIL_INDEX/GL_BYTE is definitely worth doing.  In fact, I'll probably
do it in the next few days if you don't.


>
> >     +/**
> >     + * Generate a block of test data where each pixel has a unique
> depth value in
> >     + * the range [0.0, 1.0).
> >     + */
> >     +static void
> >     +create_test_data_depth(GLfloat *data, unsigned level,
> >     +                      unsigned width, unsigned height)
> >     +{
> >     +       unsigned pixel;
> >     +       unsigned num_pixels = width * height;
> >     +       double depth_delta = 0.95 / ((SIZE >> level) * (SIZE >>
> level));
> >
> >
> > Consider changing this to the equivalent "double depth_delta = 0.95 /
> (width *
> > height);" just so that it's clearer how we know it won't overflow.
>
> Will do.
>
> >      static void
> >      print_usage_and_exit(char *prog_name)
> >      {
> >             printf("Usage: %s <test_mode>\n"
> >                    "  where <test_mode> is one of:\n"
> >                    "    draw: test blitting *to* the given texture
> type\n"
> >     -              "    read: test blitting *from* the given texture
> type\n",
> >     +              "    read: test blitting *from* the given texture
> type\n"
> >     +              "  where <format> is one of:\n"
> >     +              "    rgba\n"
> >     +              "    depth\n",
> >                    prog_name);
> >             piglit_report_result(PIGLIT_FAIL);
> >      }
> >
> >
> > all.tests needs to be updated to send in the new command-line parameter.
>
> Oops. Will do.
>
> > My comment about all.tests is the only crucial one.  With that fixed,
> this patch is:
> >
> > Reviewed-by: Paul Berry <stereotype441 at gmail.com <mailto:
> stereotype441 at gmail.com>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20120815/79eaf6b7/attachment.html>