* [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
* 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
* [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
* 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 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
* [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
* 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 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 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
* [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
* 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 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
* [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 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
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.