[4/8] shader_runner: Add a non-relative, rgba variant of relative probe rect rgb.

Submitted by Eric Anholt on Aug. 29, 2014, 9:41 p.m.

Details

Message ID 1409348479-15398-4-git-send-email-eric@anholt.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Eric Anholt Aug. 29, 2014, 9:41 p.m.
---
 tests/shaders/shader_runner.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index 717be42..2c350cf 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -2221,6 +2221,19 @@  piglit_display(void)
 			if (!piglit_probe_pixel_rgb(x, y, &c[2])) {
 				pass = false;
 			}
+		} else if (sscanf(line, "probe rect rgba "
+				  "( %f , %f , %f , %f ) "
+				  "( %f , %f , %f , %f )",
+				  c + 0, c + 1, c + 2, c + 3,
+				  c + 4, c + 5, c + 6, c + 7) == 8) {
+			x = c[0];
+			y = c[1];
+			w = c[2];
+			h = c[3];
+
+			if (!piglit_probe_rect_rgba(x, y, w, h, &c[4])) {
+				pass = false;
+			}
 		} else if (sscanf(line, "relative probe rect rgb "
 				  "( %f , %f , %f , %f ) "
 				  "( %f , %f , %f )",

Comments

On Friday, August 29, 2014 02:41:15 PM Eric Anholt wrote:
> ---
>  tests/shaders/shader_runner.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 717be42..2c350cf 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -2221,6 +2221,19 @@ piglit_display(void)
>  			if (!piglit_probe_pixel_rgb(x, y, &c[2])) {
>  				pass = false;
>  			}
> +		} else if (sscanf(line, "probe rect rgba "
> +				  "( %f , %f , %f , %f ) "
> +				  "( %f , %f , %f , %f )",
> +				  c + 0, c + 1, c + 2, c + 3,
> +				  c + 4, c + 5, c + 6, c + 7) == 8) {
> +			x = c[0];
> +			y = c[1];
> +			w = c[2];
> +			h = c[3];
> +
> +			if (!piglit_probe_rect_rgba(x, y, w, h, &c[4])) {
> +				pass = false;
> +			}
>  		} else if (sscanf(line, "relative probe rect rgb "
>  				  "( %f , %f , %f , %f ) "
>  				  "( %f , %f , %f )",
> 

I don't think %f makes much sense here - you're reading floats,
converting them to ints, then passing them to a function expecting
ints.  For relative probes, it makes sense.

I think you should do:

		} else if (sscanf(line, "probe rect rgba "
				  "( %d , %d , %d , %d ) "
				  "( %f , %f , %f , %f )",
				  &x, &y, &w, &h,
				  c, c + 1, c + 2, c + 3) == 8) {

			if (!piglit_probe_rect_rgba(x, y, w, h, c)) {
				pass = false;
			}
		}

With that changed,
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

If you disagree, and think we really should be reading floats for some
reason, I won't argue too much either.  It looks like the rest of the
code does that for some reason.  Feel free to keep the R-b; this seems
like useful functionality and I'd rather not hold it up on trivialities.
Kenneth Graunke <kenneth@whitecape.org> writes:

> On Friday, August 29, 2014 02:41:15 PM Eric Anholt wrote:
>> ---
>>  tests/shaders/shader_runner.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
>> index 717be42..2c350cf 100644
>> --- a/tests/shaders/shader_runner.c
>> +++ b/tests/shaders/shader_runner.c
>> @@ -2221,6 +2221,19 @@ piglit_display(void)
>>  			if (!piglit_probe_pixel_rgb(x, y, &c[2])) {
>>  				pass = false;
>>  			}
>> +		} else if (sscanf(line, "probe rect rgba "
>> +				  "( %f , %f , %f , %f ) "
>> +				  "( %f , %f , %f , %f )",
>> +				  c + 0, c + 1, c + 2, c + 3,
>> +				  c + 4, c + 5, c + 6, c + 7) == 8) {
>> +			x = c[0];
>> +			y = c[1];
>> +			w = c[2];
>> +			h = c[3];
>> +
>> +			if (!piglit_probe_rect_rgba(x, y, w, h, &c[4])) {
>> +				pass = false;
>> +			}
>>  		} else if (sscanf(line, "relative probe rect rgb "
>>  				  "( %f , %f , %f , %f ) "
>>  				  "( %f , %f , %f )",
>> 
>
> I don't think %f makes much sense here - you're reading floats,
> converting them to ints, then passing them to a function expecting
> ints.  For relative probes, it makes sense.
>
> I think you should do:
>
> 		} else if (sscanf(line, "probe rect rgba "
> 				  "( %d , %d , %d , %d ) "
> 				  "( %f , %f , %f , %f )",
> 				  &x, &y, &w, &h,
> 				  c, c + 1, c + 2, c + 3) == 8) {
>
> 			if (!piglit_probe_rect_rgba(x, y, w, h, c)) {
> 				pass = false;
> 			}
> 		}
>
> With that changed,
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

Nope, that seems obviously good.  And it might have caught an earlier
bug in my code generation :)  Thanks!