[PATCH 0/5] vsp-tests: Implement RPF cropping tests

[PATCH 0/5] vsp-tests: Implement RPF cropping tests

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Update the gen-image utility such that it can crop the image at input,
and provide tests to ensure that the output of the RPF is as we expect.

The first patch in the series, reformats the output filenames such that
frames, and the correct reference frame are in the correct sort order in
the filesystem. This helps with comparing the output in case of errors

Kieran Bingham (5):
  vsp-lib: sort output frames correctly
  vsp-lib: Filter non-filesystem regular characters
  gen-image: Implement option to parse an input crop
  vsp-lib: Support RPF frame cropping
  tests: Add RPF cropping test

 scripts/vsp-lib.sh          |  44 ++++++++++++++-
 src/gen-image.c             | 106 +++++++++++++++++++++++++++++++++++++-
 tests/vsp-unit-test-0021.sh |  39 ++++++++++++++-
 3 files changed, 187 insertions(+), 2 deletions(-)
 create mode 100755 tests/vsp-unit-test-0021.sh

base-commit: 067d210716b282f7e5ecede5a33c7f7fdabd1358
-- 
git-series 0.9.1

[PATCH 1/5] vsp-lib: sort output frames correctly

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

In the event of failed frames, or VSP_KEEP_FRAMES being set, the output
file names do not sort such that the reference frame is next to the
failed frame.

This can make comparing reference frames and the relevant output frames
tedious and difficult.

Re-arrange the output filenames such that the sort order will match the
option parameters correctly, followed by either the reference frame
identifier, or the frame number at the end of the filename string

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 scripts/vsp-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
index 1a356ec02374..5aff30217a27 100755
--- a/scripts/vsp-lib.sh
+++ b/scripts/vsp-lib.sh
@@ -278,12 +278,12 @@ compare_frames() {
 		}
 
 		if [ $match = "false" -o x$VSP_KEEP_FRAMES = x1 ] ; then
-			mv $frame ${0/.sh/}-$(basename ${frame/.bin/-$params.bin})
+			mv $frame ${0/.sh/}-$params-$(basename ${frame})
 		fi
 	done
 
 	if [ x$VSP_KEEP_FRAMES = x1 -o $result = "fail" ] ; then
-		mv ${frames_dir}ref-frame.bin ${0/.sh/}-ref-frame-$params.bin
+		mv ${frames_dir}ref-frame.bin ${0/.sh/}-$params-ref-frame.bin
 	else
 		rm -f ${frames_dir}ref-frame.bin
 		rm -f ${frames_dir}frame-*.bin
-- 
git-series 0.9.1

Re: [PATCH 1/5] vsp-lib: sort output frames correctly

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:03:56 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> In the event of failed frames, or VSP_KEEP_FRAMES being set, the output
> file names do not sort such that the reference frame is next to the
> failed frame.
> 
> This can make comparing reference frames and the relevant output frames
> tedious and difficult.
> 
> Re-arrange the output filenames such that the sort order will match the
> option parameters correctly, followed by either the reference frame
> identifier, or the frame number at the end of the filename string
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and pushed.

> ---
>  scripts/vsp-lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
> index 1a356ec02374..5aff30217a27 100755
> --- a/scripts/vsp-lib.sh
> +++ b/scripts/vsp-lib.sh
> @@ -278,12 +278,12 @@ compare_frames() {
>  		}
> 
>  		if [ $match = "false" -o x$VSP_KEEP_FRAMES = x1 ] ; then
> -			mv $frame ${0/.sh/}-$(basename 
${frame/.bin/-$params.bin})
> +			mv $frame ${0/.sh/}-$params-$(basename ${frame})
>  		fi
>  	done
> 
>  	if [ x$VSP_KEEP_FRAMES = x1 -o $result = "fail" ] ; then
> -		mv ${frames_dir}ref-frame.bin ${0/.sh/}-ref-frame-$params.bin
> +		mv ${frames_dir}ref-frame.bin ${0/.sh/}-$params-ref-frame.bin
>  	else
>  		rm -f ${frames_dir}ref-frame.bin
>  		rm -f ${frames_dir}frame-*.bin

-- 
Regards,

Laurent Pinchart

[PATCH 2/5] vsp-lib: Filter non-filesystem regular characters

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Parameters can contain characters not suited to use in filenames.

