From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 3/5] gen-image: Implement option to parse an input crop
Date: Mon, 13 Feb 2017 21:48:18 +0200 [thread overview]
Message-ID: <9323761.eby1YSBEA7@avalon> (raw)
In-Reply-To: <aa9aa0d5-ef74-4ae7-a834-b0648642d9b6@ideasonboard.com>
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
next prev parent reply other threads:[~2017-02-13 19:48 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9323761.eby1YSBEA7@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=linux-renesas-soc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.