[libcamera-devel,5/6] libcamera: pipeline: raspberrypi: Implementation of digital zoom
diff mbox series

Message ID 20200922100400.30766-6-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Digital zoom
Related show

Commit Message

David Plowman Sept. 22, 2020, 10:03 a.m. UTC 
During configure() we update the SensorOutputSize to the correct value
for this camera mode (which we also note internally) and work out the
minimum crop size allowed by the ISP.

Whenever a new IspCrop request is received we check it's valid and
apply it to the ISP V4L2 device. We also forward it to the IPA so that
the IPA can return the values used in the image metadata.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           |  7 +++
 .../pipeline/raspberrypi/raspberrypi.cpp      | 47 +++++++++++++++++++
 3 files changed, 55 insertions(+)

Comments

Jacopo Mondi Sept. 23, 2020, 9:51 a.m. UTC | #1 
Hi David,

On Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:
> During configure() we update the SensorOutputSize to the correct value
> for this camera mode (which we also note internally) and work out the
> minimum crop size allowed by the ISP.
>
> Whenever a new IspCrop request is received we check it's valid and
> apply it to the ISP V4L2 device. We also forward it to the IPA so that
> the IPA can return the values used in the image metadata.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           |  7 +++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 47 +++++++++++++++++++
>  3 files changed, 55 insertions(+)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index dd6ebea..91a1892 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {
>  	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
>  	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> +	{ &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },

If you feel it's more convenient, you can
s/Rectangle(0, 0, 0, 0)/{}

whatever you like the most

>  };
>
>  } /* namespace libcamera */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0555cc4..64f0e35 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)
>  			break;
>  		}
>
> +		case controls::ISP_CROP: {
> +			/* Just copy the information back. */
> +			Rectangle crop = ctrl.second.get<Rectangle>();
> +			libcameraMetadata_.set(controls::IspCrop, crop);
> +			break;
> +		}
> +

I wonder if this needs to go to the IPA at all.. I like the symmetry with
other controls but the IPA is actually not involved at all in the process...
Do you expect this to change ? Otherwise the metadata can be updated here
with the value of lastIspCrop_ ?

>  		default:
>  			LOG(IPARPI, Warning)
>  				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 50f0718..1674f8f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -182,6 +182,11 @@ public:
>  	std::queue<FrameBuffer *> embeddedQueue_;
>  	std::deque<Request *> requestQueue_;
>
> +	/* For handling digital zoom. */
> +	Size ispMinSize_;
> +	Size sensorOutputSize_;
> +	Rectangle lastIspCrop_;
> +
>  	unsigned int dropFrameCount_;
>
>  private:
> @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	LOG(RPI, Info) << "Sensor: " << camera->id()
>  		       << " - Selected mode: " << sensorFormat.toString();
>
> +	/*
> +	 * Update the SensorOutputSize to the correct value for this camera mode.
> +	 *
> +	 * \todo Move this property to CameraConfiguration when that
> +	 * feature will be made available and set it at validate() time
> +	 */
> +	data->properties_.set(properties::SensorOutputSize, sensorFormat.size);

The next instruction is to apply 'sensorFormat' to the ISP input node.
I don't know if this operation is expected to change the format, but I
would however update the property value after that.

> +
>  	/*
>  	 * This format may be reset on start() if the bayer order has changed
>  	 * because of flips in the sensor.
> @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>
> +	/*
> +	 * Figure out the smallest selection the ISP will allow. We also store
> +	 * the output image size, in pixels, from the sensor. These will be
> +	 * used for digital zoom.
> +	 */
> +	Rectangle testCrop(0, 0, 1, 1);
> +	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> +	data->ispMinSize_ = testCrop.size();
> +	data->sensorOutputSize_ = sensorFormat.size;
> +
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Rectangle crop{ 0, 0, sensorFormat.size };
>
> @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	crop.y = (sensorFormat.size.height - crop.height) >> 1;
>  	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
>
> +	data->lastIspCrop_ = crop;
> +
>  	ret = data->configureIPA();
>  	if (ret)
>  		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()
>  	/* Take the first request from the queue and action the IPA. */
>  	Request *request = requestQueue_.front();
>
> +	if (request->controls().contains(controls::IspCrop)) {
> +		Rectangle crop = request->controls().get<Rectangle>(controls::IspCrop);
> +		if (crop.width && crop.height) {
> +			/*
> +			 * The crop that we set must be:
> +			 * 1. At least as big as ispMinSize_, once that's been
> +			 *    enlarged to the same aspect ratio.
> +			 * 2. With the same mid-point, if possible.
> +			 * 3. But it can't go outside the sensor area.
> +			 */
> +			Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
> +			Size size = crop.size().expandedTo(minSize);
> +			crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));

I might again be missing something, but isn't this equal to:
                        crop.size().expandTo(minSize).boundTo({sensorOutputSize_});

I'm missing the need to centering, 'crop' has already it's own offsets
set, so we just need to make sure it's larger that the minSize and
smaller than the sensor output size.

General question: how do we expect applications to reset crop ? By
setting it to the sensorOutputSize (or at any larger value). Do we
want to predispose a special case, like a {0, 0, 0, 0} crop rectangle
resets the crop ?

> +
> +			if (crop != lastIspCrop_)
> +				isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> +			lastIspCrop_ = crop;
> +		}
> +
> +		request->controls().set(controls::IspCrop, lastIspCrop_);
> +	}
> +

Very nice, I think as soon we can test this easily with qcam we'll see
interesting results!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  	/*
>  	 * Process all the user controls by the IPA. Once this is complete, we
>  	 * queue the ISP output buffer listed in the request to start the HW
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
David Plowman Sept. 23, 2020, 1:29 p.m. UTC | #2 
Hi Jacopo

Thanks as always for the detailed review. Let me just go through a
couple of those points...!

On Wed, 23 Sep 2020 at 10:47, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David,
>
> On Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:
> > During configure() we update the SensorOutputSize to the correct value
> > for this camera mode (which we also note internally) and work out the
> > minimum crop size allowed by the ISP.
> >
> > Whenever a new IspCrop request is received we check it's valid and
> > apply it to the ISP V4L2 device. We also forward it to the IPA so that
> > the IPA can return the values used in the image metadata.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  7 +++
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 47 +++++++++++++++++++
> >  3 files changed, 55 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index dd6ebea..91a1892 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {
> >       { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > +     { &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },
>
> If you feel it's more convenient, you can
> s/Rectangle(0, 0, 0, 0)/{}
>
> whatever you like the most

Yes, thanks, that looks nicer!

>
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0555cc4..64f0e35 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)
> >                       break;
> >               }
> >
> > +             case controls::ISP_CROP: {
> > +                     /* Just copy the information back. */
> > +                     Rectangle crop = ctrl.second.get<Rectangle>();
> > +                     libcameraMetadata_.set(controls::IspCrop, crop);
> > +                     break;
> > +             }
> > +
>
> I wonder if this needs to go to the IPA at all.. I like the symmetry with
> other controls but the IPA is actually not involved at all in the process...
> Do you expect this to change ? Otherwise the metadata can be updated here
> with the value of lastIspCrop_ ?

I remember thinking about this at the time. I couldn't actually think
of a concrete reason why an IPA would want to know the crop, but I
always had the impression that it could possibly happen one day. For
example, might AWB ever want to give more importance to parts of the
image in the final result? Or might ALSC work less hard on parts of
the image outside the final crop? The current answer to all these is
no, and I have no plans to change anything, but maybe that helps
convey why it's not such a strange thing to do.

So I'm inclined to leave this as it is unless there's still a strong
feeling to the contrary (in which case, please say!). Also, as you
point out, it's nice to treat everything the same.

>
> >               default:
> >                       LOG(IPARPI, Warning)
> >                               << "Ctrl " << controls::controls.at(ctrl.first)->name()
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 50f0718..1674f8f 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -182,6 +182,11 @@ public:
> >       std::queue<FrameBuffer *> embeddedQueue_;
> >       std::deque<Request *> requestQueue_;
> >
> > +     /* For handling digital zoom. */
> > +     Size ispMinSize_;
> > +     Size sensorOutputSize_;
> > +     Rectangle lastIspCrop_;
> > +
> >       unsigned int dropFrameCount_;
> >
> >  private:
> > @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       LOG(RPI, Info) << "Sensor: " << camera->id()
> >                      << " - Selected mode: " << sensorFormat.toString();
> >
> > +     /*
> > +      * Update the SensorOutputSize to the correct value for this camera mode.
> > +      *
> > +      * \todo Move this property to CameraConfiguration when that
> > +      * feature will be made available and set it at validate() time
> > +      */
> > +     data->properties_.set(properties::SensorOutputSize, sensorFormat.size);
>
> The next instruction is to apply 'sensorFormat' to the ISP input node.
> I don't know if this operation is expected to change the format, but I
> would however update the property value after that.

