On Mon, 27 Feb 2017, Hans Verkuil wrote:
> On 02/27/2017 10:02 AM, Laurent Pinchart wrote:
> > 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 <koji.matsuoka...@renesas.com>
> >>>>
> >>>> 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 <koji.matsuoka...@renesas.com>
> >>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0...@gmail.com>
> >>>> [g.liakhovet...@gmx.de: dropped supposedly wrong hunks]
> >>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovet...@gmx.de>
> >>>> ---
> >>>>
> >>>> 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 :-)
>
> Generally the top-left is adjusted first, and then the width/height if it
> still
> can't be made to fit. I.e. the priority is to keep the width/height unchanged
> if possible.
Ok, sure, you can use either order, but if we prioritise width / height,
then the only restriction for them is to be <= original width / height,
right? So, you can always first do
if (subrect->width > rect->width)
subrect->width = rect->width;
right? That wqay you guarantee, that you can fit and that you keep as much
of the requested subrect width, as you can. And then you can adjust left /
top if still needed.
Thanks
Guennadi
>
> Regards,
>
> Hans
>
> >
> > 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)
> >
>
