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

[Laurent Pinchart]

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 ?

> > 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