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.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 :-) > > 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) > > >
