| Message ID | 20210105190522.682324-10-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Jan 05, 2021 at 08:05:19PM +0100, Jacopo Mondi wrote: > Add support for caching the value of control::ScalerCrop to return > it in the request metadata. > > No cropping is currently applied on the input video device, the control > value is returned in the metadata pack as it is received. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f1329ffb0463..381524bb3499 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -66,6 +66,7 @@ public: > Stream rawStream_; > > int32_t exposureTime_; > + Rectangle scalerCrop_; > }; > > class IPU3CameraConfiguration : public CameraConfiguration > @@ -75,6 +76,7 @@ public: > > Status validate() override; > > + const Size &cio2Size() const { return cio2Configuration_.size; } Do we need this, can't we keep using config->cio2Format().size ? > const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > @@ -468,12 +470,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * Pass the requested stream size to the CIO2 unit and get back the > * adjusted format to be propagated to the ImgU output devices. > */ > - const Size &sensorSize = config->cio2Format().size; > V4L2DeviceFormat cio2Format; > - ret = cio2->configure(sensorSize, &cio2Format); > + ret = cio2->configure(config->cio2Size(), &cio2Format); > if (ret) > return ret; > > + /* Initialize the scaler crop using the sensor's analogue crop. */ > + CameraSensorInfo sensorInfo; > + ret = cio2->sensor()->sensorInfo(&sensorInfo); > + if (ret) > + /* Use the requested CIO2 output size as fallback. */ > + sensorInfo.analogCrop = Rectangle(config->cio2Size()); > + data->scalerCrop_ = sensorInfo.analogCrop; > + > /* > * If the ImgU gets configured, its driver seems to expect that > * buffers will be queued to its outputs, as otherwise the next > @@ -656,6 +665,14 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > IPU3CameraData *data = cameraData(camera); > int error = 0; > > + ControlList &controls = request->controls(); > + if (controls.contains(controls::ScalerCrop)) > + /* > + * \todo No scaling is applied. Just return the value in the > + * request metadata as it is. > + */ > + data->scalerCrop_ = controls.get(controls::ScalerCrop); This means that request completing after the this request is queued will all have the new scaler crop rectangle. That's not right, it should be synchronized with this request. > + > /* > * Queue a buffer on the CIO2, using the raw stream buffer provided in > * the request, if any, or a CIO2 internal buffer otherwise. > @@ -991,6 +1008,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > /* Mark the request as complete. */ > request->metadata().set(controls::draft::PipelineDepth, 3); > request->metadata().set(controls::ExposureTime, exposureTime_); > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > pipe_->completeRequest(request); > } >
Hi Laurent, On Mon, Jan 11, 2021 at 01:53:38AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Jan 05, 2021 at 08:05:19PM +0100, Jacopo Mondi wrote: > > Add support for caching the value of control::ScalerCrop to return > > it in the request metadata. > > > > No cropping is currently applied on the input video device, the control > > value is returned in the metadata pack as it is received. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index f1329ffb0463..381524bb3499 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -66,6 +66,7 @@ public: > > Stream rawStream_; > > > > int32_t exposureTime_; > > + Rectangle scalerCrop_; > > }; > > > > class IPU3CameraConfiguration : public CameraConfiguration > > @@ -75,6 +76,7 @@ public: > > > > Status validate() override; > > > > + const Size &cio2Size() const { return cio2Configuration_.size; } > > Do we need this, can't we keep using config->cio2Format().size ? > > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > > const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > > > @@ -468,12 +470,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > * Pass the requested stream size to the CIO2 unit and get back the > > * adjusted format to be propagated to the ImgU output devices. > > */ > > - const Size &sensorSize = config->cio2Format().size; > > V4L2DeviceFormat cio2Format; > > - ret = cio2->configure(sensorSize, &cio2Format); > > + ret = cio2->configure(config->cio2Size(), &cio2Format); > > if (ret) > > return ret; > > > > + /* Initialize the scaler crop using the sensor's analogue crop. */ > > + CameraSensorInfo sensorInfo; > > + ret = cio2->sensor()->sensorInfo(&sensorInfo); > > + if (ret) > > + /* Use the requested CIO2 output size as fallback. */ > > + sensorInfo.analogCrop = Rectangle(config->cio2Size()); > > + data->scalerCrop_ = sensorInfo.analogCrop; > > + > > /* > > * If the ImgU gets configured, its driver seems to expect that > > * buffers will be queued to its outputs, as otherwise the next > > @@ -656,6 +665,14 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > IPU3CameraData *data = cameraData(camera); > > int error = 0; > > > > + ControlList &controls = request->controls(); > > + if (controls.contains(controls::ScalerCrop)) > > + /* > > + * \todo No scaling is applied. Just return the value in the > > + * request metadata as it is. > > + */ > > + data->scalerCrop_ = controls.get(controls::ScalerCrop); > > This means that request completing after the this request is queued will > all have the new scaler crop rectangle. That's not right, it should be > synchronized with this request. > Correct -.- should I rather take the scaler crop value to be set in metadata from the Request's controls ? > > + > > /* > > * Queue a buffer on the CIO2, using the raw stream buffer provided in > > * the request, if any, or a CIO2 internal buffer otherwise. > > @@ -991,6 +1008,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > /* Mark the request as complete. */ > > request->metadata().set(controls::draft::PipelineDepth, 3); > > request->metadata().set(controls::ExposureTime, exposureTime_); > > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > pipe_->completeRequest(request); > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Jan 18, 2021 at 11:55:33AM +0100, Jacopo Mondi wrote: > On Mon, Jan 11, 2021 at 01:53:38AM +0200, Laurent Pinchart wrote: > > On Tue, Jan 05, 2021 at 08:05:19PM +0100, Jacopo Mondi wrote: > > > Add support for caching the value of control::ScalerCrop to return > > > it in the request metadata. > > > > > > No cropping is currently applied on the input video device, the control > > > value is returned in the metadata pack as it is received. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++-- > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index f1329ffb0463..381524bb3499 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -66,6 +66,7 @@ public: > > > Stream rawStream_; > > > > > > int32_t exposureTime_; > > > + Rectangle scalerCrop_; > > > }; > > > > > > class IPU3CameraConfiguration : public CameraConfiguration > > > @@ -75,6 +76,7 @@ public: > > > > > > Status validate() override; > > > > > > + const Size &cio2Size() const { return cio2Configuration_.size; } > > > > Do we need this, can't we keep using config->cio2Format().size ? > > > > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > > > const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > > > > > @@ -468,12 +470,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > > * Pass the requested stream size to the CIO2 unit and get back the > > > * adjusted format to be propagated to the ImgU output devices. > > > */ > > > - const Size &sensorSize = config->cio2Format().size; > > > V4L2DeviceFormat cio2Format; > > > - ret = cio2->configure(sensorSize, &cio2Format); > > > + ret = cio2->configure(config->cio2Size(), &cio2Format); > > > if (ret) > > > return ret; > > > > > > + /* Initialize the scaler crop using the sensor's analogue crop. */ > > > + CameraSensorInfo sensorInfo; > > > + ret = cio2->sensor()->sensorInfo(&sensorInfo); > > > + if (ret) > > > + /* Use the requested CIO2 output size as fallback. */ > > > + sensorInfo.analogCrop = Rectangle(config->cio2Size()); > > > + data->scalerCrop_ = sensorInfo.analogCrop; > > > + > > > /* > > > * If the ImgU gets configured, its driver seems to expect that > > > * buffers will be queued to its outputs, as otherwise the next > > > @@ -656,6 +665,14 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > > IPU3CameraData *data = cameraData(camera); > > > int error = 0; > > > > > > + ControlList &controls = request->controls(); > > > + if (controls.contains(controls::ScalerCrop)) > > > + /* > > > + * \todo No scaling is applied. Just return the value in the > > > + * request metadata as it is. > > > + */ > > > + data->scalerCrop_ = controls.get(controls::ScalerCrop); > > > > This means that request completing after the this request is queued will > > all have the new scaler crop rectangle. That's not right, it should be > > synchronized with this request. > > Correct -.- > > should I rather take the scaler crop value to be set in metadata from > the Request's controls ? Yes that should fix the issue. > > > + > > > /* > > > * Queue a buffer on the CIO2, using the raw stream buffer provided in > > > * the request, if any, or a CIO2 internal buffer otherwise. > > > @@ -991,6 +1008,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > > /* Mark the request as complete. */ > > > request->metadata().set(controls::draft::PipelineDepth, 3); > > > request->metadata().set(controls::ExposureTime, exposureTime_); > > > + request->metadata().set(controls::ScalerCrop, scalerCrop_); > > > pipe_->completeRequest(request); > > > } > > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f1329ffb0463..381524bb3499 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -66,6 +66,7 @@ public: Stream rawStream_; int32_t exposureTime_; + Rectangle scalerCrop_; }; class IPU3CameraConfiguration : public CameraConfiguration @@ -75,6 +76,7 @@ public: Status validate() override; + const Size &cio2Size() const { return cio2Configuration_.size; } const StreamConfiguration &cio2Format() const { return cio2Configuration_; } const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } @@ -468,12 +470,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * Pass the requested stream size to the CIO2 unit and get back the * adjusted format to be propagated to the ImgU output devices. */ - const Size &sensorSize = config->cio2Format().size; V4L2DeviceFormat cio2Format; - ret = cio2->configure(sensorSize, &cio2Format); + ret = cio2->configure(config->cio2Size(), &cio2Format); if (ret) return ret; + /* Initialize the scaler crop using the sensor's analogue crop. */ + CameraSensorInfo sensorInfo; + ret = cio2->sensor()->sensorInfo(&sensorInfo); + if (ret) + /* Use the requested CIO2 output size as fallback. */ + sensorInfo.analogCrop = Rectangle(config->cio2Size()); + data->scalerCrop_ = sensorInfo.analogCrop; + /* * If the ImgU gets configured, its driver seems to expect that * buffers will be queued to its outputs, as otherwise the next @@ -656,6 +665,14 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) IPU3CameraData *data = cameraData(camera); int error = 0; + ControlList &controls = request->controls(); + if (controls.contains(controls::ScalerCrop)) + /* + * \todo No scaling is applied. Just return the value in the + * request metadata as it is. + */ + data->scalerCrop_ = controls.get(controls::ScalerCrop); + /* * Queue a buffer on the CIO2, using the raw stream buffer provided in * the request, if any, or a CIO2 internal buffer otherwise. @@ -991,6 +1008,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) /* Mark the request as complete. */ request->metadata().set(controls::draft::PipelineDepth, 3); request->metadata().set(controls::ExposureTime, exposureTime_); + request->metadata().set(controls::ScalerCrop, scalerCrop_); pipe_->completeRequest(request); }
Add support for caching the value of control::ScalerCrop to return it in the request metadata. No cropping is currently applied on the input video device, the control value is returned in the metadata pack as it is received. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)