[Piglit] [v8 09/13] tests/spec: EXT_image_dma_buf_import sample argb

[Pohjolainen, Topi]

On Wed, Jul 31, 2013 at 04:05:20PM -0700, Chad Versace wrote:
> >+static bool
> >+sample_and_destroy_img(unsigned w, unsigned h, EGLImageKHR img)
> >+{
> >+	GLuint prog, tex;
> >+
> >+	glGenTextures(1, &tex);
> >+	glBindTexture(GL_TEXTURE_EXTERNAL_OES, tex);
> >+
> >+	/* Set the image as level zero */
> >+	glEGLImageTargetTexture2DOES(GL_TEXTURE_EXTERNAL_OES,
> >+			(GLeglImageOES)img);
> >+	if (!piglit_check_egl_error(EGL_SUCCESS))
> >+		return false;
> 
> This should check the GL error, not the EGL error. Use piglit_check_gl_error().
> 
> Also, glEGLImageTargetTexture2DOES is allowed to fail if it does not support
> binding the given image as a texture, in which it emits GL_INVALID_OPERATION.
> See the GL_OES_EGL_image spec. Therefore, if glEGLImageTargetTexture2DOES fails
> here with GL_INVALID_OPERATION the test should skip rather than fail.

Good catch, I'll fix that.

> 
> 
> 
> >+static bool
> >+sample_buffer(void *buf, int fd, int fourcc, unsigned w, unsigned h,
> >+	unsigned stride, unsigned offset)
> >+{
> >+	EGLImageKHR img;
> >+	EGLint attr[] = {
> >+		EGL_WIDTH, w,
> >+		EGL_HEIGHT, h,
> >+		EGL_LINUX_DRM_FOURCC_EXT, fourcc,
> >+		EGL_DMA_BUF_PLANE0_FD_EXT, fd,
> >+		EGL_DMA_BUF_PLANE0_OFFSET_EXT, offset,
> >+		EGL_DMA_BUF_PLANE0_PITCH_EXT, stride,
> >+		EGL_NONE
> >+	};
> >+
> >+	img = eglCreateImageKHR(eglGetCurrentDisplay(), EGL_NO_CONTEXT,
> >+			EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)0, attr);
> 
> As in patch 8, if eglCreateImageKHR fails here with EGL_BAD_MATCH,
> then the test should skip not fail.

Will do.

> 
> >+
> >+	/**
> >+	 * Release the creator side of the buffer, EGL should have the
> >+	 * ownership now.
> >+	 */
> >+	piglit_destroy_dma_buf(buf);
> >+
> >+	/* Close the file descriptor also, EGL does not have ownership */
> >+	if (!piglit_check_egl_error(EGL_SUCCESS)) {
> 
> Move the above comment into the if-block. Otherwise the comment is confusing.

Makes sense also.

> 
> >+		close(fd);
> >+		return false;
> >+	}
> >+
> >+	if (!img) {
> >+		fprintf(stderr, "image creation succeed but returned NULL\n");
> >+		return false;
> >+	}
> >+
> >+	return sample_and_destroy_img(w, h, img);
> >+}
> 
> 
> 
> 
> >+enum piglit_result
> >+probe_rect_argb(unsigned w, unsigned h, const unsigned char *argb,
> >+		bool force_alpha_to_one)
> >+{
> >+	const unsigned cpp = 4;
> >+	unsigned i, j;
> >+
> >+	for (i = 0; i < h; ++i) {
> >+		for (j = 0; j < w; ++j) {
> >+			float expected[] = {
> >+				argb[i * w * cpp + j * cpp + 2] / 255.0f,
> >+				argb[i * w * cpp + j * cpp + 1] / 255.0f,
> >+				argb[i * w * cpp + j * cpp + 0] / 255.0f,
> >+				argb[i * w * cpp + j * cpp + 3] / 255.0f};
> >+
> >+			if (force_alpha_to_one)
> >+				expected[3] = 1.0f;
> >+
> >+			if (!piglit_probe_pixel_rgba(j, i, expected))
> >+				return PIGLIT_FAIL;
> >+		}
> >+	}
> >+
> >+	return PIGLIT_PASS;
> >+}
> 
> This function looks correct, but has unneeded overhead. The speed of Piglit runs
> does matter, especially when doing multiple runs back-to-back or when doing runs
> on simulated hardware. So, we do strive to keep Piglit running fast when it
> doesn't require too much effort on our part.
> 
> One of the slowest GL functions is glReadPixels, and this function calls glReadPixels
> for each pixel in the image. Instead, the function should be refactored so that it
> makes a single call to piglit_probe_rect_rgba, and hence a single call to glReadPixels.
> 

The framework utility 'piglit_probe_rect_rgba()' works for rectangles where all
pixels are of same color. Should I augment the framework to have another flavour
allowing each pixel to have different value (as is the case currently here) or
just simply switch to single color here in this test case also?