Re: [PATCH v2 2/4] gen-image: Implement option to parse an input crop

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 13:30:06 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Allow the user to specify an input crop in the form (X,Y)/WxH
> 
> Signed-off-by: Kieran Bingham 
> ---
>  src/gen-image.c | 132 +-
>  1 file changed, 132 insertions(+)
> 
> diff --git a/src/gen-image.c b/src/gen-image.c
> index 31d42e0211db..088e8b26f648 100644
> --- a/src/gen-image.c
> +++ b/src/gen-image.c
> @@ -95,6 +95,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;
> @@ -127,6 +134,8 @@ struct options {
>   bool rotate;
>   unsigned int compose;
>   struct params params;
> + bool crop;
> + struct image_rect inputcrop;
>  };
> 
>  /* 
> @@ -1076,6 +1085,25 @@ 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)
> +{
> + unsigned int offset = (crop->top * input->width + crop->left) * 3;
> + const uint8_t *idata = input->data + offset;
> + uint8_t *odata = output->data;
> + unsigned int y;
> +
> + for (y = 0; y < output->height; ++y) {
> + memcpy(odata, idata, output->width * 3);
> + odata += output->width * 3;
> + idata += input->width * 3;
> + }
> +}
> +
> +/* 
>   * Look Up Table
>   */
> 
> @@ -1363,6 +1391,21 @@ 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, >inputcrop);
> + image_delete(input);
> + input = cropped;
> + }
> +
>   /* Scale */
>   if (options->output_width && options->output_height) {
>   output_width = options->output_width;
> @@ -1596,6 +1639,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)/WxHCrop the input image\n");

The alignment is incorrect.

>   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");
> @@ -1628,11 +1672,13 @@ static void list_formats(void)
> 
>  #define OPT_HFLIP256
>  #define OPT_VFLIP257
> +#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'},
> @@ -1649,6 +1695,84 @@ static struct option opts[] = {
>   {0, 0, 0, 0}
>  };
> 
> +static int parse_crop(struct image_rect *crop, char *optarg, char **endp)

The optarg argument should be made const. I would also rename it to value as 
it might not come from an option.

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

How about "Invalid crop format, expected '('\n" to clearly indicate what's 
wrong ?

> + *endp = endptr;
> + return 1;
> + }
> +
> + crop->left = strtol(endptr + 1, , 10);
> + if (*endptr != ',') {
> + printf("Invalid crop position\n");
> + *endp = endptr;
> + return 1;
> + }
> +
> + if (crop->left < 0) {
> + printf("Invalid negative crop\n");
> + *endp = endptr - 1;
> + return 1;
> + }
> +
> + crop->top = strtol(endptr + 1, , 10);
> + if (*endptr != ')') {
> + printf("Invalid crop position\n");
> + *endp = endptr;
> + return 1;
> + }
> +
> + if (crop->top 

[PATCH v2 2/4] gen-image: Implement option to parse an input crop

[Kieran Bingham]

From: Kieran Bingham 

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

Signed-off-by: Kieran Bingham 
---
 src/gen-image.c | 132 +-
 1 file changed, 132 insertions(+)

diff --git a/src/gen-image.c b/src/gen-image.c
index 31d42e0211db..088e8b26f648 100644
--- a/src/gen-image.c
+++ b/src/gen-image.c
@@ -95,6 +95,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;
@@ -127,6 +134,8 @@ struct options {
bool rotate;
unsigned int compose;
struct params params;
+   bool crop;
+   struct image_rect inputcrop;
 };
 
 /* 
-
@@ -1076,6 +1085,25 @@ 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)
+{
+   unsigned int offset = (crop->top * input->width + crop->left) * 3;
+   const uint8_t *idata = input->data + offset;
+   uint8_t *odata = output->data;
+   unsigned int y;
+
+   for (y = 0; y < output->height; ++y) {
+   memcpy(odata, idata, output->width * 3);
+   odata += output->width * 3;
+   idata += input->width * 3;
+   }
+}
+
+/* 
-
  * Look Up Table
  */
 
@@ -1363,6 +1391,21 @@ 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, >inputcrop);
+   image_delete(input);
+   input = cropped;
+   }
+
/* Scale */
if (options->output_width && options->output_height) {
output_width = options->output_width;
@@ -1596,6 +1639,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)/WxHCrop 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");
@@ -1628,11 +1672,13 @@ static void list_formats(void)
 
 #define OPT_HFLIP  256
 #define OPT_VFLIP  257
+#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'},
@@ -1649,6 +1695,84 @@ static struct option opts[] = {
{0, 0, 0, 0}
 };
 
+static int parse_crop(struct image_rect *crop, char *optarg, char **endp)
+{
+   /* (X,Y)/WxH */
+   char *endptr = optarg;
+
+   if (*endptr != '(') {
+   printf("Invalid crop argument\n");
+   *endp = endptr;
+   return 1;
+   }
+
+   crop->left = strtol(endptr + 1, , 10);
+   if (*endptr != ',') {
+   printf("Invalid crop position\n");
+   *endp = endptr;
+   return 1;
+   }
+
+   if (crop->left < 0) {
+   printf("Invalid negative crop\n");
+   *endp = endptr - 1;
+   return 1;
+   }
+
+   crop->top = strtol(endptr + 1, , 10);
+   if (*endptr != ')') {
+   printf("Invalid crop position\n");
+   *endp = endptr;
+   return 1;
+   }
+
+   if (crop->top < 0) {
+   printf("Invalid negative crop\n");
+   *endp = endptr - 1;
+   return 1;
+   }
+
+   endptr++;
+
+   if (*endptr != '/') {
+   printf("Invalid crop argument\n");
+   *endp = endptr;
+   return 1;
+   }
+
+   crop->width = strtol(endptr + 1, , 10);
+   if (*endptr != 'x') {
+   printf("Invalid crop