[Piglit] [PATCH v2] util: provide way to read a texture in ES compatible way

[Tapani Pälli]

On 21.05.2018 18:01, Brian Paul wrote:
> On 05/21/2018 05:53 AM, Tapani Pälli wrote:
>> Implementation supports GL_TEXTURE_2D, GL_TEXTURE_2D_ARRAY
>> and affects following functions:
>>
>>     - piglit_probe_texel_rect_rgba
>>     - piglit_probe_texel_volume_rgba
>>
>> v2: use read_texture only on GL ES
>>      fix indentation issues
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   tests/util/piglit-util-gl.c | 121 
>> +++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 115 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/util/piglit-util-gl.c b/tests/util/piglit-util-gl.c
>> index 2443be03e..271cca4a8 100644
>> --- a/tests/util/piglit-util-gl.c
>> +++ b/tests/util/piglit-util-gl.c
>> @@ -1882,6 +1882,99 @@ piglit_probe_image_ubyte(int x, int y, int w, 
>> int h, GLenum format,
>>       return 1;
>>   }
>> +static GLuint
>> +create_fbo_from_texture(GLenum target, GLint texture, GLint level, 
>> GLint layer)
>> +{
>> +    GLuint fbo;
>> +
>> +    glGenFramebuffers(1, &fbo);
>> +    glBindFramebuffer(GL_FRAMEBUFFER, fbo);
>> +
>> +    if (layer > 0) {
>> +        glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
>> +                      texture, level, layer);
>> +    } else {
>> +        glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
>> +                       target, texture, level);
>> +    }
>> +
>> +    assert(glCheckFramebufferStatus(GL_FRAMEBUFFER) ==
>> +           GL_FRAMEBUFFER_COMPLETE);
>> +    return fbo;
>> +}
>> +
>> +static GLenum
>> +binding_from_target(GLenum target)
>> +{
>> +    switch (target)    {
>> +    case GL_TEXTURE_2D:
>> +        return GL_TEXTURE_BINDING_2D;
>> +    case GL_TEXTURE_2D_ARRAY:
>> +        return GL_TEXTURE_BINDING_2D_ARRAY;
>> +    default:
>> +        fprintf(stderr, "%s: unsupported target 0x%x\n",
>> +            __func__, target);
>> +        return 0;
>> +    }
>> +}
>> +
>> +/**
>> + * Read texels in OpenGL ES compatible way.
>> + *
>> + * Currently bound texture is attached to a framebuffer object and
>> + * contents are read using glReadPixels. Supported targets are
>> + * GL_TEXTURE_2D, GL_TEXTURE_2D_ARRAY.
>> + */
>> +static GLfloat *
>> +read_texture(int target, int level, int x, int y, int layer, int w, 
>> int h)
> 
> Maybe read_texture_es() or read_texture_via_fbo() to be a bit less generic?

Yeah, read_texure_via_fbo() sounds fine to me.

> 
>> +{
>> +    GLint width, height, depth;
>> +    GLint current_read_fbo, current_draw_fbo, current_texture;
>> +    GLenum binding = binding_from_target(target);
>> +    unsigned char *buf;
>> +    GLfloat *buffer;
>> +    unsigned offset;
>> +
>> +    assert(binding != 0);
>> +
>> +    glGetIntegerv(binding, &current_texture);
>> +    glGetIntegerv(GL_READ_FRAMEBUFFER_BINDING, &current_read_fbo);
>> +    glGetIntegerv(GL_DRAW_FRAMEBUFFER_BINDING, &current_draw_fbo);
>> +
>> +    assert(target == GL_TEXTURE_2D || target == GL_TEXTURE_2D_ARRAY);
>> +
>> +    glGetTexLevelParameteriv(target, level, GL_TEXTURE_WIDTH, &width);
>> +    glGetTexLevelParameteriv(target, level, GL_TEXTURE_HEIGHT, &height);
>> +    glGetTexLevelParameteriv(target, level, GL_TEXTURE_DEPTH, &depth);
>> +
>> +    buffer = malloc(width * height * depth * 4 * sizeof(GLfloat));
>> +    buf = malloc(width * height * depth * 4 * sizeof(unsigned char));
> 
> Aren't we always returning a 2D image?  If so, why 'depth' here?

Function supports reading a single layer of 2D array (called from 
piglit_probe_texel_volume_rgba). This could be refactored a bit cleaner 
I guess but it fulfills the current requirements of the callers. One 
option would be to move buffer alloc to the callers and not have any 
offsets calculated here.

>> +
>> +    GLuint fbo =
>> +        create_fbo_from_texture(target, current_texture, level, layer);
>> +    assert(fbo != 0);
>> +
>> +    /* Offset to the layer we are expected to read. */
>> +    offset = layer * (width * height * 4);
> 
> If we're always returning a 2D image, I don't think we need the offset.
> 
> 
>> +
>> +    glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE,
>> +             buf + offset);
>> +
>> +    for (unsigned k = offset; k < offset + width * height * 4; k++)
>> +        buffer[k] = ((float) buf[k]) / 255.0;
> 
> Wondering about the offset here too.
> 
> At the very least, I think more comments are needed to explain what's 
> going on if I'm misunderstanding this.