Indeed, I don't think it would change but it makes sense to move that
little block to just after.

>
> > +
> >       /*
> >        * This format may be reset on start() if the bayer order has changed
> >        * because of flips in the sensor.
> > @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >               return ret;
> >       }
> >
> > +     /*
> > +      * Figure out the smallest selection the ISP will allow. We also store
> > +      * the output image size, in pixels, from the sensor. These will be
> > +      * used for digital zoom.
> > +      */
> > +     Rectangle testCrop(0, 0, 1, 1);
> > +     data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> > +     data->ispMinSize_ = testCrop.size();
> > +     data->sensorOutputSize_ = sensorFormat.size;
> > +
> >       /* Adjust aspect ratio by providing crops on the input image. */
> >       Rectangle crop{ 0, 0, sensorFormat.size };
> >
> > @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       crop.y = (sensorFormat.size.height - crop.height) >> 1;
> >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> >
> > +     data->lastIspCrop_ = crop;
> > +
> >       ret = data->configureIPA();
> >       if (ret)
> >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()
> >       /* Take the first request from the queue and action the IPA. */
> >       Request *request = requestQueue_.front();
> >
> > +     if (request->controls().contains(controls::IspCrop)) {
> > +             Rectangle crop = request->controls().get<Rectangle>(controls::IspCrop);
> > +             if (crop.width && crop.height) {
> > +                     /*
> > +                      * The crop that we set must be:
> > +                      * 1. At least as big as ispMinSize_, once that's been
> > +                      *    enlarged to the same aspect ratio.
> > +                      * 2. With the same mid-point, if possible.
> > +                      * 3. But it can't go outside the sensor area.
> > +                      */
> > +                     Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
> > +                     Size size = crop.size().expandedTo(minSize);
> > +                     crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));
>
> I might again be missing something, but isn't this equal to:
>                         crop.size().expandTo(minSize).boundTo({sensorOutputSize_});
>
> I'm missing the need to centering, 'crop' has already it's own offsets
> set, so we just need to make sure it's larger that the minSize and
> smaller than the sensor output size.

So I think there are some subtle differences.

crop.size().expandTo(minSize).boundTo({sensorOutputSize_}) has the
following behaviours (if I've understood it correctly):

* I assumed this code is trying just to alter only the size of the
crop Rectangle? Rectangle::size() returns a temporary so I interpreted
the code as something like this:

Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
Size size = crop.size().expandedTo(minSize).boundedTo({sensorOutputSize_});
crop = Rectangle(crop.x, crop.y, size);

* Size::boundedTo() doesn't see the crop offsets, so the final crop
Rectangle might go outside sensorOutputSize_.

* changing the size of a Rectangle but not its offsets will cause its
centre-point to drift, and with digital zoom I think you'd expect the
centre-point of your window to remain the same.

I think if you fix these things up you'd have the same code as me.

>
> General question: how do we expect applications to reset crop ? By
> setting it to the sensorOutputSize (or at any larger value). Do we
> want to predispose a special case, like a {0, 0, 0, 0} crop rectangle
> resets the crop ?

Good point, and there's a slight philosophical problem in that you
don't know what the pipeline handler's default behaviour is. I suppose
one answer is that the application would have to do something like

sensorOutputSize.alignedDownToAspectRatio(streamConfig.size).centredTo({sensorOutputSize})

(love the long names! but note how you do need to know what aspect
ratio you want, as there's no guarantee that the SensorOutputSize is
not different)

This all seems a bit long-winded, but then if an application is
touching this stuff it could presumably just set the pan to 0,0 and
the zoom to 1 and call its own pan/zoom function.

At the moment I think I just ignore requests with zero width/height
but yes, we could use them to "reset" the crop. I don't feel strongly
on the matter. I suppose weird things might happen if a pipeline
handler didn't do the "obvious thing" by default (i.e. get the right
aspect ratio and centre it), but it doesn't sound like a mainstream
problem (and we could always mandate the behaviour if we don't
already).

But anyway, I'm not entirely sure about all of this, so would be
interested to hear further discussion.

I'll make all those other non-controversial changes in the meantime. Thanks!

Best regards
David

