On 10 October 2012 10:03, Ian Romanick <i...@freedesktop.org> wrote:
> On 09/24/2012 04:35 PM, Paul Berry wrote:
>
>> The behaviour specified by OpenGL for blits involving sRGB is
>> self-contradictory--it is unclear whether blits should perform sRGB
>> encoding/decoding, and if so, whether this encoding/decoding should be
>> dependent upon the setting of the GL_FRAMEBUFFER_SRGB enable flag.
>> Experiments with nVidia and ATI drivers indicate that they never do
>> any sRGB encoding or decoding during a blit, regardless of the buffer
>> formats and regardless of the setting of GL_FRAMEBUFFER_SRGB.
>>
>> Furthermore, OpenGL specifies that multisampled blits between formats
>> that are not "identical" should fail, however nVidia and ATI drivers
>> allow these blits.
>>
>> Existing games, such as Valve's "Left For Dead 2", appear to rely on
>> the behaviour exhibited by the the nVidia and ATI drivers, so it seems
>> sensible for Piglit to test for this behaviour too.
>>
>
> The scaled blit check is a bit hinkey. The scaled and 1:1 blits produce
> the same results because the source image is, basically, vertical stripes.
> This opens the possibility for a buggy implementation that doesn't do the
> scaled blit... it just does the 1:1 blit.
>
You mean a buggy implementation that just ignored the second set of source
coordinates and just copied an image that was the same size as the
destination rectangle? I'd be a bit surprised if such a bug weren't
already caught by other piglit tests. But I don't mind improving this test
if we can keep it from getting too complicated. How about if we render
just one row of the source image when testing scaled blits (leaving the
rest of the source image black)? That would catch the bug you're concerned
about.
>
> I have a couple minor comments below...
>
>
> ---
>> tests/all.tests | 17 +
>> tests/spec/arb_framebuffer_**srgb/CMakeLists.gl.txt | 1 +
>> tests/spec/arb_framebuffer_**srgb/blit.c | 358
>> ++++++++++++++++++++++
>> 3 files changed, 376 insertions(+)
>> create mode 100644 tests/spec/arb_framebuffer_**srgb/blit.c
>>
>> diff --git a/tests/all.tests b/tests/all.tests
>> index 177a9bb..053857d 100644
>> --- a/tests/all.tests
>> +++ b/tests/all.tests
>> @@ -1172,6 +1172,23 @@ for format in ('rgba', 'depth'):
>> arb_framebuffer_object[test_**name] = PlainExecTest(test_name
>> + ' -auto')
>>
>>
>> +# Group ARB_framebuffer_sRGB
>> +arb_framebuffer_srgb = Group()
>> +spec['ARB_framebuffer_sRGB'] = arb_framebuffer_srgb
>> +for backing_type in ('texture', 'renderbuffer'):
>> + for srgb_types in ('linear', 'srgb', 'linear_to_srgb',
>> + 'srgb_to_linear'):
>> + for blit_type in ('single_sampled', 'upsample',
>> 'downsample',
>> + 'msaa', 'scaled'):
>> + for framebuffer_srgb_setting in ('enabled',
>> + 'disabled'):
>> + test_name = ' '.join(
>> + ['blit', backing_type,
>> srgb_types,
>> + blit_type,
>> framebuffer_srgb_setting])
>> + arb_framebuffer_srgb[test_**name] =
>> concurrent_test(
>> + 'arb_framebuffer_srgb-' +
>> test_name)
>> +
>> +
>> # Group ARB_sampler_objects
>> arb_sampler_objects = Group()
>> spec['ARB_sampler_objects'] = arb_sampler_objects
>> diff --git a/tests/spec/arb_framebuffer_**srgb/CMakeLists.gl.txt
>> b/tests/spec/arb_framebuffer_**srgb/CMakeLists.gl.txt
>> index f4ef120..5aa4b63 100644
>> --- a/tests/spec/arb_framebuffer_**srgb/CMakeLists.gl.txt
>> +++ b/tests/spec/arb_framebuffer_**srgb/CMakeLists.gl.txt
>> @@ -10,3 +10,4 @@ link_libraries (
>> )
>>
>> piglit_add_executable (arb_framebuffer_srgb-pushpop pushpop.c)
>> +piglit_add_executable (arb_framebuffer_srgb-blit blit.c)
>> diff --git a/tests/spec/arb_framebuffer_**srgb/blit.c
>> b/tests/spec/arb_framebuffer_**srgb/blit.c
>> new file mode 100644
>> index 0000000..bbd69b5
>> --- /dev/null
>> +++ b/tests/spec/arb_framebuffer_**srgb/blit.c
>> @@ -0,0 +1,358 @@
>> +/*
>> + * 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.
>> + */
>> +
>> +/** \file blit.c
>> + *
>> + * Test the sRGB behaviour of blits.
>> + *
>> + * The GL 4.3 spec is contradictory about how blits should be handled
>> + * when the source or destination buffer is sRGB. From section 18.3.1
>> + * Blitting Pixel Rectangles:
>> + *
>> + * (1) When values are taken from the read buffer, if the value of
>> + * FRAMEBUFFER_ATTACHMENT_COLOR_**ENCODING for the framebuffer
>> + * attachment corresponding to the read buffer is SRGB (see
>> + * section 9.2.3), the red, green, and blue components are
>> + * converted from the non-linear sRGB color space according to
>> + * equation 8.14.
>> + *
>> + * (2) When values are taken from the read buffer, no linearization is
>> + * performed even if the format of the buffer is SRGB.
>> + *
>> + * (3) When values are written to the draw buffers, blit operations
>> + * bypass most of the fragment pipeline. The only fragment
>> + * operations which affect a blit are the pixel ownership test,
>> + * the scissor test, and sRGB conversion (see section
>> + * 17.3.9). Color, depth, and stencil masks (see section 17.4.2)
>> + * are ignored.
>> + *
>> + * (4) If SAMPLE_BUFFERS for either the read framebuffer or draw
>> + * framebuffer is greater than zero, no copy is performed and an
>> + * INVALID_OPERATION error is generated if the dimensions of the
>> + * source and destination rectangles provided to BlitFramebuffer
>> + * are not identical, or if the formats of the read and draw
>> + * framebuffers are not identical.
>> + *
>> + * And from section 17.3.9 sRGB Conversion:
>> + *
>> + * (5) If FRAMEBUFFER_SRGB is enabled and the value of
>> + * FRAMEBUFFER_ATTACHMENT_COLOR_**ENCODING for the framebuffer
>> + * attachment corresponding to the destination buffer is SRGB1
>> + * (see section 9.2.3), the R, G, and B values after blending are
>> + * converted into the non-linear sRGB color space by computing
>> + * ... [formula follows] ... If FRAMEBUFFER_SRGB is disabled or
>> + * the value of FRAMEBUFFER_ATTACHMENT_COLOR_**ENCODING is not SRGB,
>> + * then ... [no conversion is applied].
>> + *
>> + * Paragraphs (1) and (2) seem irreconcilable: the first says that
>> + * linearization should happen when reading from SRGB buffers, the
>> + * second says that it shouldn't.
>> + *
>> + * The remaining paragraphs are self-consistent, however they aren't
>> + * consistent with the observed behaviour of existing drivers (notably
>> + * nVidia and ATI drivers). Existing drivers seem to follow the much
>> + * simpler rule that blits preserve the underlying binary
>> + * representation of the pixels, regardless of whether the format is
>> + * sRGB and regardless of the setting of FRAMEBUFFER_SRGB.
>> + * Furthermore, sRGB and non-sRGB formats are considered "identical"
>> + * for the purposes of paragraph (4). Existing games seem to rely on
>> + * this behaviour.
>> + *
>> + * This test verifies that blitting is permitted, and preserves the
>> + * underlying binary representation of the pixels, under any specified
>> + * combination of the following circumstances:
>> + *
>> + * - Using framebuffers backed by textures vs renderbuffers.
>> + * - Blitting from sRGB vs linear, and to sRGB vs linear.
>> + * - Doing a 1:1 blit from a single-sampled vs MSAA buffer, and to a
>> + * single-sampled vs MSAA buffer, or doing a scaled blit between
>> + * two single-sampled buffers.
>> + * - With FRAMEBUFFER_SRGB enabled vs disabled.
>> + *
>> + * The combination to test is selected using command-line parameters.
>> + *
>> + * The test operates by rendering an image to a source framebuffer
>> + * where each pixel's 8-bit color value is equal to its X coordinate.
>> + * Then it blits this image to a destination framebuffer, and checks
>> + * (using glReadPixels) that each pixel's 8-bit color value is still
>> + * equal to its X coordinate.
>> + *
>> + * Since glReadPixels cannot be used directly on MSAA buffers, an
>> + * additional resolve blit is added when necessary, to convert the
>> + * image to single-sampled before reading the pixel values.
>> + */
>> +
>> +#include "piglit-util-gl-common.h"
>> +
>> +const int PATTERN_WIDTH = 256;
>> +const int PATTERN_HEIGHT = 64;
>> +
>> +PIGLIT_GL_TEST_MAIN(PATTERN_**WIDTH, PATTERN_HEIGHT,
>> + GLUT_DOUBLE | GLUT_RGB | GLUT_ALPHA);
>> +
>> +/* Test parameters */
>> +static bool use_textures;
>> +static GLenum src_format;
>> +static GLenum dst_format;
>> +static GLsizei src_samples;
>> +static GLsizei dst_samples;
>> +static bool scaled_blit;
>> +static bool enable_srgb_framebuffer;
>> +
>> +/* GL objects */
>> +static GLuint src_fbo;
>> +static GLuint dst_fbo;
>> +static GLuint resolve_fbo;
>> +static GLint prog;
>> +
>> +static char *vs_text = \
>> + "#version 120\n"
>> + "void main()\n"
>> + "{\n"
>> + " gl_Position = gl_Vertex;\n"
>> + "}\n";
>> +
>> +static char *fs_text = \
>> + "#version 120\n"
>> + "void main()\n"
>> + "{\n"
>> + " float x = gl_FragCoord.x;\n"
>> + " gl_FragColor = vec4((x - 0.5) / 255.0);\n"
>> + "}\n";
>> +
>> +static GLuint
>> +setup_fbo(GLenum internalformat, GLsizei num_samples)
>> +{
>> + GLuint fbo;
>> + glGenFramebuffers(1, &fbo);
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, fbo);
>> + if (use_textures && num_samples == 0) {
>> + GLuint tex;
>> + const GLint level = 0;
>> + const GLint border = 0;
>>
>
> Why not just pass literals to the TexImage2D call?
I've been doing a lot of this sort of thing recently:
glTexImage2D(GL_TEXTURE_2D, 0 /* level */, ..., 0 /* border */, GL_RGBA,
...)
and I thought I would experiment with a different style of documenting the
literals. I'm not sure whether I like it or not.
Is this a blocking issue for you?
>
>
> + glGenTextures(1, &tex);
>> + glBindTexture(GL_TEXTURE_2D, tex);
>> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
>> + GL_NEAREST);
>> + glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
>> + GL_NEAREST);
>> + glTexImage2D(GL_TEXTURE_2D, level,
>> + internalformat, PATTERN_WIDTH,
>> PATTERN_HEIGHT,
>> + border, GL_RGBA, GL_BYTE, NULL);
>> + glFramebufferTexture2D(GL_**DRAW_FRAMEBUFFER,
>> + GL_COLOR_ATTACHMENT0,
>> + GL_TEXTURE_2D, tex, level);
>> + } else {
>> + GLuint rb;
>> + glGenRenderbuffers(1, &rb);
>> + glBindRenderbuffer(GL_**RENDERBUFFER, rb);
>> + glRenderbufferStorageMultisamp**le(GL_RENDERBUFFER,
>> num_samples,
>> + internalformat,
>> PATTERN_WIDTH,
>> + PATTERN_HEIGHT);
>> + glFramebufferRenderbuffer(GL_**DRAW_FRAMEBUFFER,
>> + GL_COLOR_ATTACHMENT0,
>> + GL_RENDERBUFFER, rb);
>> + }
>> + return fbo;
>> +}
>> +
>> +static void
>> +print_usage_and_exit(char *prog_name)
>> +{
>> + printf("Usage: %s <backing_type> <sRGB_types> <blit_type>\n"
>> + " <framebuffer_srgb_setting>\n"
>> + " where <backing_type> is one of:\n"
>> + " texture (ignored for multisampled framebuffers)\n"
>> + " renderbuffer\n"
>> + " where <sRGB_types> is one of:\n"
>> + " linear (both buffers linear)\n"
>> + " srgb (both buffers sRGB)\n"
>> + " linear_to_srgb\n"
>> + " srgb_to_linear\n"
>> + " where <blit_type> is one of:\n"
>> + " single_sampled\n"
>> + " upsample\n"
>> + " downsample\n"
>> + " msaa\n"
>> + " scaled\n"
>> + " where framebuffer_srgb_setting is one of:\n"
>> + " enabled\n"
>> + " disabled\n",
>> + prog_name);
>> + piglit_report_result(PIGLIT_**FAIL);
>> +}
>> +
>> +void
>> +piglit_init(int argc, char **argv)
>> +{
>> + GLint vs, fs;
>> +
>> + if (argc != 5) {
>> + print_usage_and_exit(argv[0]);
>> + }
>> +
>> + if (strcmp(argv[1], "texture") == 0) {
>> + use_textures = true;
>> + } else if (strcmp(argv[1], "renderbuffer") == 0) {
>> + use_textures = false;
>> + } else {
>> + print_usage_and_exit(argv[0]);
>> + }
>> +
>> + if (strcmp(argv[2], "linear") == 0) {
>> + src_format = GL_RGBA;
>> + dst_format = GL_RGBA;
>> + } else if (strcmp(argv[2], "srgb") == 0) {
>> + src_format = GL_SRGB8_ALPHA8;
>> + dst_format = GL_SRGB8_ALPHA8;
>> + } else if (strcmp(argv[2], "linear_to_srgb") == 0) {
>> + src_format = GL_RGBA;
>> + dst_format = GL_SRGB8_ALPHA8;
>> + } else if (strcmp(argv[2], "srgb_to_linear") == 0) {
>> + src_format = GL_SRGB8_ALPHA8;
>> + dst_format = GL_RGBA;
>> + } else {
>> + print_usage_and_exit(argv[0]);
>> + }
>> +
>> + if (strcmp(argv[3], "single_sampled") == 0) {
>> + src_samples = 0;
>> + dst_samples = 0;
>> + scaled_blit = false;
>> + } else if (strcmp(argv[3], "upsample") == 0) {
>> + src_samples = 0;
>> + dst_samples = 1; /* selects minimum available sample
>> count */
>> + scaled_blit = false;
>> + } else if (strcmp(argv[3], "downsample") == 0) {
>> + src_samples = 1;
>> + dst_samples = 0;
>> + scaled_blit = false;
>> + } else if (strcmp(argv[3], "msaa") == 0) {
>> + src_samples = 1;
>> + dst_samples = 1;
>> + scaled_blit = false;
>> + } else if (strcmp(argv[3], "scaled") == 0) {
>> + src_samples = 0;
>> + dst_samples = 0;
>> + scaled_blit = true;
>> + } else {
>> + print_usage_and_exit(argv[0]);
>> + }
>> +
>> + if (strcmp(argv[4], "enabled") == 0) {
>> + enable_srgb_framebuffer = true;
>> + } else if (strcmp(argv[4], "disabled") == 0) {
>> + enable_srgb_framebuffer = true;
>> + } else {
>> + print_usage_and_exit(argv[0]);
>> + }
>> +
>> + piglit_require_gl_version(21);
>> + piglit_require_extension("ARB_**framebuffer_object");
>> + piglit_require_extension("ARB_**framebuffer_sRGB");
>> +
>> + vs = piglit_compile_shader_text(GL_**VERTEX_SHADER, vs_text);
>> + fs = piglit_compile_shader_text(GL_**FRAGMENT_SHADER, fs_text);
>> + prog = piglit_link_simple_program(vs, fs);
>> + glDeleteShader(vs);
>> + glDeleteShader(fs);
>> +
>> + src_fbo = setup_fbo(src_format, src_samples);
>> + dst_fbo = setup_fbo(dst_format, dst_samples);
>> + if (dst_samples != 0)
>> + resolve_fbo = setup_fbo(dst_format, 0);
>> + else
>> + resolve_fbo = 0;
>> +}
>> +
>> +static bool
>> +analyze_image(GLuint fbo)
>> +{
>> + GLfloat *expected_data = malloc(PATTERN_WIDTH * PATTERN_HEIGHT *
>> 4 *
>> + sizeof(GLfloat));
>> + unsigned x, y, component;
>> + bool pass;
>> +
>> + for (y = 0; y < PATTERN_HEIGHT; ++y) {
>> + for (x = 0; x < PATTERN_WIDTH; ++x) {
>> + for (component = 0; component < 4; ++component) {
>> + expected_data[(y * PATTERN_WIDTH + x)
>> + * 4 + component] = x /
>> 255.0;
>> + }
>> + }
>> + }
>> +
>> + glBindFramebuffer(GL_READ_**FRAMEBUFFER, fbo);
>> + pass = piglit_probe_image_rgba(0, 0, PATTERN_WIDTH,
>> PATTERN_HEIGHT,
>> + expected_data);
>> + free(expected_data);
>> + return pass;
>> +}
>> +
>> +enum piglit_result
>> +piglit_display()
>> +{
>> + bool pass;
>> +
>> + glUseProgram(prog);
>> + glDisable(GL_FRAMEBUFFER_SRGB)**;
>> +
>> + /* Clear buffers */
>> + if (resolve_fbo != 0) {
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, resolve_fbo);
>> + glClear(GL_COLOR_BUFFER_BIT);
>> + }
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, dst_fbo);
>> + glClear(GL_COLOR_BUFFER_BIT);
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, src_fbo);
>> + glClear(GL_COLOR_BUFFER_BIT);
>> +
>> + /* Draw the source image */
>> + glViewport(0, 0, PATTERN_WIDTH, PATTERN_HEIGHT);
>> + piglit_draw_rect(-1, -1, 2, 2);
>> +
>> + /* Do the blit */
>> + glBindFramebuffer(GL_READ_**FRAMEBUFFER, src_fbo);
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, dst_fbo);
>> + if (enable_srgb_framebuffer)
>> + glEnable(GL_FRAMEBUFFER_SRGB);
>> + glBlitFramebuffer(0, 0, PATTERN_WIDTH,
>> + scaled_blit ? 1 : PATTERN_HEIGHT,
>> + 0, 0, PATTERN_WIDTH, PATTERN_HEIGHT,
>> + GL_COLOR_BUFFER_BIT, GL_NEAREST);
>> + glDisable(GL_FRAMEBUFFER_SRGB)**;
>> +
>> + /* If necessary, do a resolve blit */
>> + if (resolve_fbo != 0) {
>>
>
> This confused me at first because resolve (verb) is used as a noun. I
> expected resolve_fbo to be a bool. I completely understand why it's named
> this, and I have no suggestion to improve it. I'm just complaining. :(
>
>
> + glBindFramebuffer(GL_READ_**FRAMEBUFFER, dst_fbo);
>> + glBindFramebuffer(GL_DRAW_**FRAMEBUFFER, resolve_fbo);
>> + glBlitFramebuffer(0, 0, PATTERN_WIDTH, PATTERN_HEIGHT,
>> + 0, 0, PATTERN_WIDTH, PATTERN_HEIGHT,
>> + GL_COLOR_BUFFER_BIT, GL_NEAREST);
>> + pass = analyze_image(resolve_fbo);
>> + } else {
>> + pass = analyze_image(dst_fbo);
>> + }
>> +
>> + return pass ? PIGLIT_PASS : PIGLIT_FAIL;
>> +}
>>
>>
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
