Hi Kieran,
Thank you for the patch.
On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Allow the user to specify an input crop in the form (X,Y)/WxH
>
> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> ---
> 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