Add '=','(', and ')' to the filtering, and replace with '_'

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 scripts/vsp-lib.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
index 5aff30217a27..08bf8f36c582 100755
--- a/scripts/vsp-lib.sh
+++ b/scripts/vsp-lib.sh
@@ -263,6 +263,9 @@ compare_frames() {
 	local params=${args// /-}
 	params=${params:+-$params}
 	params=${params//\//_}
+	params=${params/=/_}
+	params=${params/(/_}
+	params=${params/)/_}
 	params=$in_fmt-$out_fmt-$size$params
 
 	if [ x$__vsp_pixel_perfect != xtrue ] ; then
-- 
git-series 0.9.1

Re: [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:03:57 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Parameters can contain characters not suited to use in filenames.
> 
> Add '=','(', and ')' to the filtering, and replace with '_'

What's the issue with those characters ? :-)

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  scripts/vsp-lib.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
> index 5aff30217a27..08bf8f36c582 100755
> --- a/scripts/vsp-lib.sh
> +++ b/scripts/vsp-lib.sh
> @@ -263,6 +263,9 @@ compare_frames() {
>  	local params=${args// /-}
>  	params=${params:+-$params}
>  	params=${params//\//_}
> +	params=${params/=/_}
> +	params=${params/(/_}
> +	params=${params/)/_}

According to the bash manpage,

"If pattern begins with /, all matches of pattern are replaced with string.  
Normally only the first match is replaced."

Shouldn't you add a leading / as for the \/ substitution ?

>  	params=$in_fmt-$out_fmt-$size$params
> 
>  	if [ x$__vsp_pixel_perfect != xtrue ] ; then

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters

[Kieran Bingham]

Hi Laurent,

Thanks for the review!

On 10/02/17 07:58, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 08 Feb 2017 14:03:57 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Parameters can contain characters not suited to use in filenames.
>>
>> Add '=','(', and ')' to the filtering, and replace with '_'
> 
> What's the issue with those characters ? :-)

Actually, now I think about it; the real issue is they break my automated file
conversion scripts.

The '=' breaks Makefile wildcard matching, and the '(', just cause extra
escaping to be required on the command line, when I try to examine the output files.

It could easily be argued that those cases could be handled elsewhere, but it
seemed easy to 'clean' the filename.

>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  scripts/vsp-lib.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
>> index 5aff30217a27..08bf8f36c582 100755
>> --- a/scripts/vsp-lib.sh
>> +++ b/scripts/vsp-lib.sh
>> @@ -263,6 +263,9 @@ compare_frames() {
>>  	local params=${args// /-}
>>  	params=${params:+-$params}
>>  	params=${params//\//_}
>> +	params=${params/=/_}
>> +	params=${params/(/_}
>> +	params=${params/)/_}
> 
> According to the bash manpage,
> 
> "If pattern begins with /, all matches of pattern are replaced with string.  
> Normally only the first match is replaced."
> 
> Shouldn't you add a leading / as for the \/ substitution ?

I tried that when I copied the existing line, but it caused breakage. But of
course my shell is 'busybox sh' and not bash ...

Hrm ... I've just retested on a local script, and it worked fine.
# ./k.sh
abc=def(ghi)jkl
abc_def(ghi)jkl
abc_def_ghi)jkl
abc_def_ghi_jkl

... So I must have been trying to work around a 'non-error' :D

--
Kieran

>>  	params=$in_fmt-$out_fmt-$size$params
>>
>>  	if [ x$__vsp_pixel_perfect != xtrue ] ; then
> 

[PATCH 3/5] gen-image: Implement option to parse an input crop

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Allow the user to specify an input crop in the form (X,Y)/WxH

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/gen-image.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+)

diff --git a/src/gen-image.c b/src/gen-image.c
index 9aabefa8389c..2f370e7a8ebd 100644
--- a/src/gen-image.c
+++ b/src/gen-image.c
@@ -97,6 +97,13 @@ struct format_info {
 	struct format_yuv_info yuv;
 };
 
+struct image_rect {
+	int left;
+	int top;
+	unsigned int width;
+	unsigned int height;
+};
+
 struct image {
 	const struct format_info *format;
 	unsigned int width;
@@ -136,6 +143,8 @@ struct options {
 	struct params params;
 	enum histogram_type histo_type;
 	uint8_t histo_areas[12];
+	bool crop;
+	struct image_rect inputcrop;
 };
 
 /* -----------------------------------------------------------------------------
@@ -1085,6 +1094,26 @@ static void image_flip(const struct image *input, struct image *output,
 }
 
 /* -----------------------------------------------------------------------------
+ * Image Cropping
+ */
+
+static void image_crop(const struct image *input, const struct image *output,
+		       const struct image_rect *crop)
+{
+	const uint8_t *idata = input->data;
+	uint8_t *odata = output->data;
+	unsigned int y;
+
+	for (y = 0; y < output->height; ++y) {
+		unsigned int offset = (crop->top * input->width + crop->left) * 3;
+
+		memcpy(odata + y * output->width * 3,
+		       idata + y * input->width * 3 + offset,
+		       output->width * 3);
+	}
+}
+
+/* -----------------------------------------------------------------------------
  * Look Up Table
  */
 
@@ -1539,6 +1568,22 @@ static int process(const struct options *options)
 		input = rgb;
 	}
 
+	if (options->crop) {
+		struct image *cropped;
+
+		cropped = image_new(input->format, options->inputcrop.width,
+				options->inputcrop.height);
+
+		if (!cropped) {
+			ret = -ENOMEM;
+			goto done;
+		}
+
+		image_crop(input, cropped, &options->inputcrop);
+		image_delete(input);
+		input = cropped;
+	}
+
 	/* Scale */
 	if (options->output_width && options->output_height) {
 		output_width = options->output_width;
@@ -1773,6 +1818,7 @@ static void usage(const char *argv0)
 	printf("				or percentages ([0%% - 100%%]). Defaults to 1.0\n");
 	printf("-c, --compose n			Compose n copies of the image offset by (50,50) over a black background\n");
 	printf("-C, --no-chroma-average		Disable chroma averaging for odd pixels on output\n");
+	printf("    --crop (X,Y)/WxH            Crop the input image\n");
 	printf("-e, --encoding enc		Set the YCbCr encoding method. Valid values are\n");
 	printf("				BT.601, REC.709, BT.2020 and SMPTE240M\n");
 	printf("-f, --format format		Set the output image format\n");
@@ -1813,11 +1859,13 @@ static void list_formats(void)
 #define OPT_VFLIP		257
 #define OPT_HISTOGRAM_TYPE	258
 #define OPT_HISTOGRAM_AREAS	259
+#define OPT_CROP		260
 
 static struct option opts[] = {
 	{"alpha", 1, 0, 'a'},
 	{"clu", 1, 0, 'L'},
 	{"compose", 1, 0, 'c'},
+	{"crop", 1, 0, OPT_CROP},
 	{"encoding", 1, 0, 'e'},
 	{"format", 1, 0, 'f'},
 	{"help", 0, 0, 'h'},
@@ -1836,6 +1884,58 @@ static struct option opts[] = {
 	{0, 0, 0, 0}
 };
 
+static int parse_crop(struct options *options, char *optarg)
+{
+	char * endptr;
+
+	/* (X,Y)/WxH */
+	endptr = optarg;
+	if (*endptr != '(') {
+		printf("Invalid crop argument '%s', expected '(', got '%c'\n", optarg, *endptr);
+		return 1;
+	}
+
+	options->inputcrop.left = strtol(endptr + 1, &endptr, 10);
+	if (*endptr != ',' || endptr == optarg) {
+		printf("Invalid crop position '%s', expected ',', got '%c'\n", optarg, *endptr);
+		return 1;
+	}
+
+	options->inputcrop.top = strtol(endptr + 1, &endptr, 10);
+	if (*endptr != ')' || endptr == optarg) {
+		printf("Invalid crop position '%s', expected ')', got '%c'\n", optarg, *endptr);
+		return 1;
+	}
+
+	if (*endptr != ')') {
+		printf("Invalid crop argument '%s', expected x, got '%c'\n", optarg, *endptr);
+		return 1;
+	}
+
+	endptr++;
+
+	options->inputcrop.width = strtol(endptr + 1, &endptr, 10);
+	if (*endptr != 'x' || endptr == optarg) {
+		printf("Invalid crop size '%s', expected x, got '%c'\n", optarg, *endptr);
+		return 1;
+	}
+
+	options->inputcrop.height = strtol(endptr + 1, &endptr, 10);
+	if (*endptr != 0) {
+		printf("Invalid crop size '%s'\n", optarg);
+		return 1;
+	}
+
+	if (options->inputcrop.left < 0 || options->inputcrop.top < 0) {
+		printf("Invalid negative crop position '%s'\n", optarg);
+		return 1;
+	}
+
+	options->crop = true;
+
+	return 0;
+}
+
 static int parse_args(struct options *options, int argc, char *argv[])
 {
 	char *endptr;
@@ -2024,6 +2124,12 @@ static int parse_args(struct options *options, int argc, char *argv[])
 			break;
 		}
 
+		case OPT_CROP:
+			if (parse_crop(options, optarg))
+				return 1;
+
+			break;
+
 		default:
 			printf("Invalid option -%c\n", c);
 			printf("Run %s -h for help.\n", argv[0]);
-- 
git-series 0.9.1

Re: [PATCH 3/5] gen-image: Implement option to parse an input crop

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Allow the user to specify an input crop in the form (X,Y)/WxH
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/gen-image.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+)
> 
> diff --git a/src/gen-image.c b/src/gen-image.c
> index 9aabefa8389c..2f370e7a8ebd 100644
> --- a/src/gen-image.c
> +++ b/src/gen-image.c
> @@ -97,6 +97,13 @@ struct format_info {
>  	struct format_yuv_info yuv;
>  };
> 
> +struct image_rect {
> +	int left;
> +	int top;
> +	unsigned int width;
> +	unsigned int height;
> +};
> +
>  struct image {
>  	const struct format_info *format;
>  	unsigned int width;
> @@ -136,6 +143,8 @@ struct options {
>  	struct params params;
>  	enum histogram_type histo_type;
>  	uint8_t histo_areas[12];

I'd like to merge this series in the near future, could you rebase it on top 
of the master branch instead of the histogram branch ?

> +	bool crop;
> +	struct image_rect inputcrop;
>  };
> 
>  /* ------------------------------------------------------------------------
> @@ -1085,6 +1094,26 @@ static void image_flip(const struct image *input,
> struct image *output, }
> 
>  /* ------------------------------------------------------------------------
>   + * Image Cropping
> + */
> +
> +static void image_crop(const struct image *input, const struct image
> *output,
> +		       const struct image_rect *crop)
> +{
> +	const uint8_t *idata = input->data;
> +	uint8_t *odata = output->data;
> +	unsigned int y;
> +
> +	for (y = 0; y < output->height; ++y) {
> +		unsigned int offset = (crop->top * input->width + crop->left) 
* 3;

This variable doesn't depend on the value of y, you can compute it outside of 
the loop.

> +		memcpy(odata + y * output->width * 3,
> +		       idata + y * input->width * 3 + offset,
> +		       output->width * 3);

Instead of having to multiply the stride by y in every iteration of the loop, 
you could do

	const uint8_t *idata = input->data + offset;

...
		memcpy(odata, idata, output->width * 3);
		odata += output->width * 3;
		idata += input->width * 3;

But in addition to that, not all formats have 3 bytes per pixel :-)

> +	}
> +}
> +
> +/* ------------------------------------------------------------------------
>   * Look Up Table
>   */
> 
> @@ -1539,6 +1568,22 @@ static int process(const struct options *options)
>  		input = rgb;
>  	}
> 
> +	if (options->crop) {
> +		struct image *cropped;
> +
> +		cropped = image_new(input->format, options->inputcrop.width,
> +				options->inputcrop.height);
> +

I'd remove this blank line to keep the test logically grouped with the 
image_new() call.

> +		if (!cropped) {
> +			ret = -ENOMEM;
> +			goto done;
> +		}
> +
> +		image_crop(input, cropped, &options->inputcrop);
> +		image_delete(input);
> +		input = cropped;
> +	}
> +
>  	/* Scale */
>  	if (options->output_width && options->output_height) {
>  		output_width = options->output_width;
> @@ -1773,6 +1818,7 @@ static void usage(const char *argv0)
>  	printf("				or percentages ([0%% -
>  	100%%]). Defaults to 1.0\n");
>  	printf("-c, --compose n			Compose n copies of the image
>  	offset by (50,50)
> over a black background\n"); printf("-C, --no-chroma-average	Disable
> chroma averaging for odd pixels on output\n");
> +	printf("    --crop (X,Y)/WxH            Crop the input image\n");
>  	printf("-e, --encoding enc		Set the YCbCr encoding method.
> Valid values are\n");
> printf("				BT.601, REC.709, BT.2020 and
> SMPTE240M\n");
>  	printf("-f, --format format		Set the output image 
format\n");
> @@ -1813,11 +1859,13 @@ static void list_formats(void)
>  #define OPT_VFLIP		257
>  #define OPT_HISTOGRAM_TYPE	258
>  #define OPT_HISTOGRAM_AREAS	259
> +#define OPT_CROP		260
> 
>  static struct option opts[] = {
>  	{"alpha", 1, 0, 'a'},
>  	{"clu", 1, 0, 'L'},
>  	{"compose", 1, 0, 'c'},
> +	{"crop", 1, 0, OPT_CROP},
>  	{"encoding", 1, 0, 'e'},
>  	{"format", 1, 0, 'f'},
>  	{"help", 0, 0, 'h'},
> @@ -1836,6 +1884,58 @@ static struct option opts[] = {
>  	{0, 0, 0, 0}
>  };
> 
> +static int parse_crop(struct options *options, char *optarg)

I think you should pass an image_crop pointer to this function, to make it 
reusable should we add other crop options later.

> +{
> +	char * endptr;

s/* /*/

> +
> +	/* (X,Y)/WxH */
> +	endptr = optarg;
> +	if (*endptr != '(') {
> +		printf("Invalid crop argument '%s', expected '(', got '%c'\n", 
optarg,
> *endptr);

Could you split the line after the format string to avoid going over the 80 
characters limit ? It's not as hard a limit in gen-image as it is in the 
kernel, but it's a good practice nonetheless.

> +		return 1;
> +	}
> +
> +	options->inputcrop.left = strtol(endptr + 1, &endptr, 10);
> +	if (*endptr != ',' || endptr == optarg) {
> +		printf("Invalid crop position '%s', expected ',', got '%c'\n",
> optarg, *endptr);

How about using something similar to media_print_streampos() (from media-ctl) 
to parse error messages ? You could then shorten the messages as the tool 
would show the location where the error happened.

> +		return 1;
> +	}
> +
> +	options->inputcrop.top = strtol(endptr + 1, &endptr, 10);
> +	if (*endptr != ')' || endptr == optarg) {
> +		printf("Invalid crop position '%s', expected ')', got '%c'\n",
> optarg, *endptr);
> +		return 1;
> +	}
> +
> +	if (*endptr != ')') {
> +		printf("Invalid crop argument '%s', expected x, got '%c'\n",
> optarg, *endptr);
> +		return 1;
> +	}
> +
> +	endptr++;

Shouldn't you test for '/' here ?

> +
> +	options->inputcrop.width = strtol(endptr + 1, &endptr, 10);
> +	if (*endptr != 'x' || endptr == optarg) {
> +		printf("Invalid crop size '%s', expected x, got '%c'\n",
> optarg, *endptr);
> +		return 1;
> +	}
> +
> +	options->inputcrop.height = strtol(endptr + 1, &endptr, 10);
> +	if (*endptr != 0) {
> +		printf("Invalid crop size '%s'\n", optarg);
> +		return 1;
> +	}
> +
> +	if (options->inputcrop.left < 0 || options->inputcrop.top < 0) {
> +		printf("Invalid negative crop position '%s'\n", optarg);
> +		return 1;
> +	}
> +
> +	options->crop = true;

If you pass a crop rectangle pointer to the function, this line should be 
moved out to the caller.
> +
> +	return 0;
> +}
> +
>  static int parse_args(struct options *options, int argc, char *argv[])
>  {
>  	char *endptr;
> @@ -2024,6 +2124,12 @@ static int parse_args(struct options *options, int
> argc, char *argv[]) break;
>  		}
> 
> +		case OPT_CROP:
> +			if (parse_crop(options, optarg))
> +				return 1;
> +
> +			break;
> +
>  		default:
>  			printf("Invalid option -%c\n", c);
>  			printf("Run %s -h for help.\n", argv[0]);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] gen-image: Implement option to parse an input crop

[Kieran Bingham]

Hi Laurent,

Thanks for the review,

On 10/02/17 08:19, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Allow the user to specify an input crop in the form (X,Y)/WxH
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/gen-image.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 106 insertions(+)
>>
>> diff --git a/src/gen-image.c b/src/gen-image.c
>> index 9aabefa8389c..2f370e7a8ebd 100644
>> --- a/src/gen-image.c
>> +++ b/src/gen-image.c
>> @@ -97,6 +97,13 @@ struct format_info {
>>  	struct format_yuv_info yuv;
>>  };
>>
>> +struct image_rect {
>> +	int left;
>> +	int top;
>> +	unsigned int width;
>> +	unsigned int height;
>> +};
>> +
>>  struct image {
>>  	const struct format_info *format;
>>  	unsigned int width;
>> @@ -136,6 +143,8 @@ struct options {
>>  	struct params params;
>>  	enum histogram_type histo_type;
>>  	uint8_t histo_areas[12];
> 
> I'd like to merge this series in the near future, could you rebase it on top 
> of the master branch instead of the histogram branch ?

Ack.

>> +	bool crop;
>> +	struct image_rect inputcrop;
>>  };
>>
>>  /* ------------------------------------------------------------------------
>> @@ -1085,6 +1094,26 @@ static void image_flip(const struct image *input,
>> struct image *output, }
>>
>>  /* ------------------------------------------------------------------------
>>   + * Image Cropping
>> + */
>> +
>> +static void image_crop(const struct image *input, const struct image
>> *output,
>> +		       const struct image_rect *crop)
>> +{
>> +	const uint8_t *idata = input->data;
>> +	uint8_t *odata = output->data;
>> +	unsigned int y;
>> +
>> +	for (y = 0; y < output->height; ++y) {
>> +		unsigned int offset = (crop->top * input->width + crop->left) 
> * 3;
> 
> This variable doesn't depend on the value of y, you can compute it outside of 
> the loop.

Ah yes, :D
Done.

>> +		memcpy(odata + y * output->width * 3,
>> +		       idata + y * input->width * 3 + offset,
>> +		       output->width * 3);
> 
> Instead of having to multiply the stride by y in every iteration of the loop, 
> you could do
> 
> 	const uint8_t *idata = input->data + offset;
> 
> ...
> 		memcpy(odata, idata, output->width * 3);
> 		odata += output->width * 3;
> 		idata += input->width * 3;
> 

I was replicating from the compose function, - but I'm fine with this.
Done.

> But in addition to that, not all formats have 3 bytes per pixel :-)

Ah, but I thought they do at the point I call this function. This conversion
only occurs after the formats are converted

	/* Convert colorspace */
	if (options->input_format->type == FORMAT_YUV) {
		... # Image converted to YUV24
	} else if (options->input_format->rgb.bpp < 24) {
		... # Image converted to RGB24
	}

	if (options->crop) {
		... Actual crop performed, on 24bpp image.
	}

Perhaps it would be useful to declare that this function only operates on 24bit
images somehow. It is accordingly located next to image_scale, image_compose,
image_rotate, and image_flip which also operate on 24bpp images.

We can always make it more generic later if we need to use the code in other
parts of gen-image

>> +	}
>> +}
>> +
>> +/* ------------------------------------------------------------------------
>>   * Look Up Table
>>   */
>>
>> @@ -1539,6 +1568,22 @@ static int process(const struct options *options)
>>  		input = rgb;
>>  	}
>>
>> +	if (options->crop) {
>> +		struct image *cropped;
>> +
>> +		cropped = image_new(input->format, options->inputcrop.width,
>> +				options->inputcrop.height);
>> +
> 
> I'd remove this blank line to keep the test logically grouped with the 
> image_new() call.

Ack.
Done.

> 
>> +		if (!cropped) {
>> +			ret = -ENOMEM;
>> +			goto done;
>> +		}
>> +
>> +		image_crop(input, cropped, &options->inputcrop);
>> +		image_delete(input);
>> +		input = cropped;
>> +	}
>> +
>>  	/* Scale */
>>  	if (options->output_width && options->output_height) {
>>  		output_width = options->output_width;
>> @@ -1773,6 +1818,7 @@ static void usage(const char *argv0)
>>  	printf("				or percentages ([0%% -
>>  	100%%]). Defaults to 1.0\n");
>>  	printf("-c, --compose n			Compose n copies of the image
>>  	offset by (50,50)
>> over a black background\n"); printf("-C, --no-chroma-average	Disable
>> chroma averaging for odd pixels on output\n");
>> +	printf("    --crop (X,Y)/WxH            Crop the input image\n");
>>  	printf("-e, --encoding enc		Set the YCbCr encoding method.
>> Valid values are\n");
>> printf("				BT.601, REC.709, BT.2020 and
>> SMPTE240M\n");
>>  	printf("-f, --format format		Set the output image 
> format\n");
>> @@ -1813,11 +1859,13 @@ static void list_formats(void)
>>  #define OPT_VFLIP		257
>>  #define OPT_HISTOGRAM_TYPE	258
>>  #define OPT_HISTOGRAM_AREAS	259
>> +#define OPT_CROP		260
>>
>>  static struct option opts[] = {
>>  	{"alpha", 1, 0, 'a'},
>>  	{"clu", 1, 0, 'L'},
>>  	{"compose", 1, 0, 'c'},
>> +	{"crop", 1, 0, OPT_CROP},
>>  	{"encoding", 1, 0, 'e'},
>>  	{"format", 1, 0, 'f'},
>>  	{"help", 0, 0, 'h'},
>> @@ -1836,6 +1884,58 @@ static struct option opts[] = {
>>  	{0, 0, 0, 0}
>>  };
>>
>> +static int parse_crop(struct options *options, char *optarg)
> 
> I think you should pass an image_crop pointer to this function, to make it 
> reusable should we add other crop options later.

That's very reasonable :D

Done.

>> +{
>> +	char * endptr;
> 
> s/* /*/
> 
>> +
>> +	/* (X,Y)/WxH */
>> +	endptr = optarg;
>> +	if (*endptr != '(') {
>> +		printf("Invalid crop argument '%s', expected '(', got '%c'\n", 
> optarg,
>> *endptr);
> 
> Could you split the line after the format string to avoid going over the 80 
> characters limit ? It's not as hard a limit in gen-image as it is in the 
> kernel, but it's a good practice nonetheless.
> 
>> +		return 1;
>> +	}
>> +
>> +	options->inputcrop.left = strtol(endptr + 1, &endptr, 10);
>> +	if (*endptr != ',' || endptr == optarg) {
>> +		printf("Invalid crop position '%s', expected ',', got '%c'\n",
>> optarg, *endptr);
> 
> How about using something similar to media_print_streampos() (from media-ctl) 
> to parse error messages ? You could then shorten the messages as the tool 
> would show the location where the error happened.

That sounds good if it's easily replicable. I'll dig it out.


>> +		return 1;
>> +	}
>> +
>> +	options->inputcrop.top = strtol(endptr + 1, &endptr, 10);
>> +	if (*endptr != ')' || endptr == optarg) {
>> +		printf("Invalid crop position '%s', expected ')', got '%c'\n",
>> optarg, *endptr);
>> +		return 1;
>> +	}
>> +
>> +	if (*endptr != ')') {
>> +		printf("Invalid crop argument '%s', expected x, got '%c'\n",
>> optarg, *endptr);
>> +		return 1;
>> +	}
>> +
>> +	endptr++;
> 
> Shouldn't you test for '/' here ?

Eeek - I was sure I was ... Where did it go :( ... I must have lost the check
when I added in '(', ')' syntax. Apologies, and will-fix.

Done.

>> +
>> +	options->inputcrop.width = strtol(endptr + 1, &endptr, 10);
>> +	if (*endptr != 'x' || endptr == optarg) {
>> +		printf("Invalid crop size '%s', expected x, got '%c'\n",
>> optarg, *endptr);
>> +		return 1;
>> +	}
>> +
>> +	options->inputcrop.height = strtol(endptr + 1, &endptr, 10);
>> +	if (*endptr != 0) {
>> +		printf("Invalid crop size '%s'\n", optarg);
>> +		return 1;
>> +	}
>> +
>> +	if (options->inputcrop.left < 0 || options->inputcrop.top < 0) {
>> +		printf("Invalid negative crop position '%s'\n", optarg);
>> +		return 1;
>> +	}
>> +
>> +	options->crop = true;
> 
> If you pass a crop rectangle pointer to the function, this line should be 
> moved out to the caller.

Ack, Done.

>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_args(struct options *options, int argc, char *argv[])
>>  {
>>  	char *endptr;
>> @@ -2024,6 +2124,12 @@ static int parse_args(struct options *options, int
>> argc, char *argv[]) break;
>>  		}
>>
>> +		case OPT_CROP:
>> +			if (parse_crop(options, optarg))
>> +				return 1;
>> +
>> +			break;
>> +
>>  		default:
>>  			printf("Invalid option -%c\n", c);
>>  			printf("Run %s -h for help.\n", argv[0]);
> 

Re: [PATCH 3/5] gen-image: Implement option to parse an input crop

[Laurent Pinchart]

Hi Kieran,

On Friday 10 Feb 2017 11:18:09 Kieran Bingham wrote:
> On 10/02/17 08:19, Laurent Pinchart wrote:
> > On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> 
> >> Allow the user to specify an input crop in the form (X,Y)/WxH
> >> 
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >> 
> >>  src/gen-image.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 106 insertions(+)
> >> 
> >> diff --git a/src/gen-image.c b/src/gen-image.c
> >> index 9aabefa8389c..2f370e7a8ebd 100644
> >> --- a/src/gen-image.c
> >> +++ b/src/gen-image.c

[snip]

> >> +static void image_crop(const struct image *input, const struct image
> >> *output,
> >> +		       const struct image_rect *crop)
> >> +{
> >> +	const uint8_t *idata = input->data;
> >> +	uint8_t *odata = output->data;
> >> +	unsigned int y;
> >> +
> >> +	for (y = 0; y < output->height; ++y) {
> >> +		unsigned int offset = (crop->top * input->width + crop->left)
> > 
> > * 3;
> > 
> > This variable doesn't depend on the value of y, you can compute it outside
> > of the loop.
> 
> Ah yes, :D
> Done.
> 
> >> +		memcpy(odata + y * output->width * 3,
> >> +		       idata + y * input->width * 3 + offset,
> >> +		       output->width * 3);
> > 
> > Instead of having to multiply the stride by y in every iteration of the
> > loop, you could do
> > 
> > 	const uint8_t *idata = input->data + offset;
> > 
> > ...
> > 
> > 		memcpy(odata, idata, output->width * 3);
> > 		odata += output->width * 3;
> > 		idata += input->width * 3;
> 
> I was replicating from the compose function, - but I'm fine with this.
> Done.
> 
> > But in addition to that, not all formats have 3 bytes per pixel :-)
> 
> Ah, but I thought they do at the point I call this function. This conversion
> only occurs after the formats are converted
> 
> 	/* Convert colorspace */
> 	if (options->input_format->type == FORMAT_YUV) {
> 		... # Image converted to YUV24
> 	} else if (options->input_format->rgb.bpp < 24) {
> 		... # Image converted to RGB24
> 	}
> 
> 	if (options->crop) {
> 		... Actual crop performed, on 24bpp image.
> 	}
> 
> Perhaps it would be useful to declare that this function only operates on
> 24bit images somehow. It is accordingly located next to image_scale,
> image_compose, image_rotate, and image_flip which also operate on 24bpp
> images.

You're right, my bad. The code is thus fine from that point of view.

> We can always make it more generic later if we need to use the code in other
> parts of gen-image

That's fine with me.

> >> +	}
> >> +}

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] vsp-lib: Support RPF frame cropping

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Pass the optional '--crop (X,Y)/WxH' parameter through reference_frame
allowing the input to be cropped for comparison

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 scripts/vsp-lib.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
index 08bf8f36c582..42a4bb20d1be 100755
--- a/scripts/vsp-lib.sh
+++ b/scripts/vsp-lib.sh
@@ -162,6 +162,9 @@ reference_frame() {
 		vflip)
 			[ x$value = x1 ] && options="$options --vflip"
 			;;
+		crop)
+			options="$options --crop $value"
+			;;
 		esac
 	done
 
@@ -717,6 +720,40 @@ format_rpf_wpf() {
 	__vsp_wpf_format=$5
 }
 
+format_crop_rpf_wpf() {
+	local rpf=$1
+	local wpf=$2
+	local infmt=$(format_v4l2_to_mbus $3)
+	local size=$4
+	local outfmt=$(format_v4l2_to_mbus $5)
+	local rpfcrop=$6
+	local wpfcrop=$7
+	local rpfoutsize=
+	local outsize=
+
+	if [ x$rpfcrop != 'x' ] ; then
+		rpfcrop="crop:$rpfcrop"
+		rpfoutsize=$(echo $rpfcrop | sed 's/.*\///')
+	else
+		rpfoutsize=$size
+	fi
+
+	if [ x$wpfcrop != 'x' ] ; then
+		wpfcrop="crop:$wpfcrop"
+		outsize=$(echo $wpfcrop | sed 's/.*\///')
+	else
+		outsize=$rpfoutsize
+	fi
+
+	$mediactl -d $mdev -V "'$dev rpf.$rpf':0 [fmt:$infmt/$size $rpfcrop]"
+	$mediactl -d $mdev -V "'$dev rpf.$rpf':1 [fmt:$infmt/$rpfoutsize]"
+	$mediactl -d $mdev -V "'$dev wpf.$wpf':0 [fmt:$infmt/$rpfoutsize $wpfcrop]"
+	$mediactl -d $mdev -V "'$dev wpf.$wpf':1 [fmt:$outfmt/$outsize]"
+
+	__vsp_rpf_format=$3
+	__vsp_wpf_format=$5
+}
+
 format_wpf() {
 	local format=$(format_v4l2_to_mbus $1)
 	local size=$2
-- 
git-series 0.9.1

Re: [PATCH 4/5] vsp-lib: Support RPF frame cropping

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:03:59 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Pass the optional '--crop (X,Y)/WxH' parameter through reference_frame
> allowing the input to be cropped for comparison
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  scripts/vsp-lib.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
> index 08bf8f36c582..42a4bb20d1be 100755
> --- a/scripts/vsp-lib.sh
> +++ b/scripts/vsp-lib.sh
> @@ -162,6 +162,9 @@ reference_frame() {
>  		vflip)
>  			[ x$value = x1 ] && options="$options --vflip"
>  			;;
> +		crop)
> +			options="$options --crop $value"
> +			;;

Could you please keep the options sorted alphabetically ?

>  		esac
>  	done
> 
> @@ -717,6 +720,40 @@ format_rpf_wpf() {
>  	__vsp_wpf_format=$5
>  }
> 
> +format_crop_rpf_wpf() {
> +	local rpf=$1
> +	local wpf=$2
> +	local infmt=$(format_v4l2_to_mbus $3)
> +	local size=$4
> +	local outfmt=$(format_v4l2_to_mbus $5)
> +	local rpfcrop=$6
> +	local wpfcrop=$7
> +	local rpfoutsize=
> +	local outsize=
> +
> +	if [ x$rpfcrop != 'x' ] ; then

I don't think this will work properly. You want to test for the presence of an 
RPF crop argument. If it's absent, and the WPF crop argument is specified, WPF 
crop will be stored in $6, and will thus be assigned to $rpfcrop.

I see two possible solutions. One of them is to make the RPF crop argument 
mandatory. After all, given the function name, one can expect RPF crop to be 
specified, otherwise the caller should use rpf-wpf, not crop-rpf-wpf. Another 
more versatile option would be to add RPF crop support to the existing 
format_rpf_wpf() crop function, and make both RPF and WPF crop optional. You 
could use named option for that (rcrop=... wcrop=...) or use a special value 
to indicate that an option should be skipped (for instance "- (0,0)/640x480" 
to set the WPF crop rectangle without setting the RPF crop rectangle).

> +		rpfcrop="crop:$rpfcrop"
> +		rpfoutsize=$(echo $rpfcrop | sed 's/.*\///')
> +	else
> +		rpfoutsize=$size
> +	fi
> +
> +	if [ x$wpfcrop != 'x' ] ; then
> +		wpfcrop="crop:$wpfcrop"
> +		outsize=$(echo $wpfcrop | sed 's/.*\///')
> +	else
> +		outsize=$rpfoutsize
> +	fi
> +
> +	$mediactl -d $mdev -V "'$dev rpf.$rpf':0 [fmt:$infmt/$size $rpfcrop]"
> +	$mediactl -d $mdev -V "'$dev rpf.$rpf':1 [fmt:$infmt/$rpfoutsize]"
> +	$mediactl -d $mdev -V "'$dev wpf.$wpf':0 [fmt:$infmt/$rpfoutsize
> $wpfcrop]"
> +	$mediactl -d $mdev -V "'$dev wpf.$wpf':1 [fmt:$outfmt/$outsize]"
> +
> +	__vsp_rpf_format=$3
> +	__vsp_wpf_format=$5
> +}
> +
>  format_wpf() {
>  	local format=$(format_v4l2_to_mbus $1)
>  	local size=$2

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] vsp-lib: Support RPF frame cropping

[Kieran Bingham]

Ooops, I left this on my screen, and have already sent the V2 anyway.

Hitting send for posterity.

--
Kieran

On 10/02/17 09:20, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 08 Feb 2017 14:03:59 Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> Pass the optional '--crop (X,Y)/WxH' parameter through reference_frame
>> allowing the input to be cropped for comparison
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  scripts/vsp-lib.sh | 37 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>
>> diff --git a/scripts/vsp-lib.sh b/scripts/vsp-lib.sh
>> index 08bf8f36c582..42a4bb20d1be 100755
>> --- a/scripts/vsp-lib.sh
>> +++ b/scripts/vsp-lib.sh
>> @@ -162,6 +162,9 @@ reference_frame() {
>>  		vflip)
>>  			[ x$value = x1 ] && options="$options --vflip"
>>  			;;
>> +		crop)
>> +			options="$options --crop $value"
>> +			;;
> 
> Could you please keep the options sorted alphabetically ?

Fixed,

> 
>>  		esac
>>  	done
>>
>> @@ -717,6 +720,40 @@ format_rpf_wpf() {
>>  	__vsp_wpf_format=$5
>>  }
>>
>> +format_crop_rpf_wpf() {
>> +	local rpf=$1
>> +	local wpf=$2
>> +	local infmt=$(format_v4l2_to_mbus $3)
>> +	local size=$4
>> +	local outfmt=$(format_v4l2_to_mbus $5)
>> +	local rpfcrop=$6
>> +	local wpfcrop=$7
>> +	local rpfoutsize=
>> +	local outsize=
>> +
>> +	if [ x$rpfcrop != 'x' ] ; then
> 
> I don't think this will work properly. You want to test for the presence of an 
> RPF crop argument. If it's absent, and the WPF crop argument is specified, WPF 
> crop will be stored in $6, and will thus be assigned to $rpfcrop.

Yes, you're correct.

> 
> I see two possible solutions. One of them is to make the RPF crop argument 
> mandatory. After all, given the function name, one can expect RPF crop to be 
> specified, otherwise the caller should use rpf-wpf, not crop-rpf-wpf. Another 
> more versatile option would be to add RPF crop support to the existing 
> format_rpf_wpf() crop function, and make both RPF and WPF crop optional. You 
> could use named option for that (rcrop=... wcrop=...) or use a special value 
> to indicate that an option should be skipped (for instance "- (0,0)/640x480" 
> to set the WPF crop rectangle without setting the RPF crop rectangle).

I think prefer named arguments over positional arguments in non-type-checked
code such as this...

Although I think I went down the 'fast' route of duplicating rpf-crop over
adding named arguments. That's not a good enough reason not to do it though ...

As there are no current users of the 'wpf' crop parameter, I've changed this to
support --rpfcrop=... --wpfcrop=...


> 
>> +		rpfcrop="crop:$rpfcrop"
>> +		rpfoutsize=$(echo $rpfcrop | sed 's/.*\///')
>> +	else
>> +		rpfoutsize=$size
>> +	fi
>> +
>> +	if [ x$wpfcrop != 'x' ] ; then
>> +		wpfcrop="crop:$wpfcrop"
>> +		outsize=$(echo $wpfcrop | sed 's/.*\///')
>> +	else
>> +		outsize=$rpfoutsize
>> +	fi
>> +
>> +	$mediactl -d $mdev -V "'$dev rpf.$rpf':0 [fmt:$infmt/$size $rpfcrop]"
>> +	$mediactl -d $mdev -V "'$dev rpf.$rpf':1 [fmt:$infmt/$rpfoutsize]"
>> +	$mediactl -d $mdev -V "'$dev wpf.$wpf':0 [fmt:$infmt/$rpfoutsize
>> $wpfcrop]"
>> +	$mediactl -d $mdev -V "'$dev wpf.$wpf':1 [fmt:$outfmt/$outsize]"
>> +
>> +	__vsp_rpf_format=$3
>> +	__vsp_wpf_format=$5
>> +}
>> +
>>  format_wpf() {
>>  	local format=$(format_v4l2_to_mbus $1)
>>  	local size=$2
> 
--
Kieran

[PATCH 5/5] tests: Add RPF cropping test

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Test both the input cropping size and position

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 tests/vsp-unit-test-0021.sh | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+)
 create mode 100755 tests/vsp-unit-test-0021.sh

diff --git a/tests/vsp-unit-test-0021.sh b/tests/vsp-unit-test-0021.sh
new file mode 100755
index 000000000000..d00dd0dece97
--- /dev/null
+++ b/tests/vsp-unit-test-0021.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+#
+# Test RPF crop in RGB and YUV mode. Use a RPF -> WPF pipeline with a fixed ARGB32
+# format on the input and capture output frames in all RGB formats supported
+# by the WPF.
+#
+
+source vsp-lib.sh
+
+features="rpf.0 wpf.0"
+crops="(0,0)/512x384 (32,32)/512x384 (32,64)/512x384 (64,32)/512x384"
+
+
+test_rpf_cropping() {
+	test_start "RPF crop from $crop"
+
+	pipe_configure rpf-wpf 0 0
+	format_configure crop-rpf-wpf 0 0 RGB24 1024x768 ARGB32 $crop
+
+	vsp_runner rpf.0 &
+	vsp_runner wpf.0
+
+	local result=$(compare_frames crop=${crop})
+
+	test_complete $result
+}
+
+test_main() {
+	local format
+	local crop
+
+	for crop in $crops ; do
+		test_rpf_cropping $crop
+	done
+}
+
+test_init $0 "$features"
+test_run
-- 
git-series 0.9.1

Re: [PATCH 5/5] tests: Add RPF cropping test

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:04:00 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Test both the input cropping size and position
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  tests/vsp-unit-test-0021.sh | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+)
>  create mode 100755 tests/vsp-unit-test-0021.sh
> 
> diff --git a/tests/vsp-unit-test-0021.sh b/tests/vsp-unit-test-0021.sh
> new file mode 100755
> index 000000000000..d00dd0dece97
> --- /dev/null
> +++ b/tests/vsp-unit-test-0021.sh
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +
> +#
> +# Test RPF crop in RGB and YUV mode. Use a RPF -> WPF pipeline with a fixed
> ARGB32
> +# format on the input and capture output frames in all RGB formats
> supported
> +# by the WPF.
> +#
> +
> +source vsp-lib.sh
> +
> +features="rpf.0 wpf.0"
> +crops="(0,0)/512x384 (32,32)/512x384 (32,64)/512x384 (64,32)/512x384"
> +
> +

A single blank line should do.

> +test_rpf_cropping() {
> +	test_start "RPF crop from $crop"
> +
> +	pipe_configure rpf-wpf 0 0
> +	format_configure crop-rpf-wpf 0 0 RGB24 1024x768 ARGB32 $crop
> +
> +	vsp_runner rpf.0 &
> +	vsp_runner wpf.0
> +
> +	local result=$(compare_frames crop=${crop})
> +
> +	test_complete $result
> +}
> +
> +test_main() {
> +	local format

You don't use this variable.

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	local crop
> +
> +	for crop in $crops ; do
> +		test_rpf_cropping $crop
> +	done
> +}
> +
> +test_init $0 "$features"
> +test_run

-- 
Regards,

Laurent Pinchart