Hi Laurent, On Mon, 27 Feb 2017, Laurent Pinchart wrote: > Hi Guennadi, > > Thank you for the patch. > > On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote: > > From: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx> > > > > 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.xm@xxxxxxxxxxx> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx> > > [g.liakhovetski@xxxxxx: dropped supposedly wrong hunks] > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> > > --- > > > > 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? Thanks Guennadi > > 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 >
