[Piglit] [PATCH 1/5] msaa: Add files containing the shared code to test multiple draw buffers
Anuj Phogat
anuj.phogat at gmail.com
Thu Jul 12 11:52:36 PDT 2012
[snip]
>
> It seems like a lot of the code in this patch is very similar to code that
> already exists in sample-alpha-to-coverage.cpp and sample-alpha-to-one.cpp.
> How difficult would it be to adapt those existing tests to use the common
> code? (I think it would be ok to land this patch series first, and do that
> refactoring later).
>
I had this in mind. This should not take much effort to share this code among
sample-alpha-to-coverage.cpp and sample-alpha-to-one.cpp. I'll work on it after
pushing this series.
> This looks like it's going to be a good set of tests. I have a bunch of
> small nit-picks below¸ but in general I like it.
>
[snip]
>> + * Note: Testing for 3 three draw buffers is supported at present. We
>> have to
>>
>> + * generate the fragment shader at run time to allow drawing to user
>> specified
>> + * number of draw buffers. I'll skip it for now as this doesn't seem to
>> make
>> + * any difference to test with different number of draw buffers.
>
>
> I found this paragraph difficult to follow. You might consider rephrasing
> the first two sentences to something like this:
>
> Note: At present, the test always uses three draw buffers. To test other
> numbers of draw buffers, we would have to modify the fragment shader in
> nontrivial ways at run time.
>
[snip]
>> +static const char *frag_template =
>> + "#version 130\n"
>> + "#define OUT_TYPE %s\n"
>> + "out OUT_TYPE frag_out_0;\n"
>> + "out vec4 frag_out_1;\n"
>> + "out vec4 frag_out_2;\n"
>> + "uniform OUT_TYPE icolor;\n"
>
>
> It's kind of confusing to call this variable "icolor" because it's not
> always an integer variable. How about "frag0color" or something?
>
right.
[snip]
>> +shader_compile(void)
>> +{
>> + /* Compile program */
>> + GLint vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vert);
>> +
>> + const char *out_type_glsl = get_out_type_glsl();;
>> + unsigned frag_alloc_len = strlen(frag_template) +
>> + strlen(out_type_glsl) + 1;
>> + char *frag = (char *) malloc(frag_alloc_len);
>> + sprintf(frag, frag_template, out_type_glsl);
>> +
>> + GLint fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, frag);
>
>
> It doesn't really matter since this is a piglit test, but we should probably
> free(frag) here.
>
right
>> +
>> +void
>> +free_data_arrays(void)
>> +{
>> + if(color != NULL) {
>> + free(color);
>> + color = NULL;
>> + }
>> + if(coverage != NULL) {
>> + free(coverage);
>> + coverage = NULL;
>> + }
>> + if(expected != NULL) {
>> + free(expected);
>> + expected = NULL;
>> + }
>> +}
>
>
> FYI, it is safe to pass a NULL pointer to free() (it does nothing if you
> do), so this could be rewritten as:
>
> void
> free_data_arrays(void)
> {
> free(color);
> color = NULL;
> free(coverage);
> coverage = NULL;
> free(expected);
> expected = NULL;
> }
>
Yeah, I can skip these null pointer checks.
>>
>> +
>> +void
>> +float_color_to_int_color(int *dst, float *src)
>> +{
>> + if(!dst || !src) {
>> + printf("Null src/dst pointer\n");
>> + exit(1);
>> + }
>
>
> This check isn't really necessary IMHO. If src or dst is null, the loop
> below will segfault, and the programmer will have no less information to
> debug the problem. (In fact, if they have core dumps enabled, they will
> have even more information, since they will have a core dump).
>
I agree.
[snip]
>> +void
>> +draw_pattern(bool sample_alpha_to_coverage,
>> + bool sample_alpha_to_one,
>> + bool is_reference_image,
>> + float *float_color)
>> +{
>> + glUseProgram(prog);
>> + glClearColor(bg_color[0], bg_color[1],
>> + bg_color[2], bg_color[3]);
>> + glClear(buffer_to_test);
>> +
>> + if (sample_alpha_to_coverage) {
>> + if(!is_reference_image)
>> + glEnable (GL_SAMPLE_ALPHA_TO_COVERAGE);
>> + glUniform1i(alpha_to_coverage_loc, true);
>> + }
>> + else
>> + glUniform1i(alpha_to_coverage_loc, false);
>> +
>> + if (!is_reference_image && sample_alpha_to_one)
>> + glEnable (GL_SAMPLE_ALPHA_TO_ONE);
>
>
> I think this would read more easily if we split the code to set the uniform
> from the code to set up the GL state, e.g.:
>
> glUniform1i(alpha_to_coverage_loc, sample_alpha_to_coverage);
> if (!is_reference_image) {
> if (sample_alpha_to_coverage)
> glEnable(GL_SAMPLE_ALPHA_TO_COVERAGE);
> if (sample_alpha_to_one)
> glEnable(GL_SAMPLE_ALPHA_TO_ONE);
> }
>
Yeah, this looks cleaner.
>>
>> +
>> + unsigned indices[6] = {0, 1, 2, 0, 2, 3};
>> + int integer_color[num_rects * num_components];
>> +
>> + /* For integer color buffers convert the color data to integer
>> format */
>> + if(is_buffer_zero_integer_format) {
>> + float_color_to_int_color(integer_color, float_color);
>> + }
>> +
>> + for (int i = 0; i < num_rects; ++i) {
>> + float vertices[4][2] = {
>> + { 0.0f, 0.0f + i * (pattern_height / num_rects) },
>> + { 0.0f, (i + 1.0f) * (pattern_height / num_rects) },
>> + { pattern_width, (i + 1.0f) * (pattern_height / num_rects)
>> },
>> + { pattern_width, 0.0f + i * (pattern_height / num_rects) }
>> };
>> +
>> + glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE,
>> + sizeof(vertices[0]),
>> + (void *) vertices);
>> +
>> + glUniform4fv(color_loc, 1, (float_color + i *
>> num_components));
>> + if(is_buffer_zero_integer_format) {
>> + glUniform4iv(icolor_loc, 1,
>> + integer_color + i * num_components);
>> + }
>> + else {
>> + glUniform4fv(icolor_loc, 1,
>> + (float_color + i * num_components));
>> + }
>> + glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT,
>> + (void *) indices);
>> + }
>> +
>> + if (!is_reference_image) {
>> + if (sample_alpha_to_coverage)
>> + glDisable (GL_SAMPLE_ALPHA_TO_COVERAGE);
>> +
>> + if (sample_alpha_to_one)
>> + glDisable (GL_SAMPLE_ALPHA_TO_ONE);
>> + }
>
>
> Personally I would just replace this with
>
> glDisable(GL_SAMPLE_ALPHA_TO_COVERAGE);
> glDisable(GL_SAMPLE_ALPHA_TO_ONE);
>
> There's no harm, and it makes it manifestly obvious that those bits will be
> disabled when the function returns.
>
I just tried to avoid unnecessary state changes. But I agree these checks
have no added advantage in this test case.
>>
>> +}
>> +
>> +void
>> +compute_expected(bool sample_alpha_to_coverage,
>> + bool sample_alpha_to_one,
>> + int draw_buffer_count)
>> +{
>> + int i, j;
>> + unsigned buffer_idx_offset = draw_buffer_count *
>> + num_rects *
>> + num_components;
>> +
>> + /* Compute the coverage value used in the test */
>> + if (num_samples &&
>> + sample_alpha_to_coverage &&
>> + !is_buffer_zero_integer_format) {
>> +
>> + for (i = 0; i < num_rects; i++) {
>> + /* Coverege value for all the draw buffers comes
>> from
>
>
> "coverage"
>
>>
>> + * the fragment alpha values of draw buffer zero
>> + */
>> + float frag_alpha = color[i * num_components + 3];
>> + coverage[i] = (frag_alpha < 1.0) ? frag_alpha :
>> 1.0;
>> + }
>> + }
>> + else {
>> + for (i = 0; i < num_rects; i++)
>> + coverage[i] = 1.0;
>> + }
>> +
>> + /* Coverage value decides the number of samples in multisample
>> buffer
>> + * covered by an incoming fragment, which will then receive the
>> + * fragment data. When the multisample buffer is resolved it gets
>> + * blended with the background color which is written to the
>> remaining
>> + * samples.
>> + * Page 254 (page 270 of the PDF) of the OpenGL 3.0 spec says:
>> + * "The method of combination is not specified, though a simple
>> average
>> + * computed independently for each color component is
>> recommended."
>> + * This is followed by NVIDIA and AMD in their proprietary
>> drivers.
>> + */
>> + for (i = 0; i < num_rects; i++) {
>> +
>> + float samples_used = coverage[i] * num_samples;
>> + /* Exepected color values are computed only for integer
>
>
> "expected"
>
>>
[snip]
>> +/**
>> + * Alpha values are visualized by blending the image with a grayscale
>> + * checkerboard.
>> + */
>> +void
>> +visualize_image(float *img, bool lhs, int draw_buffer_count)
>
>
> This function looks almost exactly like the visualize_image() function in
> formats.cpp. Can we share this code?
>
Yes, I picked it from formats.cpp and made few changes specific to these
test cases. I can move the function to common.cpp and define few variables
for these changes.
>>
>> +{
>> + float *visualization = (float *) malloc(sizeof(float) * 3 *
>> + pattern_width * pattern_height);
>> + for (int y = 0; y < pattern_height; ++y) {
>> + for (int x = 0; x < pattern_width; ++x) {
>> + float r = 0, g = 0, b = 0, a = 1;
>> + float *pixel =
>> + &img[(y * pattern_width + x) *
>> num_components];
>> + r = pixel[0];
>> + g = pixel[1];
>> + b = pixel[2];
>> + a = pixel[3];
>> +
>> + float checker = ((x ^ y) & 0x10) ? 0.75 : 0.25;
>> + r = r * a + checker * (1 - a);
>> + g = g * a + checker * (1 - a);
>> + b = b * a + checker * (1 - a);
>> +
>> + visualization[(y * pattern_width + x) * 3] = r;
>> + visualization[(y * pattern_width + x) * 3 + 1] =
>> g;
>> + visualization[(y * pattern_width + x) * 3 + 2] =
>> b;
>> + }
>> + }
>> + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0);
>> + glViewport(0, 0, pattern_width, pattern_height);
>> + glUseProgram(0);
>> + glWindowPos2f(lhs ? 0 : pattern_width,
>> + draw_buffer_count * pattern_height);
>> + glDrawPixels(pattern_width, pattern_height, GL_RGB, GL_FLOAT,
>> + visualization);
>> + free(visualization);
>> + free(img);
>
>
> It seems weird for this function to free the input image as a side effect.
> I'd recommend doing it in the caller.
>
right
[snip]
>> +void
>> +ms_fbo_and_draw_buffers_setup(int samples,
>> + int width,
>> + int height,
>> + int n_attachments,
>> + GLenum buffer_zero_format)
>> +{
>> + int maxBuffers;
>> + glGetIntegerv(GL_MAX_COLOR_ATTACHMENTS, &maxBuffers);
>> +
>> + /* Ensure that requested number of color attachments are
>> + * supported by the implementation and fragment shader.
>> + */
>> + if (n_attachments <= (int) ARRAY_SIZE(draw_buffers) &&
>> + n_attachments <= maxBuffers)
>> + num_draw_buffers = n_attachments;
>> + else
>> + printf("Number of attachments requested are not
>> supported\n");
>
>
> In the else block, we should probably add
> "piglit_report_result(PIGLIT_SKIP);" so that the test just skips on
> implementations that don't support enough attachment points.
>
right
>>
>> +
>> + pattern_width = width;
>> + pattern_height = height;
>> +
>> + /* Setup frame buffer objects with required configuration */
>> + FboConfig ms_config(samples, pattern_width, pattern_height);
>> + ms_config.color_internalformat = buffer_zero_format;
>> + ms_fbo.setup(ms_config);
>> +
>> + /* Create resolve_fbo with dimensions large enough to accomodate
>> + * all the draw buffers
>> + */
>> + FboConfig resolve_config(0, pattern_width,
>> + num_draw_buffers * pattern_height);
>> + resolve_config.color_internalformat = GL_RGBA;
>> + resolve_fbo.setup(resolve_config);
>> +
>> + /* Create resolve_int_fbo to store downsampled integer draw buffer
>> */
>> + if (buffer_zero_format == GL_RGBA8I) {
>> + resolve_config.color_internalformat = GL_RGBA8I;
>> + /* Assuming single integer buffer */
>> + resolve_config.height = pattern_height;
>> + resolve_int_fbo.setup(resolve_config);
>> + is_buffer_zero_integer_format = true;
>> + }
>> + else if (buffer_zero_format != GL_RGBA){
>> + printf("Draw buffer zero format is not"
>> + " supported by test functions.\n");
>> + piglit_report_result(PIGLIT_SKIP);
>
>
> This should only happen if there is a bug in the piglit test, right?
> (because the caller passed an unexpected value to
> ms_fbo_and_draw_buffers_setup()) In that case, this should be PIGLIT_FAIL
> so that we notice the problem and fix it. PIGLIT_SKIP should just be for
> situations where the test is inapplicable because the underlying GL
> implementation is missing a required extension (or something similar).
>
right
Thanks
Anuj
More information about the Piglit
mailing list