| Message ID | 20210203162600.206297-6-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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? 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? I think this patch is a good step in the right direction, the question above is just to understand the bigger picture. Reviewed-by: Niklas Söderlund <niklas.soderlund@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; > }; > > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@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@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@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); > > > } > > >
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@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@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@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
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); }
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@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)