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

[Marek Olšák]

On Wed, Nov 9, 2016 at 8:10 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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.

No idea what's causing the failures, but releasing the FBO should be
really easy at the end of that block.

Marek