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

[Jacopo Mondi]

Hi Niklas

On Thu, Feb 04, 2021 at 10:40:55AM +0100, Niklas Söderlund wrote:
> HI Jacopo,
>
> Thanks for your work.
>
> 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.

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

>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

Thanks
  j
>
> > ---
> >  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;
> >  };
> >
> >  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);
> >  }
> >
> > --
> > 2.30.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund