| Message ID | 20210318103941.18837-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for the patch, On 18/03/2021 11:39, Jacopo Mondi wrote: > From: Dave Olsthoorn <dave@bewaar.me> > > The value of IF_CROP_MAX seems to be a typo. A resolution of 40x540 seems > unlikely and excludes camera's with a 640x480 resolution, like the OV7251 > in several Microsoft Surface products, from working. > > This patch corrects the value to 40 since a minimal resolution of 40x40 > seems more logical. > > Signed-off-by: Dave Olsthoorn <dave@bewaar.me> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index b6a1fe34494e..dd7d2e947141 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -44,7 +44,7 @@ static constexpr unsigned int BDS_ALIGN_W = 2; > static constexpr unsigned int BDS_ALIGN_H = 4; > > static constexpr unsigned int IF_CROP_MAX_W = 40; > -static constexpr unsigned int IF_CROP_MAX_H = 540; > +static constexpr unsigned int IF_CROP_MAX_H = 40; > > static constexpr float BDS_SF_MAX = 2.5; > static constexpr float BDS_SF_MIN = 1.0; >
Hi Jacopo, Thank you for the patch. On Thu, Mar 18, 2021 at 11:39:41AM +0100, Jacopo Mondi wrote: > From: Dave Olsthoorn <dave@bewaar.me> > > The value of IF_CROP_MAX seems to be a typo. A resolution of 40x540 seems > unlikely and excludes camera's with a 640x480 resolution, like the OV7251 > in several Microsoft Surface products, from working. IF_CROP_MAX_H seems to be the maximum cropping that can be applied vertically, and is used to limit the search space. I'm not sure if that's a device limitation or a software limitation, but in any case, it's not a minimum vertical size as this commit message seems to imply, and I don't see why it would excluse a 640x480 input. Could the problem be instead caused by unsigned int minIFHeight = iif.height - IF_CROP_MAX_H; in calculateBDSHeight() that will be a very large positive value if iif.height < IF_CROP_MAX_H ? > This patch corrects the value to 40 since a minimal resolution of 40x40 > seems more logical. > > Signed-off-by: Dave Olsthoorn <dave@bewaar.me> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/imgu.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index b6a1fe34494e..dd7d2e947141 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -44,7 +44,7 @@ static constexpr unsigned int BDS_ALIGN_W = 2; > static constexpr unsigned int BDS_ALIGN_H = 4; > > static constexpr unsigned int IF_CROP_MAX_W = 40; > -static constexpr unsigned int IF_CROP_MAX_H = 540; > +static constexpr unsigned int IF_CROP_MAX_H = 40; > > static constexpr float BDS_SF_MAX = 2.5; > static constexpr float BDS_SF_MIN = 1.0;
Hi Laurent, On Tue, Apr 13, 2021 at 02:53:38AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Mar 18, 2021 at 11:39:41AM +0100, Jacopo Mondi wrote: > > From: Dave Olsthoorn <dave@bewaar.me> > > > > The value of IF_CROP_MAX seems to be a typo. A resolution of 40x540 seems > > unlikely and excludes camera's with a 640x480 resolution, like the OV7251 > > in several Microsoft Surface products, from working. > > IF_CROP_MAX_H seems to be the maximum cropping that can be applied > vertically, and is used to limit the search space. I'm not sure if > that's a device limitation or a software limitation, but in any case, > it's not a minimum vertical size as this commit message seems to imply, > and I don't see why it would excluse a 640x480 input. Indeed, the value does not reprent the min resolution... > > Could the problem be instead caused by > > unsigned int minIFHeight = iif.height - IF_CROP_MAX_H; > > in calculateBDSHeight() that will be a very large positive value if > iif.height < IF_CROP_MAX_H ? > I can add a check for that, unfortunately I cannot reproduce the problem and I cannot tell if that would fix it. Dave could you test a follow-up patch eventually ? Thanks j > > This patch corrects the value to 40 since a minimal resolution of 40x40 > > seems more logical. > > > > Signed-off-by: Dave Olsthoorn <dave@bewaar.me> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/imgu.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > > index b6a1fe34494e..dd7d2e947141 100644 > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > > @@ -44,7 +44,7 @@ static constexpr unsigned int BDS_ALIGN_W = 2; > > static constexpr unsigned int BDS_ALIGN_H = 4; > > > > static constexpr unsigned int IF_CROP_MAX_W = 40; > > -static constexpr unsigned int IF_CROP_MAX_H = 540; > > +static constexpr unsigned int IF_CROP_MAX_H = 40; > > > > static constexpr float BDS_SF_MAX = 2.5; > > static constexpr float BDS_SF_MIN = 1.0; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index b6a1fe34494e..dd7d2e947141 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -44,7 +44,7 @@ static constexpr unsigned int BDS_ALIGN_W = 2; static constexpr unsigned int BDS_ALIGN_H = 4; static constexpr unsigned int IF_CROP_MAX_W = 40; -static constexpr unsigned int IF_CROP_MAX_H = 540; +static constexpr unsigned int IF_CROP_MAX_H = 40; static constexpr float BDS_SF_MAX = 2.5; static constexpr float BDS_SF_MIN = 1.0;