>
> > +
> > +                     if (crop != lastIspCrop_)
> > +                             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > +                     lastIspCrop_ = crop;
> > +             }
> > +
> > +             request->controls().set(controls::IspCrop, lastIspCrop_);
> > +     }
> > +
>
> Very nice, I think as soon we can test this easily with qcam we'll see
> interesting results!
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
> >       /*
> >        * Process all the user controls by the IPA. Once this is complete, we
> >        * queue the ISP output buffer listed in the request to start the HW
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 23, 2020, 5:12 p.m. UTC | #3 
Hi David,

On Wed, Sep 23, 2020 at 02:29:05PM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks as always for the detailed review. Let me just go through a
> couple of those points...!
>
> On Wed, 23 Sep 2020 at 10:47, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David,
> >
> > On Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:
> > > During configure() we update the SensorOutputSize to the correct value
> > > for this camera mode (which we also note internally) and work out the
> > > minimum crop size allowed by the ISP.
> > >
> > > Whenever a new IspCrop request is received we check it's valid and
> > > apply it to the ISP V4L2 device. We also forward it to the IPA so that
> > > the IPA can return the values used in the image metadata.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h           |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp           |  7 +++
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 47 +++++++++++++++++++
> > >  3 files changed, 55 insertions(+)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > index dd6ebea..91a1892 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {
> > >       { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > >       { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > > +     { &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },
> >
> > If you feel it's more convenient, you can
> > s/Rectangle(0, 0, 0, 0)/{}
> >
> > whatever you like the most
>
> Yes, thanks, that looks nicer!
>
> >
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 0555cc4..64f0e35 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)
> > >                       break;
> > >               }
> > >
> > > +             case controls::ISP_CROP: {
> > > +                     /* Just copy the information back. */
> > > +                     Rectangle crop = ctrl.second.get<Rectangle>();
> > > +                     libcameraMetadata_.set(controls::IspCrop, crop);
> > > +                     break;
> > > +             }
> > > +
> >
> > I wonder if this needs to go to the IPA at all.. I like the symmetry with
> > other controls but the IPA is actually not involved at all in the process...
> > Do you expect this to change ? Otherwise the metadata can be updated here
> > with the value of lastIspCrop_ ?
>
> I remember thinking about this at the time. I couldn't actually think
> of a concrete reason why an IPA would want to know the crop, but I
> always had the impression that it could possibly happen one day. For
> example, might AWB ever want to give more importance to parts of the
> image in the final result? Or might ALSC work less hard on parts of
> the image outside the final crop? The current answer to all these is
> no, and I have no plans to change anything, but maybe that helps
> convey why it's not such a strange thing to do.
>
> So I'm inclined to leave this as it is unless there's still a strong
> feeling to the contrary (in which case, please say!). Also, as you
> point out, it's nice to treat everything the same.
>

It's fine, there's no real overhead in sending it to the IPA in rpi
case, so I guess it's fine for now

