Hi Kieran,
Thank you for the patch.
On Friday 10 Feb 2017 13:30:06 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 | 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, &options->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)/WxH Crop 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_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)
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, &endptr, 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, &endptr, 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, &endptr, 10);
> + if (*endptr != 'x') {
> + printf("Invalid crop size\n");
> + *endp = endptr;
> + return 1;
> + }
> +
> + crop->height = strtol(endptr + 1, &endptr, 10);
> + if (*endptr != 0) {
> + printf("Invalid crop size\n");
> + *endp = endptr;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +void print_streampos(const char *p, const char *end)
This function should be static. I would rename it to parser_print_error(),
"streampos" not being very explicit.
> +{
> + int pos;
> +
> + pos = end - p + 1;
> +
> + if (pos < 0)
> + pos = 0;
> + if (pos > (int) strlen(p))
> + pos = strlen(p);
> +
> + printf("\n");
> + printf(" %s\n", p);
> + printf(" %*s\n", pos, "^");
> +}
> +
> static int parse_args(struct options *options, int argc, char *argv[])
> {
> char *endptr;
> @@ -1809,6 +1933,14 @@ static int parse_args(struct options *options, int
> argc, char *argv[]) options->vflip = true;
> break;
>
> + case OPT_CROP:
> + if (parse_crop(&options->inputcrop, optarg, &endptr))
{
> + print_streampos(optarg, endptr);
I would move this inside parse_crop().
I'll fix while applying, no need to resend.
> + return 1;
> + }
> + options->crop = true;
> + break;
> +
> default:
> printf("Invalid option -%c\n", c);
> printf("Run %s -h for help.\n", argv[0]);
--
Regards,
Laurent Pinchart
