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

[Chad Versace]

On 08/08/2013 01:19 AM, Pohjolainen, Topi wrote:
> 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.

The GL/GLES fork of the probe functions is a really messy part of Piglit. Sigh...

Everything is so messy, that I don't know the best thing to do here. Whatever you
do, though, should not harm the accuracy of any existing tests. That eliminates
rewriting the GL version of piglit_probe_*rgba to use ubytes as the GLES version
does.

Though it would be nice to have Piglit's ugly mess of probe functions unified
between GL and GLES, I would first rather not stall this series on doing any
cleanup in that area.

I suggest adding a variant of piglit-util-gl.c:piglit_probe_image_color to
piglit-util-gles.c with the signature below.

int
piglit_probe_image_ubyte(int x, int y, int w, int h, GLenum format,
			 const float *image)