| Message ID | Pine.LNX.4.64.1702262150090.17018@axis700.grange (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Guennadi, Thank you for the patch. On Sunday 26 Feb 2017 21:58:16 Guennadi Liakhovetski wrote: > From: Koji Matsuoka <koji.matsuoka.xm@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.xm@renesas.com> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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. > 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. 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. 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)
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@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.xm@renesas.com> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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? 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 >
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@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.xm@renesas.com> > >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > >> [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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 :-) 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)
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@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.xm@renesas.com> >>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> >>>> [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] >>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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. 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) >
On Mon, 27 Feb 2017, 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@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.xm@renesas.com> > > >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > >> [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > > >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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 :-) > > subrect->left = max(subrect->left, rect->left); I prefer to avoid assignments like "a = max(a, b)" to avoid a redundant "a = a" assignment, when a >= b :-) > 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; But this is exactly what I meant, isn't it? Consider an area 100..1000 and a subrect 200..2000. Obviously, width is wrong. You have two possibilities to adjust it: (1) size-priority. You maximise the size (900) and then adjust the origin to accommodate it (100). (2) origin-priority: you keep origin (200) and maximise size, based on that (800). My approach does (1), yours does (2). I prefer (1). Your approach also would break if origin is way too large, e.g. 1100..2000, whereas mine would work unchanged. Thanks Guennadi > (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 >
Hi Guennadi, On Monday 27 Feb 2017 10:13:53 Guennadi Liakhovetski wrote: > On Mon, 27 Feb 2017, Laurent Pinchart wrote: > > 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@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.xm@renesas.com> > >>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > >>>> [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > >>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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 > > :-) > > > > subrect->left = max(subrect->left, rect->left); > > I prefer to avoid assignments like "a = max(a, b)" to avoid a redundant > "a = a" assignment, when a >= b :-) The compiler should hopefully optimize that for you. > > 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; > > But this is exactly what I meant, isn't it? Consider an area 100..1000 and > a subrect 200..2000. Obviously, width is wrong. You have two > possibilities to adjust it: (1) size-priority. You maximise the size > (900) and then adjust the origin to accommodate it (100). (2) > origin-priority: you keep origin (200) and maximise size, based on that > (800). My approach does (1), yours does (2). I prefer (1). Your approach > also would break if origin is way too large, e.g. 1100..2000, whereas mine > would work unchanged. OK, I see what you mean now. It's "clamp to boundary" vs. "move within boundaries and clamp if necessary". I haven't look at the caller to see what is needed here. Whatever option you choose, it would make sense to rename the function accordingly (and possibly move it to the V4L2 core). > > (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)
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@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.xm@renesas.com> > >>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > >>>> [g.liakhovetski@gmx.de: dropped supposedly wrong hunks] > >>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@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) > > >
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) subrect->left = rect->left; else if (rect->left + rect->width > subrect->left + subrect->width) subrect->left = rect->left + rect->width - subrect->width; - if (rect->top > subrect->top) + if (rect->top < subrect->top) subrect->top = rect->top; else if (rect->top + rect->height > subrect->top + subrect->height)