Sure, I can add a bit more comments what is going on.


> Thanks.
> 
> -Brian
> 
>> +
>> +    free(buf);
>> +
>> +    glDeleteFramebuffers(1, &fbo);
>> +
>> +    /* Restore FBO state. */
>> +    glBindFramebuffer(GL_READ_FRAMEBUFFER, current_read_fbo);
>> +    glBindFramebuffer(GL_DRAW_FRAMEBUFFER, current_draw_fbo);
>> +
>> +    return buffer;
>> +}
>> +
>> +
>>   /**
>>    * Read a texel rectangle from the given location and compare its 
>> RGB value to
>>    * the given expected values.
>> @@ -1957,10 +2050,18 @@ int piglit_probe_texel_rect_rgba(int target, 
>> int level, int x, int y,
>>       glGetTexLevelParameteriv(target, level, GL_TEXTURE_WIDTH, &width);
>>       glGetTexLevelParameteriv(target, level, GL_TEXTURE_HEIGHT, 
>> &height);
>> -    buffer = malloc(width * height * 4 * sizeof(GLfloat));
>> -
>> -    glGetTexImage(target, level, GL_RGBA, GL_FLOAT, buffer);
>> +#ifndef PIGLIT_USE_OPENGL
>> +    if (target == GL_TEXTURE_2D) {
>> +        buffer = read_texture(target, level, x, y, 0, w, h);
>> +    } else {
>> +#else
>> +        buffer = malloc(width * height * 4 * sizeof(GLfloat));
>> +        glGetTexImage(target, level, GL_RGBA, GL_FLOAT, buffer);
>> +#endif
>> +#ifndef PIGLIT_USE_OPENGL
>> +    }
>> +#endif
>>       assert(x >= 0);
>>       assert(y >= 0);
>>       assert(x+w <= width);
>> @@ -2021,10 +2122,18 @@ int piglit_probe_texel_volume_rgba(int target, 
>> int level, int x, int y, int z,
>>       glGetTexLevelParameteriv(target, level, GL_TEXTURE_WIDTH, &width);
>>       glGetTexLevelParameteriv(target, level, GL_TEXTURE_HEIGHT, 
>> &height);
>>       glGetTexLevelParameteriv(target, level, GL_TEXTURE_DEPTH, &depth);
>> -    buffer = malloc(width * height * depth * 4 * sizeof(GLfloat));
>> -
>> -    glGetTexImage(target, level, GL_RGBA, GL_FLOAT, buffer);
>> +#ifndef PIGLIT_USE_OPENGL
>> +    if (target == GL_TEXTURE_2D_ARRAY) {
>> +        buffer = read_texture(target, level, x, y, z, w, h);
>> +    } else {
>> +#else
>> +        buffer = malloc(width * height * depth * 4 * sizeof(GLfloat));
>> +        glGetTexImage(target, level, GL_RGBA, GL_FLOAT, buffer);
>> +#endif
>> +#ifndef PIGLIT_USE_OPENGL
>> +    }
>> +#endif
>>       assert(x >= 0);
>>       assert(y >= 0);
>>       assert(d >= 0);
>>
>