| Message ID | 20200805123004.18794-1-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-08-05 14:30:04 +0200, Jacopo Mondi wrote: > Adjust the top-left corner coordinates of the analogCrop rectangle > to match the libcamera and V4L2 specification. > > As per the V4L2 selection API specification, all rectangles accessed > using V4L2_SEL_TGT_* are defined relatively to the full pixel array, > which is retrieved using the V4L2_SEL_TGT_NATIVE target. Libcamera > defines the analogCrop rectangle to be defined relatively > to the active pixel matrix rectangle. > > To compensate the difference in the reference rectangle between the two > specification, subtract from the TGT_CROP rectangle top-left corner the > offset of the active pixel array previously obtained with > TGT_CROP_DEFAULT. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > > This patch depends on the inclusion of the following patch in the > Linux kernel: > [PATCH 4/4] media: i2c: imx219: Selection compliance fixes I think the patch looks fine, but this comment puzzles me. Do this change depend on the for mentioned patch for correct behavior of the controls or is a regression introduced due to a missing fix for imx219 ? In either case for this patch as long as it's not merged and introducing a regression, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera_sensor.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 350f49accad9..8f09206ff635 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -508,6 +508,18 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const > return ret; > } > > + /* > + * The top-left corner position of the selection rectangle returned by > + * the V4L2_SEL_TGT_CROP target is defined relatively to the full pixel > + * array, while libcamera's analogCrop is defined relatively to the > + * active portion of the pixel matrix. > + * > + * Subtract from the top-left coordinates the offsets of the > + * CROP_DEFAULT target rectangle to align the two. > + */ > + info->analogCrop.x -= rect.x; > + info->analogCrop.y -= rect.y; > + > /* The bit depth and image size depend on the currently applied format. */ > V4L2SubdeviceFormat format{}; > ret = subdev_->getFormat(pad_, &format); > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Wed, Aug 05, 2020 at 02:30:04PM +0200, Jacopo Mondi wrote: > Adjust the top-left corner coordinates of the analogCrop rectangle > to match the libcamera and V4L2 specification. > > As per the V4L2 selection API specification, all rectangles accessed > using V4L2_SEL_TGT_* are defined relatively to the full pixel array, > which is retrieved using the V4L2_SEL_TGT_NATIVE target. Libcamera > defines the analogCrop rectangle to be defined relatively > to the active pixel matrix rectangle. > > To compensate the difference in the reference rectangle between the two > specification, subtract from the TGT_CROP rectangle top-left corner the > offset of the active pixel array previously obtained with > TGT_CROP_DEFAULT. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > > This patch depends on the inclusion of the following patch in the > Linux kernel: > [PATCH 4/4] media: i2c: imx219: Selection compliance fixes Let's postpone this until the review discussions related to the above patch are complete. > --- > src/libcamera/camera_sensor.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 350f49accad9..8f09206ff635 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -508,6 +508,18 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const > return ret; > } > > + /* > + * The top-left corner position of the selection rectangle returned by > + * the V4L2_SEL_TGT_CROP target is defined relatively to the full pixel > + * array, while libcamera's analogCrop is defined relatively to the > + * active portion of the pixel matrix. > + * > + * Subtract from the top-left coordinates the offsets of the > + * CROP_DEFAULT target rectangle to align the two. > + */ > + info->analogCrop.x -= rect.x; > + info->analogCrop.y -= rect.y; > + > /* The bit depth and image size depend on the currently applied format. */ > V4L2SubdeviceFormat format{}; > ret = subdev_->getFormat(pad_, &format);
Hi Niklas, On Wed, Aug 05, 2020 at 11:17:50PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-08-05 14:30:04 +0200, Jacopo Mondi wrote: > > Adjust the top-left corner coordinates of the analogCrop rectangle > > to match the libcamera and V4L2 specification. > > > > As per the V4L2 selection API specification, all rectangles accessed > > using V4L2_SEL_TGT_* are defined relatively to the full pixel array, > > which is retrieved using the V4L2_SEL_TGT_NATIVE target. Libcamera > > defines the analogCrop rectangle to be defined relatively > > to the active pixel matrix rectangle. > > > > To compensate the difference in the reference rectangle between the two > > specification, subtract from the TGT_CROP rectangle top-left corner the > > offset of the active pixel array previously obtained with > > TGT_CROP_DEFAULT. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > > > This patch depends on the inclusion of the following patch in the > > Linux kernel: > > [PATCH 4/4] media: i2c: imx219: Selection compliance fixes > > I think the patch looks fine, but this comment puzzles me. Do this > change depend on the for mentioned patch for correct behavior of the > controls or is a regression introduced due to a missing fix for imx219 ? > > In either case for this patch as long as it's not merged and introducing > a regression, Let's say someone (me) has implemented upstream support for G_SELECTION which does not match the intended behaviour. Partly because that part is not specified at all in V4L2 :) libcamera assumes the current mainline behaviour, and the above mentioned kernel series makes imx219 comply with what is expected. I sent it out yesterday, the single patch itself is not controversial, the documentation part might take longer, but the imx219 change could be broke out. The moment that patch gets in, the libcamera side should be updated as well, and this patch does so. So yes, I sent it out, but it will have to stay out of tree for a while (5.10 or late fix for 5.9? I'll ping Hans tomorrow and ask). > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > src/libcamera/camera_sensor.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 350f49accad9..8f09206ff635 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -508,6 +508,18 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const > > return ret; > > } > > > > + /* > > + * The top-left corner position of the selection rectangle returned by > > + * the V4L2_SEL_TGT_CROP target is defined relatively to the full pixel > > + * array, while libcamera's analogCrop is defined relatively to the > > + * active portion of the pixel matrix. > > + * > > + * Subtract from the top-left coordinates the offsets of the > > + * CROP_DEFAULT target rectangle to align the two. > > + */ > > + info->analogCrop.x -= rect.x; > > + info->analogCrop.y -= rect.y; > > + > > /* The bit depth and image size depend on the currently applied format. */ > > V4L2SubdeviceFormat format{}; > > ret = subdev_->getFormat(pad_, &format); > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 350f49accad9..8f09206ff635 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -508,6 +508,18 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const return ret; } + /* + * The top-left corner position of the selection rectangle returned by + * the V4L2_SEL_TGT_CROP target is defined relatively to the full pixel + * array, while libcamera's analogCrop is defined relatively to the + * active portion of the pixel matrix. + * + * Subtract from the top-left coordinates the offsets of the + * CROP_DEFAULT target rectangle to align the two. + */ + info->analogCrop.x -= rect.x; + info->analogCrop.y -= rect.y; + /* The bit depth and image size depend on the currently applied format. */ V4L2SubdeviceFormat format{}; ret = subdev_->getFormat(pad_, &format);
Adjust the top-left corner coordinates of the analogCrop rectangle to match the libcamera and V4L2 specification. As per the V4L2 selection API specification, all rectangles accessed using V4L2_SEL_TGT_* are defined relatively to the full pixel array, which is retrieved using the V4L2_SEL_TGT_NATIVE target. Libcamera defines the analogCrop rectangle to be defined relatively to the active pixel matrix rectangle. To compensate the difference in the reference rectangle between the two specification, subtract from the TGT_CROP rectangle top-left corner the offset of the active pixel array previously obtained with TGT_CROP_DEFAULT. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- This patch depends on the inclusion of the following patch in the Linux kernel: [PATCH 4/4] media: i2c: imx219: Selection compliance fixes --- src/libcamera/camera_sensor.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) -- 2.27.0