> >
> > >               default:
> > >                       LOG(IPARPI, Warning)
> > >                               << "Ctrl " << controls::controls.at(ctrl.first)->name()
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 50f0718..1674f8f 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -182,6 +182,11 @@ public:
> > >       std::queue<FrameBuffer *> embeddedQueue_;
> > >       std::deque<Request *> requestQueue_;
> > >
> > > +     /* For handling digital zoom. */
> > > +     Size ispMinSize_;
> > > +     Size sensorOutputSize_;
> > > +     Rectangle lastIspCrop_;
> > > +
> > >       unsigned int dropFrameCount_;
> > >
> > >  private:
> > > @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >       LOG(RPI, Info) << "Sensor: " << camera->id()
> > >                      << " - Selected mode: " << sensorFormat.toString();
> > >
> > > +     /*
> > > +      * Update the SensorOutputSize to the correct value for this camera mode.
> > > +      *
> > > +      * \todo Move this property to CameraConfiguration when that
> > > +      * feature will be made available and set it at validate() time
> > > +      */
> > > +     data->properties_.set(properties::SensorOutputSize, sensorFormat.size);
> >
> > The next instruction is to apply 'sensorFormat' to the ISP input node.
> > I don't know if this operation is expected to change the format, but I
> > would however update the property value after that.
>
> Indeed, I don't think it would change but it makes sense to move that
> little block to just after.
>
> >
> > > +
> > >       /*
> > >        * This format may be reset on start() if the bayer order has changed
> > >        * because of flips in the sensor.
> > > @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >               return ret;
> > >       }
> > >
> > > +     /*
> > > +      * Figure out the smallest selection the ISP will allow. We also store
> > > +      * the output image size, in pixels, from the sensor. These will be
> > > +      * used for digital zoom.
> > > +      */
> > > +     Rectangle testCrop(0, 0, 1, 1);
> > > +     data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> > > +     data->ispMinSize_ = testCrop.size();
> > > +     data->sensorOutputSize_ = sensorFormat.size;
> > > +
> > >       /* Adjust aspect ratio by providing crops on the input image. */
> > >       Rectangle crop{ 0, 0, sensorFormat.size };
> > >
> > > @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >       crop.y = (sensorFormat.size.height - crop.height) >> 1;
> > >       data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > >
> > > +     data->lastIspCrop_ = crop;
> > > +
> > >       ret = data->configureIPA();
> > >       if (ret)
> > >               LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
> > > @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()
> > >       /* Take the first request from the queue and action the IPA. */
> > >       Request *request = requestQueue_.front();
> > >
> > > +     if (request->controls().contains(controls::IspCrop)) {
> > > +             Rectangle crop = request->controls().get<Rectangle>(controls::IspCrop);
> > > +             if (crop.width && crop.height) {
> > > +                     /*
> > > +                      * The crop that we set must be:
> > > +                      * 1. At least as big as ispMinSize_, once that's been
> > > +                      *    enlarged to the same aspect ratio.
> > > +                      * 2. With the same mid-point, if possible.
> > > +                      * 3. But it can't go outside the sensor area.
> > > +                      */
> > > +                     Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
> > > +                     Size size = crop.size().expandedTo(minSize);
> > > +                     crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));
> >
> > I might again be missing something, but isn't this equal to:
> >                         crop.size().expandTo(minSize).boundTo({sensorOutputSize_});
> >
> > I'm missing the need to centering, 'crop' has already it's own offsets
> > set, so we just need to make sure it's larger that the minSize and
> > smaller than the sensor output size.
>
> So I think there are some subtle differences.
>
> crop.size().expandTo(minSize).boundTo({sensorOutputSize_}) has the
> following behaviours (if I've understood it correctly):
>
> * I assumed this code is trying just to alter only the size of the
> crop Rectangle? Rectangle::size() returns a temporary so I interpreted
> the code as something like this:
>
> Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
> Size size = crop.size().expandedTo(minSize).boundedTo({sensorOutputSize_});
> crop = Rectangle(crop.x, crop.y, size);
>
> * Size::boundedTo() doesn't see the crop offsets, so the final crop
> Rectangle might go outside sensorOutputSize_.

Yes, it's a Size, not a Rectangle!

>
> * changing the size of a Rectangle but not its offsets will cause its
> centre-point to drift, and with digital zoom I think you'd expect the
> centre-point of your window to remain the same.
>
> I think if you fix these things up you'd have the same code as me.

Indeed! thanks for confirming this!

>
> >
> > General question: how do we expect applications to reset crop ? By
> > setting it to the sensorOutputSize (or at any larger value). Do we
> > want to predispose a special case, like a {0, 0, 0, 0} crop rectangle
> > resets the crop ?
>
> Good point, and there's a slight philosophical problem in that you
> don't know what the pipeline handler's default behaviour is. I suppose

Well, we can establish that in the Control definition and pipeline
will have to follow...

> one answer is that the application would have to do something like
>
> sensorOutputSize.alignedDownToAspectRatio(streamConfig.size).centredTo({sensorOutputSize})
>
> (love the long names! but note how you do need to know what aspect
> ratio you want, as there's no guarantee that the SensorOutputSize is
> not different)
>
> This all seems a bit long-winded, but then if an application is
> touching this stuff it could presumably just set the pan to 0,0 and
> the zoom to 1 and call its own pan/zoom function.
>
> At the moment I think I just ignore requests with zero width/height
> but yes, we could use them to "reset" the crop. I don't feel strongly
> on the matter. I suppose weird things might happen if a pipeline
> handler didn't do the "obvious thing" by default (i.e. get the right
> aspect ratio and centre it), but it doesn't sound like a mainstream
> problem (and we could always mandate the behaviour if we don't
> already).
>
> But anyway, I'm not entirely sure about all of this, so would be
> interested to hear further discussion.

I don't have a firm answer to this either.. I'm not concerned of
pipeline handler mis-beahving, as if they do so, it means they're not
compliant with out specs and shall be fixed... But as you say, an
application which goes to the extent of being able to implement
pan/zoom will probably like to handle it by itself... Anyway, that's
not related to this patch.. Maybe it's worth another \todo entry in
the control definition?

