[libcamera-devel] [PATCH 5/7] libcamera: ipu3: Always report crop region

[Jacopo Mondi]

Hi Laurent, Niklas,

On Fri, Feb 05, 2021 at 12:19:46AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 04, 2021 at 10:48:46AM +0100, Jacopo Mondi wrote:
> > On Thu, Feb 04, 2021 at 10:40:55AM +0100, Niklas Söderlund wrote:
> > > On 2021-02-03 17:25:58 +0100, Jacopo Mondi wrote:
> > > > Report the crop region for every completed request.
> > > >
> > > > The crop region is initialized as the sensor's analogue crop
> > > > rectangle and updated when a Request with a new region completes.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > For my understanding. Before this patch the controls::ScalerCrop is
> > > currently only carried as is from the Request controls to the Request
> > > metadata. And after this change the rectangle is cached and reported in
> > > every Request's metadata while being updated if set in a Requests
> > > controls?
> >
> > Correct
> >
> > > And before and after this patch this is just "informative" as the IPU3
> > > pipeline does not yet react and reflects the setting to the hardware?
> >
> > And correct too
> >
> > > I think this patch is a good step in the right direction, the question
> > > above is just to understand the bigger picture.
> >
> > I mainly added this as android requires the scaler crop region
> > reported for every completed request, but I think it the right think
> > to do for libcamera as well, but... we need a policy in my opinion.
>
> I agree with you, I think this is the right behaviour, and it definitely
> should be documented. Maybe a quick patch to add a \todo comment could
> be added to this series ?
>
> > These days I think that having a clear distinction between controls
> > and metadata in our controls definition would have helped in
> > documenting things like "this metadata has to be repotred for each
> > completed requests" and such
>
> I don't like much how Android duplicates the definitions in the
> documentation they generate, but I agree with you we could do better.
> Maybe we could add request and metadata elements in the YAML file to
> document the behaviour specific to how a control is used in a request
> and in metadata ?

I was thinking the same.. we could have a section in the controls
documentation to specify specificities of the behaviour when the
control is used in a request or in metadata.

I would not also mind a "Pipeline implementation details" section,
have you seen anywhere something similar ? :D

In the meantime, this series is fully reviewed, so I'll merge as it is
if that's ok.

>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----
> > > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index db0d6b91be70..a9c8e180d1c4 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -65,6 +65,7 @@ public:
> > > >  	Stream rawStream_;
> > > >
> > > >  	uint32_t exposureTime_;
> > > > +	Rectangle cropRegion;
>
> s/cropRegion/cropRegion_/
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > >  };
> > > >
> > > >  class IPU3CameraConfiguration : public CameraConfiguration
> > > > @@ -472,6 +473,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > > +	CameraSensorInfo sensorInfo;
> > > > +	cio2->sensor()->sensorInfo(&sensorInfo);
> > > > +	data->cropRegion = sensorInfo.analogCrop;
> > > > +
> > > >  	/*
> > > >  	 * If the ImgU gets configured, its driver seems to expect that
> > > >  	 * buffers will be queued to its outputs, as otherwise the next
> > > > @@ -967,10 +972,10 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> > > >  	/* \todo Move the ExposureTime control to the IPA. */
> > > >  	request->metadata().set(controls::ExposureTime, exposureTime_);
> > > >  	/* \todo Actually apply the scaler crop region to the ImgU. */
> > > > -	if (request->controls().contains(controls::ScalerCrop)) {
> > > > -		Rectangle cropRegion = request->controls().get(controls::ScalerCrop);
> > > > -		request->metadata().set(controls::ScalerCrop, cropRegion);
> > > > -	}
> > > > +	if (request->controls().contains(controls::ScalerCrop))
> > > > +		cropRegion = request->controls().get(controls::ScalerCrop);
> > > > +	request->metadata().set(controls::ScalerCrop, cropRegion);
> > > > +
> > > >  	pipe_->completeRequest(request);
> > > >  }
> > > >
>
> --
> Regards,
>
> Laurent Pinchart