Hi Guennadi,
On Monday 27 Feb 2017 09:54:19 Guennadi Liakhovetski wrote:
> On Mon, 27 Feb 2017, Laurent Pinchart wrote:
> > On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote:
> >> From: Koji Matsuoka <[email protected]>
> >>
> >> update_subrect() adjusts the sub-rectangle to be inside a base area.
> >> It checks width and height to not exceed those of the area, then it
> >> checks the low border (left or top) to lie within the area, then the
> >> high border (right or bottom) to lie there too. This latter check has
> >> a bug, which is fixed by this patch.
> >>
> >> Signed-off-by: Koji Matsuoka <[email protected]>
> >> Signed-off-by: Yoshihiro Kaneko <[email protected]>
> >> [[email protected]: dropped supposedly wrong hunks]
> >> Signed-off-by: Guennadi Liakhovetski <[email protected]>
> >> ---
> >>
> >> This is a part of the https://patchwork.linuxtv.org/patch/26441/
> >> submitted almost 2.5 years ago. Back then I commented to the patch but
> >> never got a reply or an update. I preserved original authorship and Sob
> >> tags, although this version only uses a small portion of the original
> >> patch. This version is of course completely untested, any testing (at
> >> least regression) would be highly appreciated! This code is only used by
> >> the SH CEU driver and only in cropping / zooming scenarios.
> >>
> >> drivers/media/platform/soc_camera/soc_scale_crop.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> b/drivers/media/platform/soc_camera/soc_scale_crop.c index
> >> f77252d..4bfc1bf
> >> 100644
> >> --- a/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> +++ b/drivers/media/platform/soc_camera/soc_scale_crop.c
> >> @@ -70,14 +70,14 @@ static void update_subrect(struct v4l2_rect *rect,
> >> struct v4l2_rect *subrect)
> >> if (rect->height < subrect->height)
> >> subrect->height = rect->height;
> >>
> >> - if (rect->left > subrect->left)
> >> + if (rect->left < subrect->left)
> >
> > This looks wrong to me. If the purpose of the function is indeed to adjust
> > subrect to stay within rect, the condition doesn't need to be changed.
> >
> >> subrect->left = rect->left;
> >> else if (rect->left + rect->width >
> >> subrect->left + subrect->width)
> >
> > This condition, however, is wrong.
>
> Arrrrgh, of course, I meant to change this one! Thanks for catching.
>
> >> subrect->left = rect->left + rect->width -
> >> subrect->width;
> >
> > More than that, adjusting the width first and then the left coordinate can
> > result in an incorrect width.
>
> The width is adjusted in the beginning only to stay within the area, you
> cannot go beyond it anyway. So, that has to be done anyway. And then the
> origin is adjusted.
>
> > It looks to me like you should drop the width
> > check at the beginning of this function, and turn the "else if" here into
> > an "if" with the right condition. Or, even better in my opinion, use the
> > min/max/clamp macros.
>
> Well, that depends on what result we want to achieve, what parameter we
> prioritise. This approach prioritises width and height, and then adjusts
> edges to accommodate as much of them as possible. A different approach
> would be to prioritise the origin (top and left) and adjust width and
> height to stay within the area. Do we have a preference for this?
Don't you need both ? "Inside the area" is a pretty well-defined concept :-)
subrect->left = max(subrect->left, rect->left);
subrect->top = max(subrect->top, rect->top);
subrect->width = min(subrect->left + subrect->width,
rect->left + rect->width) - subrect->left;
subrect->height = min(subrect->top + subrect->height,
rect->top + rect->height) - subrect->top;
(Completely untested)
> > Same comments for the vertical checks.
> >
> >> - if (rect->top > subrect->top)
> >> + if (rect->top < subrect->top)
> >> subrect->top = rect->top;
> >> else if (rect->top + rect->height >
> >> subrect->top + subrect->height)
--
Regards,
Laurent Pinchart
