<div class="gmail_quote">On 15 August 2012 16:47, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 08/15/2012 02:17 PM, Paul Berry wrote:<br>
> On 15 August 2012 12:51, Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a><br>
</div><div class="im">> <mailto:<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>>> wrote:<br>
><br>
>     Extend the framebuffer-blit-levels test to verify that<br>
>     glBlitFramebuffer works when blitting to and from miplevels other than<br>
>     zero for depth textures.<br>
><br>
</div>>     CC: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>><br>
<div class="im">>     ---<br>
>      .../framebuffer-blit-levels.c                      | 125 ++++++++++++++++-----<br>
>      1 file changed, 94 insertions(+), 31 deletions(-)<br>
><br>
>     diff --git a/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c<br>
>     b/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c<br>
>     index 31e0cf2..65bc497 100644<br>
>     --- a/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c<br>
>     +++ b/tests/spec/arb_framebuffer_object/framebuffer-blit-levels.c<br>
>     @@ -70,6 +70,14 @@ GLuint aux_framebuffer;<br>
>      GLuint test_texture;<br>
>      GLuint aux_texture;<br>
><br>
>     +GLenum texture_internal_format;<br>
>     +GLenum texture_format;<br>
>     +GLenum texture_type;<br>
><br>
><br>
> Is texture_type really necessary?  It looks like we always use GL_FLOAT.<br>
><br>
> By the same token, it looks like we always use the same values for<br>
> texture_internal_format and texture_format.<br>
><br>
> I won't push you on the subject, though, because I realize that this change<br>
> makes the code more self-explanatory :)<br>
<br>
</div>You're right. These variables aren't strictly necessary. But, if we ever extend<br>
the test for more formats, such as for GL_STENCIL_INDEX/GL_BYTE, then these<br>
variables will become useful.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
>     +/**<br>
>     + * Generate a block of test data where each pixel has a unique depth value in<br>
>     + * the range [0.0, 1.0).<br>
>     + */<br>
>     +static void<br>
>     +create_test_data_depth(GLfloat *data, unsigned level,<br>
>     +                      unsigned width, unsigned height)<br>
>     +{<br>
>     +       unsigned pixel;<br>
>     +       unsigned num_pixels = width * height;<br>
>     +       double depth_delta = 0.95 / ((SIZE >> level) * (SIZE >> level));<br>
><br>
><br>
> Consider changing this to the equivalent "double depth_delta = 0.95 / (width *<br>
> height);" just so that it's clearer how we know it won't overflow.<br>
<br>
</div>Will do.<br>
<div class="im"><br>
>      static void<br>
>      print_usage_and_exit(char *prog_name)<br>
>      {<br>
>             printf("Usage: %s <test_mode>\n"<br>
>                    "  where <test_mode> is one of:\n"<br>
>                    "    draw: test blitting *to* the given texture type\n"<br>
>     -              "    read: test blitting *from* the given texture type\n",<br>
>     +              "    read: test blitting *from* the given texture type\n"<br>
>     +              "  where <format> is one of:\n"<br>
>     +              "    rgba\n"<br>
>     +              "    depth\n",<br>
>                    prog_name);<br>
>             piglit_report_result(PIGLIT_FAIL);<br>
>      }<br>
><br>
><br>
> all.tests needs to be updated to send in the new command-line parameter.<br>
<br>
</div>Oops. Will do.<br>
<div class="im"><br>
> My comment about all.tests is the only crucial one.  With that fixed, this patch is:<br>
><br>
</div>> Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>><br>
</blockquote></div><br>