[Piglit] [v9 10/13] tests/spec: EXT_image_dma_buf_import sample argb

[Chad Versace]

On 08/09/2013 03:43 AM, Topi Pohjolainen wrote:
> Tests that one can sample the given buffer without other mipmap
> levels by setting texture filtering accordingly.
>
> v2:
>     - compile only on platforms that have drm (Eric)
>     - use standard drm definitions for fourcc instead of duplicated
>       local (Daniel, Eric)
>     - removed irrelevant quote of the spec (Eric)
>     - use helper variables for width, height and cpp instead
>       of repeating the magic numbers over and over again (Eric)
>     - used the same refactored common logic as the mipmap ARGB test
>
> v3 (Eric):
>     - use properly linked egl-extension calls
>
> v4 (Eric):
>     - add to 'all.tests'
>     - removed inclusion of standard EGL entrypoints as there is the
>       special dispatcher for the test cases to use
>
> v5 (Eric, Chad):
>     - sample using image-external extension
>     - fix inclusion in all.tests
>
> v6 (Chad):
>     - check for GL error instead of EGL when invoking
>       glEGLImageTargetTexture2DOES()
>     - allow the test to be skipped if external textures or the
>       chosen format are not supported
>     - fix the scope of the comment describing the need for closing
>       file descriptors in case image creation fails
>     - introduce format as command line argument and merge the XRGB
>       test here
>     - don't invoke glReadPixels() for each pixel separately
>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>   tests/all.tests                                    |   2 +
>   .../ext_image_dma_buf_import/CMakeLists.gles2.txt  |  23 +++
>   .../spec/ext_image_dma_buf_import/sample_common.c  | 164 +++++++++++++++++++++
>   .../spec/ext_image_dma_buf_import/sample_common.h  |  37 +++++
>   tests/spec/ext_image_dma_buf_import/sample_rgb.c   | 112 ++++++++++++++
>   5 files changed, 338 insertions(+)
>   create mode 100644 tests/spec/ext_image_dma_buf_import/CMakeLists.gles2.txt
>   create mode 100644 tests/spec/ext_image_dma_buf_import/sample_common.c
>   create mode 100644 tests/spec/ext_image_dma_buf_import/sample_common.h
>   create mode 100644 tests/spec/ext_image_dma_buf_import/sample_rgb.c



> +static enum piglit_result
> +sample_and_destroy_img(unsigned w, unsigned h, EGLImageKHR img)
> +{

...

> +	/* Set the image as level zero */
> +	glEGLImageTargetTexture2DOES(GL_TEXTURE_EXTERNAL_OES,
> +			(GLeglImageOES)img);
> +	error = glGetError();
> +
> +	/**
> +	 * EGL may not support binding of external textures, this is not an
> +	 * error.
> +	 */
> +	if (error == GL_INVALID_OPERATION)
> +		return PIGLIT_SKIP;
> +
> +	if (!piglit_check_gl_error(GL_NO_ERROR))
> +		return PIGLIT_FAIL;

The same problem with error codes occurs here as I described in patch 9. The error
in patch 9 involved eglGetError; patch 10 involves glGetError().

Just like eglGetError(), glGetError() resets the error code to GL_NO_ERROR. So,
the call to piglit_check_gl_error() here will always succeed.

To fix this, don't call piglit_check_gl_error(). Instead, just use the comparison
   if (error != GL_NO_ERROR)


> +static enum piglit_result
> +sample_buffer(void *buf, int fd, int fourcc, unsigned w, unsigned h,
> +	unsigned stride, unsigned offset)
> +{

...

> +	img = eglCreateImageKHR(eglGetCurrentDisplay(), EGL_NO_CONTEXT,
> +			EGL_LINUX_DMA_BUF_EXT, (EGLClientBuffer)0, attr);
> +
> +	/* Release the creator side of the buffer. */
> +	piglit_destroy_dma_buf(buf);
> +
> +	/* EGL may not support the format, this is not an error. */
> +	if (!img && piglit_check_egl_error(EGL_BAD_MATCH))
> +		return PIGLIT_SKIP;
> +
> +	if (!piglit_check_egl_error(EGL_SUCCESS)) {
> +		/* Close the descriptor also, EGL does not have ownership */
> +		close(fd);
> +		return PIGLIT_FAIL;
> +	}
> +
> +	if (!img) {
> +		fprintf(stderr, "image creation succeeded but returned NULL\n");
> +		return PIGLIT_FAIL;
> +	}

Same problem with eglGetError that I described in patch 9.


Other than the misuse of egl/glGetError, the patch looks good.