[Piglit] [PATCH] Test that sRGB-related blits are allowed and do not do encoding/decoding.

[Brian Paul]

On 09/24/2012 05: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.
> ---
>   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 = \

You don't need the trailing backslash here and below.


> +	"#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";
> +
[...]


Looks OK to me, but I haven't really been following the srgb/blit 
discussion so someone else should also review.

-Brian