[Piglit] [PATCH] [v2] Add test case of render FBO with point coordinate.
Zhao, Jian J
jian.j.zhao at intel.com
Wed Feb 8 23:54:07 PST 2012
Hi Ian,
Thanks for your reply. My answer is inline. And I will send an update version soon.
> -----Original Message-----
> From: Ian Romanick [mailto:idr at freedesktop.org]
> Sent: Thursday, January 19, 2012 7:26 AM
> To: Zhao, Jian J
> Cc: piglit at lists.freedesktop.org
> Subject: Re: [Piglit] [PATCH] [v2] Add test case of render FBO with point
> coordinate.
>
> On 01/18/2012 03:24 AM, Jian Zhao wrote:
> > Add test case of rendering with point coordinate, it should work with
> > direct rendering, rendering with FBO.
> >
> > As the es spec 2.0.25 says:
> > "the gl-PointCoord fragment shader input defines a per-fragment
> > coordinate space (s; t) where s varies from 0 to 1 across the point
> > horizontally left-to-right, and t ranges from 0 to 1 across the point
> > vertically top-to-bottom."
> >
> > v2: To simplify the case, set the point size in the program instead of
> > the shader. (per Yuanhan's suggestion)
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=44613
> > Signed-off-by: Jian Zhao<jian.j.zhao at intel.com>
> > ---
> > tests/all.tests | 1 +
> > tests/bugs/CMakeLists.gl.txt | 1 +
> > tests/bugs/pointcoord.c | 115
> ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 117 insertions(+), 0 deletions(-)
> > create mode 100644 tests/bugs/pointcoord.c
> >
> > diff --git a/tests/all.tests b/tests/all.tests index 2e23634..a0babe2
> > 100644
> > --- a/tests/all.tests
> > +++ b/tests/all.tests
> > @@ -566,6 +566,7 @@ add_plain_test(bugs, 'fdo25614-genmipmap')
> > add_plain_test(bugs, 'fdo28551')
> > add_plain_test(bugs, 'fdo31934')
> > add_plain_test(bugs, 'point-sprite')
> > +add_plain_test(bugs, 'pointcoord')
>
> Put this in tests/fbo and call it fbo-gl_PointCoord.
OK.
>
> > add_plain_test(bugs, 'r300-readcache')
> > add_plain_test(bugs, 'tex1d-2dborder')
> > add_plain_test(bugs, 'tri-tex-crash') diff --git
> > a/tests/bugs/CMakeLists.gl.txt b/tests/bugs/CMakeLists.gl.txt index
> > 5c1864e..ed4b528 100644
> > --- a/tests/bugs/CMakeLists.gl.txt
> > +++ b/tests/bugs/CMakeLists.gl.txt
> > @@ -24,6 +24,7 @@ add_executable (r300-readcache r300-readcache.c)
> > add_executable (tex1d-2dborder tex1d-2dborder.c)
> > add_executable (fdo20701 fdo20701.c)
> > add_executable (point-sprite point-sprite.c)
> > +add_executable (pointcoord pointcoord.c)
> > add_executable (fdo22540 fdo22540.c)
> > add_executable (fdo23489 fdo23489.c)
> > add_executable (fdo23670-depth_test fdo23670-depth_test.c) diff
> > --git a/tests/bugs/pointcoord.c b/tests/bugs/pointcoord.c new file
> > mode 100644 index 0000000..6b03fb3
> > --- /dev/null
> > +++ b/tests/bugs/pointcoord.c
> > @@ -0,0 +1,115 @@
> > +/*
> > + * Copyright © 2012 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a
> > + * copy of this software and associated documentation files (the
> > +"Software"),
> > + * to deal in the Software without restriction, including without
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE,
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + *
> > + * Authors:
> > + * * Jian Zhao<jian.j.zhao at intel.com>
>
> This is unnecessary. git-log already captures authorship information.
>
> > + *
> > + */
> > +
> > +/**
> > + * \file pointcoord.c
> > + * Verify that applications can use point coordinate correctly,
> > + * especailly with FBO.
> > + */
> > +
> > +#include "piglit-util.h"
> > +
> > +int piglit_width = 100, piglit_height = 100; int piglit_window_mode =
> > +GLUT_RGB | GLUT_SINGLE;
> > +
> > +static const char vs_text[] =
> > + "#version 120 \n"
> > + "attribute vec4 vPosition;\n"
> > + "void main()\n"
> > + "{\n"
> > + "gl_Position = vPosition;\n"
> > + "}\n"
> > + ;
>
> Since your code never calls glBindAttribLocation or glGetAttribLocation, you
> should just use gl_Vertex.
OK. Just change to use gl_Vertex.
>
> > +
> > +static const char fs_text[] =
> > + "#version 120 \n"
> > + "void main() { gl_FragColor = vec4(gl_PointCoord.x, gl_PointCoord.y, 0.0,
> 1.0); }\n"
> > + ;
> > +static GLuint prog;
> > +
> > +static const float green[] = { 0.0, 1.0, 0.0, 1.0 }; static const
> > +float black[] = { 0.0, 0.0, 0.0, 1.0 };
> > +
> > +enum piglit_result
> > +piglit_display(void)
> > +{
> > + GLboolean pass = GL_TRUE;
> > + int PointSize = 64;
> > + GLuint fb, rb;
> > +
> > + piglit_require_extension("GL_ARB_point_sprite");
>
> This belongs in piglit_init. This test also uses GL_ARB_framebuffer_object,
> but it doesn't check for it.
Yes. I will add it and move it to piglit_init.
>
> > + glEnable(GL_POINT_SPRITE_ARB);
> > +
> > + glPointSize(PointSize);
> > +
> > + glGenRenderbuffers(1,&rb);
> > + glBindRenderbuffer(GL_RENDERBUFFER, rb);
> > + glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA, 100, 100);
> > +
> > + glGenFramebuffers(1,&fb);
> > + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fb);
> > + glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER,
> GL_COLOR_ATTACHMENT0,
> > + GL_RENDERBUFFER, rb);
>
> All of this FBO setup belongs in piglit_init.
OK.
>
> > +
> > + glClearColor(0.0, 0.0, 0.0, 1.0);
> > + glClear(GL_COLOR_BUFFER_BIT);
> > + glBegin(GL_POINTS);
> > + glVertex2f(0.0, 0.0);
> > + glEnd();
> > +
> > + glBindFramebuffer(GL_READ_FRAMEBUFFER, fb);
> > + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0);
> > + glBlitFramebuffer(0, 0, 100, 100, 0, 0, 100, 100,
> > +GL_COLOR_BUFFER_BIT, GL_NEAREST);
>
> Is this necessary? Is the failure present if probing directly from the FBO?
> Probing directly from the FBO would save a step and allow making this a
> concurrent test. That would make everyone happier. If that works,
> surround this block with 'if (!piglit_automatic)'.
Yes. I will add piglit_automatic here.
>
> > + glFlush();
>
> This is unnecessary.
>
> > +
> > + pass&= piglit_probe_pixel_rgb(0, 0, black);
> > + pass&= piglit_probe_pixel_rgb(18, 18, green);
> > + pass&= piglit_probe_pixel_rgb(18, 81, black);
>
> It is better to do
>
> pass = piglit_probe_pixel_rgb(...) && pass;
>
> because we want all of the pixels to be probed when there is a failure.
> C's evaluation rules will cause later calls to piglit_probe_pixel_rgb to be
> skipped once pass is false. Also, this should probe more than one pixel that is
> covered by the sprite.
Yes. Good point.
>
> > +
> > + assert(!glGetError());
>
> Use piglit_check_gl_error.
>
> > +
> > + return pass ? PIGLIT_PASS : PIGLIT_FAIL; }
> > +
> > +void
> > +piglit_init(int argc, char **argv)
> > +{
> > + GLuint vs, fs;
> > +
> > + piglit_require_vertex_shader();
> > + piglit_require_fragment_shader();
> > +
> > + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, fs_text);
> > + vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_text);
> > + prog = piglit_link_simple_program(vs, fs);
> > +
> > + piglit_LinkProgram(prog);
> > + piglit_link_check_status(prog);
>
> piglit_link_simple_program already does both of these things.
Thanks very much!
Best regards
Zhaojian
>
> > +
> > + piglit_UseProgram(prog);
> > +}
More information about the Piglit
mailing list