Thanks
  j

>
> I'll make all those other non-controversial changes in the meantime. Thanks!
>
> Best regards
> David
>
> >
> > > +
> > > +                     if (crop != lastIspCrop_)
> > > +                             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
> > > +                     lastIspCrop_ = crop;
> > > +             }
> > > +
> > > +             request->controls().set(controls::IspCrop, lastIspCrop_);
> > > +     }
> > > +
> >
> > Very nice, I think as soon we can test this easily with qcam we'll see
> > interesting results!
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Thanks
> >    j
> >
> > >       /*
> > >        * Process all the user controls by the IPA. Once this is complete, we
> > >        * queue the ISP output buffer listed in the request to start the HW
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index dd6ebea..91a1892 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -58,6 +58,7 @@  static const ControlInfoMap RPiControls = {
 	{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
 	{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
+	{ &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },
 };
 
 } /* namespace libcamera */
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 0555cc4..64f0e35 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -687,6 +687,13 @@  void IPARPi::queueRequest(const ControlList &controls)
 			break;
 		}
 
+		case controls::ISP_CROP: {
+			/* Just copy the information back. */
+			Rectangle crop = ctrl.second.get<Rectangle>();
+			libcameraMetadata_.set(controls::IspCrop, crop);
+			break;
+		}
+
 		default:
 			LOG(IPARPI, Warning)
 				<< "Ctrl " << controls::controls.at(ctrl.first)->name()
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 50f0718..1674f8f 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -182,6 +182,11 @@  public:
 	std::queue<FrameBuffer *> embeddedQueue_;
 	std::deque<Request *> requestQueue_;
 
+	/* For handling digital zoom. */
+	Size ispMinSize_;
+	Size sensorOutputSize_;
+	Rectangle lastIspCrop_;
+
 	unsigned int dropFrameCount_;
 
 private:
@@ -496,6 +501,14 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	LOG(RPI, Info) << "Sensor: " << camera->id()
 		       << " - Selected mode: " << sensorFormat.toString();
 
+	/*
+	 * Update the SensorOutputSize to the correct value for this camera mode.
+	 *
+	 * \todo Move this property to CameraConfiguration when that
+	 * feature will be made available and set it at validate() time
+	 */
+	data->properties_.set(properties::SensorOutputSize, sensorFormat.size);
+
 	/*
 	 * This format may be reset on start() if the bayer order has changed
 	 * because of flips in the sensor.
@@ -592,6 +605,16 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 		return ret;
 	}
 
+	/*
+	 * Figure out the smallest selection the ISP will allow. We also store
+	 * the output image size, in pixels, from the sensor. These will be
+	 * used for digital zoom.
+	 */
+	Rectangle testCrop(0, 0, 1, 1);
+	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
+	data->ispMinSize_ = testCrop.size();
+	data->sensorOutputSize_ = sensorFormat.size;
+
 	/* Adjust aspect ratio by providing crops on the input image. */
 	Rectangle crop{ 0, 0, sensorFormat.size };
 
@@ -608,6 +631,8 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	crop.y = (sensorFormat.size.height - crop.height) >> 1;
 	data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
 
+	data->lastIspCrop_ = crop;
+
 	ret = data->configureIPA();
 	if (ret)
 		LOG(RPI, Error) << "Failed to configure the IPA: " << ret;
@@ -1449,6 +1474,28 @@  void RPiCameraData::tryRunPipeline()
 	/* Take the first request from the queue and action the IPA. */
 	Request *request = requestQueue_.front();
 
+	if (request->controls().contains(controls::IspCrop)) {
+		Rectangle crop = request->controls().get<Rectangle>(controls::IspCrop);
+		if (crop.width && crop.height) {
+			/*
+			 * The crop that we set must be:
+			 * 1. At least as big as ispMinSize_, once that's been
+			 *    enlarged to the same aspect ratio.
+			 * 2. With the same mid-point, if possible.
+			 * 3. But it can't go outside the sensor area.
+			 */
+			Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());
+			Size size = crop.size().expandedTo(minSize);
+			crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));
+
+			if (crop != lastIspCrop_)
+				isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);
+			lastIspCrop_ = crop;
+		}
+
+		request->controls().set(controls::IspCrop, lastIspCrop_);
+	}
+
 	/*
 	 * Process all the user controls by the IPA. Once this is complete, we
 	 * queue the ISP output buffer listed in the request to start the HW