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

[Kieran Bingham]

Hi Laurent,

Thanks for the review,

On 10/02/17 08:19, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
>> From: Kieran Bingham 
>>
>> Allow the user to specify an input crop in the form (X,Y)/WxH
>>
>> Signed-off-by: Kieran Bingham 
>> ---
>>  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, >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)/WxHCrop the input image\n");
>>  

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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 08 Feb 2017 14:03:58 Kieran Bingham wrote:
> From: Kieran Bingham 
> 
> Allow the user to specify an input crop in the form (X,Y)/WxH
> 
> Signed-off-by: Kieran Bingham 
> ---
>  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, >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)/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");
> @@ -1813,11 +1859,13 @@ static void list_formats(void)
>  #define OPT_VFLIP257
>  #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 = 

[PATCH 3/5] 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 | 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, >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)/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");
@@ -1813,11 +1859,13 @@ static void list_formats(void)
 #define OPT_VFLIP  257
 #define OPT_HISTOGRAM_TYPE 258
 #define OPT_HISTOGRAM_AREAS259
+#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, , 10);
+   if (*endptr != ',' || endptr == optarg) {
+   printf("Invalid crop position '%s', expected ',', got '%c'\n", 
optarg, *endptr);
+   return 1;
+   }
+
+   options->inputcrop.top = strtol(endptr + 1, , 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, , 10);
+   if (*endptr != 'x' || endptr == optarg) {
+   printf("Invalid crop