[libcamera-devel] [PATCH] libcamera: camera_sensor: Match V4L2 specification

[Jacopo Mondi]

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 at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund