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

[Pohjolainen, Topi]

On Fri, Aug 02, 2013 at 11:19:23AM -0700, Chad Versace wrote:
> >>>+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?
> 
> I'm sorry, you're right: piglit_probe_rect_rgba won't work here. The should use
> a a variant of piglit_probe that allows each pixel to have a different value.
> Two existing candidates are piglit_probe_image_color and piglit_compare_images_color.
> To use either function, you must first, as you're already doing in probe_rect_argb,
> convert the argb array to a float array, but I see no problem in doing that.

Well these are only available for desktop profile (piglit-util-gl.c). They are
absent in the GLES counterpart (piglit-util-gles.c). The interfaces are present
as they are found in the common header (piglit-util-gl-common.h).

If I introduced the equivalent for GLES profile also should I try to do some
refactoring in the desktop and common parts or just simply have parallel
implementation as is the case with the existing "piglit_probe_*()"-functions?
Only difference afterall is the data type written by 'glReadPixels()', GLES
simply divides the values by 255 to get the normalized floats gotten directly by
desktop.

If not then would it rather make sense to have different flavor for GLES where
one simply operates with unsigned bytes instead of floats? The conversion to
floats would really be unnecessary here as both the original data and the values
returned by 'glReadPixels()' are non-normalized unsigned.

I don't have a strong preference either way.

-Topi