* [PATCH 0/5] vsp-tests: Implement RPF cropping tests
@ 2017-02-08 14:03 Kieran Bingham
2017-02-08 14:03 ` [PATCH 1/5] vsp-lib: sort output frames correctly Kieran Bingham
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:03 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/5] vsp-lib: sort output frames correctly
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
@ 2017-02-08 14:03 ` Kieran Bingham
2017-02-10 9:21 ` Laurent Pinchart
2017-02-08 14:03 ` [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters Kieran Bingham
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:03 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
2017-02-08 14:03 ` [PATCH 1/5] vsp-lib: sort output frames correctly Kieran Bingham
@ 2017-02-08 14:03 ` Kieran Bingham
2017-02-10 7:58 ` Laurent Pinchart
2017-02-08 14:03 ` [PATCH 3/5] gen-image: Implement option to parse an input crop Kieran Bingham
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:03 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/5] gen-image: Implement option to parse an input crop
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
2017-02-08 14:03 ` [PATCH 1/5] vsp-lib: sort output frames correctly Kieran Bingham
2017-02-08 14:03 ` [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters Kieran Bingham
@ 2017-02-08 14:03 ` Kieran Bingham
2017-02-10 8:19 ` Laurent Pinchart
2017-02-08 14:03 ` [PATCH 4/5] vsp-lib: Support RPF frame cropping Kieran Bingham
2017-02-08 14:04 ` [PATCH 5/5] tests: Add RPF cropping test Kieran Bingham
4 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:03 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/5] vsp-lib: Support RPF frame cropping
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
` (2 preceding siblings ...)
2017-02-08 14:03 ` [PATCH 3/5] gen-image: Implement option to parse an input crop Kieran Bingham
@ 2017-02-08 14:03 ` Kieran Bingham
2017-02-10 9:20 ` Laurent Pinchart
2017-02-08 14:04 ` [PATCH 5/5] tests: Add RPF cropping test Kieran Bingham
4 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:03 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/5] tests: Add RPF cropping test
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
` (3 preceding siblings ...)
2017-02-08 14:03 ` [PATCH 4/5] vsp-lib: Support RPF frame cropping Kieran Bingham
@ 2017-02-08 14:04 ` Kieran Bingham
2017-02-10 9:22 ` Laurent Pinchart
4 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-08 14:04 UTC (permalink / raw)
To: laurent.pinchart, linux-renesas-soc, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters
2017-02-08 14:03 ` [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters Kieran Bingham
@ 2017-02-10 7:58 ` Laurent Pinchart
2017-02-10 9:08 ` Kieran Bingham
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-10 7:58 UTC (permalink / raw)
To: Kieran Bingham; +Cc: linux-renesas-soc, kieran.bingham
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gen-image: Implement option to parse an input crop
2017-02-08 14:03 ` [PATCH 3/5] gen-image: Implement option to parse an input crop Kieran Bingham
@ 2017-02-10 8:19 ` Laurent Pinchart
2017-02-10 11:18 ` Kieran Bingham
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-10 8:19 UTC (permalink / raw)
To: Kieran Bingham; +Cc: linux-renesas-soc, kieran.bingham
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters
2017-02-10 7:58 ` Laurent Pinchart
@ 2017-02-10 9:08 ` Kieran Bingham
0 siblings, 0 replies; 15+ messages in thread
From: Kieran Bingham @ 2017-02-10 9:08 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham; +Cc: linux-renesas-soc
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] vsp-lib: Support RPF frame cropping
2017-02-08 14:03 ` [PATCH 4/5] vsp-lib: Support RPF frame cropping Kieran Bingham
@ 2017-02-10 9:20 ` Laurent Pinchart
2017-02-10 14:53 ` Kieran Bingham
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-10 9:20 UTC (permalink / raw)
To: Kieran Bingham; +Cc: linux-renesas-soc, kieran.bingham
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] vsp-lib: sort output frames correctly
2017-02-08 14:03 ` [PATCH 1/5] vsp-lib: sort output frames correctly Kieran Bingham
@ 2017-02-10 9:21 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-10 9:21 UTC (permalink / raw)
To: Kieran Bingham; +Cc: linux-renesas-soc, kieran.bingham
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] tests: Add RPF cropping test
2017-02-08 14:04 ` [PATCH 5/5] tests: Add RPF cropping test Kieran Bingham
@ 2017-02-10 9:22 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-10 9:22 UTC (permalink / raw)
To: Kieran Bingham; +Cc: linux-renesas-soc, kieran.bingham
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gen-image: Implement option to parse an input crop
2017-02-10 8:19 ` Laurent Pinchart
@ 2017-02-10 11:18 ` Kieran Bingham
2017-02-13 19:48 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Kieran Bingham @ 2017-02-10 11:18 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham; +Cc: linux-renesas-soc
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]);
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] vsp-lib: Support RPF frame cropping
2017-02-10 9:20 ` Laurent Pinchart
@ 2017-02-10 14:53 ` Kieran Bingham
0 siblings, 0 replies; 15+ messages in thread
From: Kieran Bingham @ 2017-02-10 14:53 UTC (permalink / raw)
To: Laurent Pinchart, Kieran Bingham; +Cc: linux-renesas-soc
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gen-image: Implement option to parse an input crop
2017-02-10 11:18 ` Kieran Bingham
@ 2017-02-13 19:48 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2017-02-13 19:48 UTC (permalink / raw)
To: Kieran Bingham; +Cc: Kieran Bingham, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-02-13 19:48 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-08 14:03 [PATCH 0/5] vsp-tests: Implement RPF cropping tests Kieran Bingham
2017-02-08 14:03 ` [PATCH 1/5] vsp-lib: sort output frames correctly Kieran Bingham
2017-02-10 9:21 ` Laurent Pinchart
2017-02-08 14:03 ` [PATCH 2/5] vsp-lib: Filter non-filesystem regular characters Kieran Bingham
2017-02-10 7:58 ` Laurent Pinchart
2017-02-10 9:08 ` Kieran Bingham
2017-02-08 14:03 ` [PATCH 3/5] gen-image: Implement option to parse an input crop Kieran Bingham
2017-02-10 8:19 ` Laurent Pinchart
2017-02-10 11:18 ` Kieran Bingham
2017-02-13 19:48 ` Laurent Pinchart
2017-02-08 14:03 ` [PATCH 4/5] vsp-lib: Support RPF frame cropping Kieran Bingham
2017-02-10 9:20 ` Laurent Pinchart
2017-02-10 14:53 ` Kieran Bingham
2017-02-08 14:04 ` [PATCH 5/5] tests: Add RPF cropping test Kieran Bingham
2017-02-10 9:22 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.