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.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? 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
