[Piglit] [PATCH 06/18] shader_runner: Refactor handling of fb commands.

[Francisco Jerez]

Marek Olšák <maraeo at gmail.com> writes:

> On Wed, Oct 19, 2016 at 1:36 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> This refactors the implementation of the various "fb" commands to be
>> part of a single 'if (parse_str(line, "fb ", ...)) {}' block in order
>> to make code-sharing easier among fb subcommands.  Will be more useful
>> when we start introducing additional fb subcommands.
>> ---
>>  tests/shaders/shader_runner.c | 67 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 40 insertions(+), 27 deletions(-)
>>
>> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
>> index ab2b907..4a2c807 100644
>> --- a/tests/shaders/shader_runner.c
>> +++ b/tests/shaders/shader_runner.c
>> @@ -140,7 +140,7 @@ static bool prog_in_use = false;
>>  static bool sso_in_use = false;
>>  static GLchar *prog_err_info = NULL;
>>  static GLuint vao = 0;
>> -static GLuint fbo = 0;
>> +static GLuint draw_fbo = 0;
>>  static GLint render_width, render_height;
>>
>>  static bool report_subtests = false;
>> @@ -2959,13 +2959,16 @@ piglit_display(void)
>>                         do_enable_disable(rest, true);
>>                 } else if (sscanf(line, "depthfunc %31s", s) == 1) {
>>                         glDepthFunc(piglit_get_gl_enum_from_name(s));
>> -               } else if (sscanf(line, "fb tex 2d %d", &tex) == 1) {
>> -                       GLenum status;
>> +               } else if (parse_str(line, "fb ", &rest)) {
>> +               GLuint fbo = 0;
>
> Wrong indentation. (it can lead to buggy code)
>

Yeah, the whole block is intentionally indented one tab to the left,
it's cleaned up in the next commit.

>>
>> -                       if (fbo == 0) {
>> -                               glGenFramebuffers(1, &fbo);
>> -                               glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>> -                       }
>> +               if (parse_str(rest, "tex 2d ", &rest)) {
>> +                       glGenFramebuffers(1, &fbo);
>> +                       glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>> +
>> +                       REQUIRE(parse_int(rest, &tex, &rest),
>> +                               "Framebuffer binding command not "
>> +                               "understood at: %s\n", rest);
>>
>>                         glFramebufferTexture2D(GL_FRAMEBUFFER,
>>                                                GL_COLOR_ATTACHMENT0,
>> @@ -2976,21 +2979,12 @@ piglit_display(void)
>>                                 piglit_report_result(PIGLIT_FAIL);
>>                         }
>>
>> -                       status = glCheckFramebufferStatus(GL_FRAMEBUFFER);
>> -                       if (status != GL_FRAMEBUFFER_COMPLETE) {
>> -                               fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);
>> -                               piglit_report_result(PIGLIT_FAIL);
>> -                       }
>> -
>> -                       render_width = get_texture_binding(tex)->width;
>> -                       render_height = get_texture_binding(tex)->height;
>> -               } else if (sscanf(line, "fb tex layered %d", &tex) == 1) {
>> -                       GLenum status;
>> +                       w = get_texture_binding(tex)->width;
>> +                       h = get_texture_binding(tex)->height;
>>
>> -                       if (fbo == 0) {
>> -                               glGenFramebuffers(1, &fbo);
>> -                               glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>> -                       }
>> +               } else if (sscanf(rest, "tex layered %d", &tex) == 1) {
>> +                       glGenFramebuffers(1, &fbo);
>> +                       glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>>
>>                         glFramebufferTexture(GL_FRAMEBUFFER,
>>                                              GL_COLOR_ATTACHMENT0,
>> @@ -3000,11 +2994,31 @@ piglit_display(void)
>>                                 piglit_report_result(PIGLIT_FAIL);
>>                         }
>>
>> -                       status = glCheckFramebufferStatus(GL_FRAMEBUFFER);
>> -                       if (status != GL_FRAMEBUFFER_COMPLETE) {
>> -                               fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);
>> -                               piglit_report_result(PIGLIT_FAIL);
>> -                       }
>> +                       w = get_texture_binding(tex)->width;
>> +                       h = get_texture_binding(tex)->height;
>> +
>> +               } else {
>> +                       fprintf(stderr, "Unknown fb bind subcommand "
>> +                               "\"%s\"\n", rest);
>> +                       piglit_report_result(PIGLIT_FAIL);
>> +               }
>> +
>> +               const GLenum status = glCheckFramebufferStatus(GL_FRAMEBUFFER);
>> +               if (status != GL_FRAMEBUFFER_COMPLETE) {
>> +                       fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);
>> +                       piglit_report_result(PIGLIT_FAIL);
>> +               }
>> +
>> +               render_width = w;
>> +               render_height = h;
>> +
>> +               /* Delete the previous draw FB in case
>> +                * it's no longer reachable.
>> +                */
>> +               if (draw_fbo != 0)
>> +                       glDeleteFramebuffers(1, &draw_fbo);
>> +
>> +               draw_fbo = fbo;
>>
>>                 } else if (parse_str(line, "frustum", &rest)) {
>>                         parse_floats(rest, c, 6, NULL);
>> @@ -3622,7 +3636,6 @@ piglit_init(int argc, char **argv)
>>                         sso_in_use = false;
>>                         prog_err_info = NULL;
>>                         vao = 0;
>> -                       fbo = 0;
>
> This breaks some piglit tests with --process-isolation 0. Adding
> "draw_fbo = 0" here fixes that. Is that the right fix?
>

Interesting, I guess the test breaks while trying to deallocate the FBO
From the previous test?  Or does it break in some other way?  In the
former case I guess that setting 'draw_fbo = 0' would work around the
issue, but it would probably also lead to FBO object leaks.

> Marek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20161109/a061e0e9/attachment.sig>