| Message ID | 20240613155949.1041061-4-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Daniel Scally (2024-06-13 16:59:45) > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Implement support for the ScalerCrop control that allows to apply a > digital zoom to the captured streams. > > Initialize the camera controls at camera registration time and update > them at configure time as the sensor's analogue crop size might change > depending on the desired Camera configuration. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++- > 1 file changed, 137 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > index c4f1afbc..2c34f3e9 100644 > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > @@ -100,6 +100,8 @@ public: > > PixelFormat bestRawFormat() const; > > + void updateControls(); > + > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; > > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const > return rawFormat; > } > > +void MaliC55CameraData::updateControls() > +{ > + if (!sensor_) > + return; > + > + IPACameraSensorInfo sensorInfo; > + int ret = sensor_->sensorInfo(&sensorInfo); > + if (ret) { > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > + return; > + } > + > + ControlInfoMap::Map controls; > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; This looks like it should be a constant somewhere outside of here? Perhaps class constant or just a constant at the top of the file. > + controls[&controls::ScalerCrop] = > + ControlInfo(ispMinCrop, sensorInfo.analogCrop, > + sensorInfo.analogCrop); > + > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > +} > + > /* > * Make sure the provided raw pixel format is supported and adjust it to > * one of the supported ones if it's not. > @@ -515,6 +538,8 @@ private: > const StreamConfiguration &config, > V4L2SubdeviceFormat &subdevFormat); > > + void applyScalerCrop(Camera *camera, const ControlList &controls); > + > void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data, > const std::string &name); > bool registerTPGCamera(MediaLink *link); > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > pipe->stream = stream; > } > > + data->updateControls(); > + > return 0; > } > > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera) > } > } > > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, > + const ControlList &controls) > +{ > + MaliC55CameraData *data = cameraData(camera); > + > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > + if (!scalerCrop) > + return; > + > + if (!data->sensor_) { > + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; > + return; > + } > + > + Rectangle nativeCrop = *scalerCrop; > + > + IPACameraSensorInfo sensorInfo; > + int ret = data->sensor_->sensorInfo(&sensorInfo); > + if (ret) { > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > + return; > + } > + > + /* > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle > + * comes straight from the RPi pipeline handler. > + * > + * Create a version of the crop rectangle aligned to the analogue crop > + * rectangle top-left coordinates and scaled in the [analogue crop to > + * output frame] ratio to take into account binning/skipping on the > + * sensor. > + */ > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop > + .topLeft()); > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size()); > + > + /* > + * The crop rectangle should be: > + * 1. At least as big as ispMinCropSize_, once that's been > + * enlarged to the same aspect ratio. > + * 2. With the same mid-point, if possible. > + * 3. But it can't go outside the sensor area. > + */ > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; Especially if it's used multiple times. > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); > + Size size = ispCrop.size().expandedTo(minSize); > + ispCrop = size.centeredTo(ispCrop.center()) > + .enclosedIn(Rectangle(sensorInfo.outputSize)); > + > + /* > + * As the resizer can't upscale, the crop rectangle has to be larger > + * than the larger stream output size. > + */ > + Size maxYuvSize; > + for (MaliC55Pipe &pipe : pipes_) { > + Probably delete that blank line. > + if (!pipe.stream) > + continue; > + > + const StreamConfiguration &config = pipe.stream->configuration(); > + if (isFormatRaw(config.pixelFormat)) { > + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream"; > + return; > + } > + > + Size streamSize = config.size; > + if (streamSize.width > maxYuvSize.width) > + maxYuvSize.width = streamSize.width; > + if (streamSize.height > maxYuvSize.height) > + maxYuvSize.height = streamSize.height; > + } > + > + ispCrop.size().expandTo(maxYuvSize); > + > + /* > + * Now apply the scaler crop to each enabled output. This overrides the > + * crop configuration performed at configure() time and can cause > + * square pixels if the crop rectangle and scaler output FOV ratio are Can cause 'non square' pixels perhaps? > + * different. > + */ > + for (MaliC55Pipe &pipe : pipes_) { > + > + if (!pipe.stream) > + continue; > + > + /* Create a copy to avoid setSelection() to modify ispCrop. */ 'to prevent setSelection() from modifying ispCrop' > + Rectangle pipeCrop = ispCrop; > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop); > + if (ret) { > + LOG(MaliC55, Error) > + << "Failed to apply crop to " > + << (pipe.stream == &data->frStream_ ? > + "FR" : "DS") << " pipe"; > + return; > + } > + } > +} > + > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > { > int ret; > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > return ret; > } > > + /* > + * Some controls need to be applied immediately, as in example, > + * the ScalerCrop one. > + * > + * \todo Move it buffer queue time (likely after the IPA has filled in > + * the parameters buffer) once we have plumbed the IPA loop in. > + */ > + applyScalerCrop(camera, request->controls()); > + > return 0; > } > > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) > if (data->init()) > return false; > > - /* \todo: Init properties and controls. */ > + /* \todo: Init properties. */ > + > + data->updateControls(); > > registerMaliCamera(std::move(data), sensor->name()); > } > -- > 2.30.2 >
On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote: > Quoting Daniel Scally (2024-06-13 16:59:45) > > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Implement support for the ScalerCrop control that allows to apply a > > digital zoom to the captured streams. > > > > Initialize the camera controls at camera registration time and update > > them at configure time as the sensor's analogue crop size might change > > depending on the desired Camera configuration. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++- > > 1 file changed, 137 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > index c4f1afbc..2c34f3e9 100644 > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > @@ -100,6 +100,8 @@ public: > > > > PixelFormat bestRawFormat() const; > > > > + void updateControls(); > > + > > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; > > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; > > > > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const > > return rawFormat; > > } > > > > +void MaliC55CameraData::updateControls() > > +{ > > + if (!sensor_) > > + return; > > + > > + IPACameraSensorInfo sensorInfo; > > + int ret = sensor_->sensorInfo(&sensorInfo); > > + if (ret) { > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > > + return; > > + } > > + > > + ControlInfoMap::Map controls; > > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; > > This looks like it should be a constant somewhere outside of here? > Perhaps class constant or just a constant at the top of the file. > > > + controls[&controls::ScalerCrop] = > > + ControlInfo(ispMinCrop, sensorInfo.analogCrop, > > + sensorInfo.analogCrop); > > + > > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > > +} > > + > > /* > > * Make sure the provided raw pixel format is supported and adjust it to > > * one of the supported ones if it's not. > > @@ -515,6 +538,8 @@ private: > > const StreamConfiguration &config, > > V4L2SubdeviceFormat &subdevFormat); > > > > + void applyScalerCrop(Camera *camera, const ControlList &controls); > > + > > void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data, > > const std::string &name); > > bool registerTPGCamera(MediaLink *link); > > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > > pipe->stream = stream; > > } > > > > + data->updateControls(); > > + > > return 0; > > } > > > > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera) > > } > > } > > > > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, > > + const ControlList &controls) > > +{ > > + MaliC55CameraData *data = cameraData(camera); > > + > > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > > + if (!scalerCrop) > > + return; > > + > > + if (!data->sensor_) { > > + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; > > + return; > > + } > > + > > + Rectangle nativeCrop = *scalerCrop; > > + > > + IPACameraSensorInfo sensorInfo; > > + int ret = data->sensor_->sensorInfo(&sensorInfo); Gathering the sensor info is a bit expensive, it should be cached. > > + if (ret) { > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > > + return; > > + } > > + > > + /* > > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle > > + * comes straight from the RPi pipeline handler. Does this mean we should have a helper somewhere ? And does this comment belong here, or to the commit message ? > > + * > > + * Create a version of the crop rectangle aligned to the analogue crop > > + * rectangle top-left coordinates and scaled in the [analogue crop to > > + * output frame] ratio to take into account binning/skipping on the > > + * sensor. > > + */ > > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop > > + .topLeft()); > > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size()); > > + > > + /* > > + * The crop rectangle should be: > > + * 1. At least as big as ispMinCropSize_, once that's been > > + * enlarged to the same aspect ratio. Does this mean the hardware doesn' support sensor resolutions smaller than 640x480 ? > > + * 2. With the same mid-point, if possible. Why so ? Isn't the user in control of that ? > > + * 3. But it can't go outside the sensor area. > > + */ > > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; > > Especially if it's used multiple times. > > > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); > > + Size size = ispCrop.size().expandedTo(minSize); > > + ispCrop = size.centeredTo(ispCrop.center()) > > + .enclosedIn(Rectangle(sensorInfo.outputSize)); > > + > > + /* > > + * As the resizer can't upscale, the crop rectangle has to be larger > > + * than the larger stream output size. > > + */ > > + Size maxYuvSize; > > + for (MaliC55Pipe &pipe : pipes_) { > > + > > Probably delete that blank line. > > > + if (!pipe.stream) > > + continue; > > + > > + const StreamConfiguration &config = pipe.stream->configuration(); > > + if (isFormatRaw(config.pixelFormat)) { > > + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream"; > > + return; > > + } > > + > > + Size streamSize = config.size; > > + if (streamSize.width > maxYuvSize.width) > > + maxYuvSize.width = streamSize.width; > > + if (streamSize.height > maxYuvSize.height) > > + maxYuvSize.height = streamSize.height; > > + } > > + > > + ispCrop.size().expandTo(maxYuvSize); > > + > > + /* > > + * Now apply the scaler crop to each enabled output. This overrides the > > + * crop configuration performed at configure() time and can cause > > + * square pixels if the crop rectangle and scaler output FOV ratio are > > Can cause 'non square' pixels perhaps? > > > + * different. > > + */ > > + for (MaliC55Pipe &pipe : pipes_) { > > + > > + if (!pipe.stream) > > + continue; > > + > > + /* Create a copy to avoid setSelection() to modify ispCrop. */ > > 'to prevent setSelection() from modifying ispCrop' > > > + Rectangle pipeCrop = ispCrop; > > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop); > > + if (ret) { > > + LOG(MaliC55, Error) > > + << "Failed to apply crop to " > > + << (pipe.stream == &data->frStream_ ? > > + "FR" : "DS") << " pipe"; > > + return; > > + } > > + } The effective ScalerCrop value should be reported in metadata. > > +} > > + > > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > > { > > int ret; > > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > > return ret; > > } > > > > + /* > > + * Some controls need to be applied immediately, as in example, > > + * the ScalerCrop one. > > + * > > + * \todo Move it buffer queue time (likely after the IPA has filled in > > + * the parameters buffer) once we have plumbed the IPA loop in. > > + */ > > + applyScalerCrop(camera, request->controls()); > > + > > return 0; > > } > > > > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) > > if (data->init()) > > return false; > > > > - /* \todo: Init properties and controls. */ > > + /* \todo: Init properties. */ > > + > > + data->updateControls(); > > > > registerMaliCamera(std::move(data), sensor->name()); > > }
Hi Laurent On Mon, Jun 17, 2024 at 08:11:33AM GMT, Laurent Pinchart wrote: > On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote: > > Quoting Daniel Scally (2024-06-13 16:59:45) > > > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > Implement support for the ScalerCrop control that allows to apply a > > > digital zoom to the captured streams. > > > > > > Initialize the camera controls at camera registration time and update > > > them at configure time as the sensor's analogue crop size might change > > > depending on the desired Camera configuration. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++- > > > 1 file changed, 137 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > > index c4f1afbc..2c34f3e9 100644 > > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp > > > @@ -100,6 +100,8 @@ public: > > > > > > PixelFormat bestRawFormat() const; > > > > > > + void updateControls(); > > > + > > > PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; > > > Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; > > > > > > @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const > > > return rawFormat; > > > } > > > > > > +void MaliC55CameraData::updateControls() > > > +{ > > > + if (!sensor_) > > > + return; > > > + > > > + IPACameraSensorInfo sensorInfo; > > > + int ret = sensor_->sensorInfo(&sensorInfo); > > > + if (ret) { > > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > > > + return; > > > + } > > > + > > > + ControlInfoMap::Map controls; > > > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; > > > > This looks like it should be a constant somewhere outside of here? > > Perhaps class constant or just a constant at the top of the file. > > ack > > > + controls[&controls::ScalerCrop] = > > > + ControlInfo(ispMinCrop, sensorInfo.analogCrop, > > > + sensorInfo.analogCrop); > > > + > > > + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); > > > +} > > > + > > > /* > > > * Make sure the provided raw pixel format is supported and adjust it to > > > * one of the supported ones if it's not. > > > @@ -515,6 +538,8 @@ private: > > > const StreamConfiguration &config, > > > V4L2SubdeviceFormat &subdevFormat); > > > > > > + void applyScalerCrop(Camera *camera, const ControlList &controls); > > > + > > > void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data, > > > const std::string &name); > > > bool registerTPGCamera(MediaLink *link); > > > @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera, > > > pipe->stream = stream; > > > } > > > > > > + data->updateControls(); > > > + > > > return 0; > > > } > > > > > > @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera) > > > } > > > } > > > > > > +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, > > > + const ControlList &controls) > > > +{ > > > + MaliC55CameraData *data = cameraData(camera); > > > + > > > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > > > + if (!scalerCrop) > > > + return; > > > + > > > + if (!data->sensor_) { > > > + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; > > > + return; > > > + } > > > + > > > + Rectangle nativeCrop = *scalerCrop; > > > + > > > + IPACameraSensorInfo sensorInfo; > > > + int ret = data->sensor_->sensorInfo(&sensorInfo); > > Gathering the sensor info is a bit expensive, it should be cached. > I can cache it at configure() time, even if the pixel rate and blanking values might be outdated as they can change during streaming time. In general, sensorInfo describes the current sensor configuration, it's not desirable to cache it. However here I actually only need the sensor's output size and the analog crop which shouldn't change during streaming. Should I cache the two ? It seems a bit of an hack to have them cached in CameraData. > > > + if (ret) { > > > + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; > > > + return; > > > + } > > > + > > > + /* > > > + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle > > > + * comes straight from the RPi pipeline handler. > > Does this mean we should have a helper somewhere ? And does this comment I'm not sure where should I put it. This combines properties of the CameraSensor class with ISP's ones (the min crop size) and with the value of an application provided control. Could go in CameraSensor but a CameraSensor::rescaleScalerCrop() feels a bit out of place ? > belong here, or to the commit message ? > Yes > > > + * > > > + * Create a version of the crop rectangle aligned to the analogue crop > > > + * rectangle top-left coordinates and scaled in the [analogue crop to > > > + * output frame] ratio to take into account binning/skipping on the > > > + * sensor. > > > + */ > > > + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop > > > + .topLeft()); > > > + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size()); > > > + > > > + /* > > > + * The crop rectangle should be: > > > + * 1. At least as big as ispMinCropSize_, once that's been > > > + * enlarged to the same aspect ratio. > > Does this mean the hardware doesn' support sensor resolutions smaller > than 640x480 ? > Yes, the TRM reports: Minimum supported resolution: 640 (w) x 480 (h) Now I wonder if is this the minimum supported sensor input resolution or the ISP minimum output size ? If 640x480 represents the minimum allowed input size, then it shouldn't be here. We crop on the resizer's input, not on the ISP input, so (unless there's a minimum resizer's crop limit) we shouldn't care about this here, but rather filter out all sensor modes smaller than 640x480 during validate(). If 640x480 represnts the minimum output resolution, then we should fix constexpr Size kMaliC55MinSize = { 128, 128 }; Dan do you have any idea here ? > > > + * 2. With the same mid-point, if possible. > > Why so ? Isn't the user in control of that ? It is, if you look at Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); Size size = ispCrop.size().expandedTo(minSize); ispCrop = size.centeredTo(ispCrop.center()) .enclosedIn(Rectangle(sensorInfo.outputSize)); 'size' is the ScalerCrop size expanded to the minimum accepted ISP crop size, and we're centering it inside 'ispCrop.center()' and 'ispCrop' already has a top-left corner set by the user relatively to the native pixel array size (and re-scaled in the analogueCrop-to-outputSize ratio). So, if ispCrop.size() > minSize then Size size = ispCrop.size().expandedTo(minSize); will give you ispCrop.size() and the rectangle resulting from centering 'size' inside 'ispCrop' will be 'ispCrop' again. If ispCrop.size() < minSize then ispCrop will be expanded to minSize and the resulting Rectangle will be centered over ispCrop. Does this match your understanding ? However, there is one thing missing in this patch: * Create a version of the crop rectangle aligned to the analogue crop * rectangle top-left coordinates */ Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop .topLeft()); Assumes the ScalerCrop is contained in the analogueCrop rectangle, otherwise the here above 'translatedBy' will give you a negative top-left corner. This is actually correct if we assume controls::ScalerCropMaximum is set to be the AnalogueCrop (so all ScalerCrop will be contained in AnalogueCrop). RPi does set ScalerCropMaximum, this patch doesn't. I presume however we should actually make sure the actual value of ScalerCrop is at least contained in the AnalogueCrop instead of assuming the application does the right thing. In case ScalerCrop > AnalogueCrop should we adjust it and return its value without applying any crop ? An helper in CameraSensor could actually help addressing this for all pipelines > > > > + * 3. But it can't go outside the sensor area. > > > + */ > > > + Rectangle ispMinCrop{ 0, 0, 640, 480 }; > > > > Especially if it's used multiple times. > > > > > + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); > > > + Size size = ispCrop.size().expandedTo(minSize); > > > + ispCrop = size.centeredTo(ispCrop.center()) > > > + .enclosedIn(Rectangle(sensorInfo.outputSize)); > > > + > > > + /* > > > + * As the resizer can't upscale, the crop rectangle has to be larger > > > + * than the larger stream output size. > > > + */ > > > + Size maxYuvSize; > > > + for (MaliC55Pipe &pipe : pipes_) { > > > + > > > > Probably delete that blank line. > > I like it! :) > > > + if (!pipe.stream) > > > + continue; > > > + > > > + const StreamConfiguration &config = pipe.stream->configuration(); > > > + if (isFormatRaw(config.pixelFormat)) { > > > + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream"; > > > + return; > > > + } > > > + > > > + Size streamSize = config.size; > > > + if (streamSize.width > maxYuvSize.width) > > > + maxYuvSize.width = streamSize.width; > > > + if (streamSize.height > maxYuvSize.height) > > > + maxYuvSize.height = streamSize.height; > > > + } > > > + > > > + ispCrop.size().expandTo(maxYuvSize); > > > + > > > + /* > > > + * Now apply the scaler crop to each enabled output. This overrides the > > > + * crop configuration performed at configure() time and can cause > > > + * square pixels if the crop rectangle and scaler output FOV ratio are > > > > Can cause 'non square' pixels perhaps? > > Ah! I always said "square pixels" to indicate this situation, but in my mind "square pixels" were good. So I thought it was an expression in English I didn't fully understand :) I'll fix it. > > > + * different. > > > + */ > > > + for (MaliC55Pipe &pipe : pipes_) { > > > + > > > + if (!pipe.stream) > > > + continue; > > > + > > > + /* Create a copy to avoid setSelection() to modify ispCrop. */ > > > > 'to prevent setSelection() from modifying ispCrop' > > > > > + Rectangle pipeCrop = ispCrop; > > > + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop); > > > + if (ret) { > > > + LOG(MaliC55, Error) > > > + << "Failed to apply crop to " > > > + << (pipe.stream == &data->frStream_ ? > > > + "FR" : "DS") << " pipe"; > > > + return; > > > + } > > > + } > > The effective ScalerCrop value should be reported in metadata. > true that > > > +} > > > + > > > int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > > > { > > > int ret; > > > @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) > > > return ret; > > > } > > > > > > + /* > > > + * Some controls need to be applied immediately, as in example, > > > + * the ScalerCrop one. > > > + * > > > + * \todo Move it buffer queue time (likely after the IPA has filled in > > > + * the parameters buffer) once we have plumbed the IPA loop in. > > > + */ > > > + applyScalerCrop(camera, request->controls()); > > > + > > > return 0; > > > } > > > > > > @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) > > > if (data->init()) > > > return false; > > > > > > - /* \todo: Init properties and controls. */ > > > + /* \todo: Init properties. */ > > > + > > > + data->updateControls(); > > > > > > registerMaliCamera(std::move(data), sensor->name()); > > > } > > -- > Regards, > > Laurent Pinchart
Hello both On 24/06/2024 16:01, Jacopo Mondi wrote: > Hi Laurent > > On Mon, Jun 17, 2024 at 08:11:33AM GMT, Laurent Pinchart wrote: >> On Thu, Jun 13, 2024 at 08:33:11PM +0100, Kieran Bingham wrote: >>> Quoting Daniel Scally (2024-06-13 16:59:45) >>>> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> >>>> Implement support for the ScalerCrop control that allows to apply a >>>> digital zoom to the captured streams. >>>> >>>> Initialize the camera controls at camera registration time and update >>>> them at configure time as the sensor's analogue crop size might change >>>> depending on the desired Camera configuration. >>>> >>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> --- >>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 138 ++++++++++++++++++- >>>> 1 file changed, 137 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp >>>> index c4f1afbc..2c34f3e9 100644 >>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp >>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp >>>> @@ -100,6 +100,8 @@ public: >>>> >>>> PixelFormat bestRawFormat() const; >>>> >>>> + void updateControls(); >>>> + >>>> PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; >>>> Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; >>>> >>>> @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const >>>> return rawFormat; >>>> } >>>> >>>> +void MaliC55CameraData::updateControls() >>>> +{ >>>> + if (!sensor_) >>>> + return; >>>> + >>>> + IPACameraSensorInfo sensorInfo; >>>> + int ret = sensor_->sensorInfo(&sensorInfo); >>>> + if (ret) { >>>> + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; >>>> + return; >>>> + } >>>> + >>>> + ControlInfoMap::Map controls; >>>> + Rectangle ispMinCrop{ 0, 0, 640, 480 }; >>> This looks like it should be a constant somewhere outside of here? >>> Perhaps class constant or just a constant at the top of the file. >>> > ack > >>>> + controls[&controls::ScalerCrop] = >>>> + ControlInfo(ispMinCrop, sensorInfo.analogCrop, >>>> + sensorInfo.analogCrop); >>>> + >>>> + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); >>>> +} >>>> + >>>> /* >>>> * Make sure the provided raw pixel format is supported and adjust it to >>>> * one of the supported ones if it's not. >>>> @@ -515,6 +538,8 @@ private: >>>> const StreamConfiguration &config, >>>> V4L2SubdeviceFormat &subdevFormat); >>>> >>>> + void applyScalerCrop(Camera *camera, const ControlList &controls); >>>> + >>>> void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data, >>>> const std::string &name); >>>> bool registerTPGCamera(MediaLink *link); >>>> @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera, >>>> pipe->stream = stream; >>>> } >>>> >>>> + data->updateControls(); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera) >>>> } >>>> } >>>> >>>> +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, >>>> + const ControlList &controls) >>>> +{ >>>> + MaliC55CameraData *data = cameraData(camera); >>>> + >>>> + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); >>>> + if (!scalerCrop) >>>> + return; >>>> + >>>> + if (!data->sensor_) { >>>> + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; >>>> + return; >>>> + } >>>> + >>>> + Rectangle nativeCrop = *scalerCrop; >>>> + >>>> + IPACameraSensorInfo sensorInfo; >>>> + int ret = data->sensor_->sensorInfo(&sensorInfo); >> Gathering the sensor info is a bit expensive, it should be cached. >> > I can cache it at configure() time, even if the pixel rate and > blanking values might be outdated as they can change during streaming > time. > > In general, sensorInfo describes the current sensor configuration, > it's not desirable to cache it. However here I actually only need the > sensor's output size and the analog crop which shouldn't change during > streaming. Should I cache the two ? It seems a bit of an hack to have > them cached in CameraData. > >>>> + if (ret) { >>>> + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; >>>> + return; >>>> + } >>>> + >>>> + /* >>>> + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle >>>> + * comes straight from the RPi pipeline handler. >> Does this mean we should have a helper somewhere ? And does this comment > I'm not sure where should I put it. This combines properties of the > CameraSensor class with ISP's ones (the min crop size) and with the > value of an application provided control. Could go in CameraSensor but a > CameraSensor::rescaleScalerCrop() feels a bit out of place ? > >> belong here, or to the commit message ? >> > Yes > >>>> + * >>>> + * Create a version of the crop rectangle aligned to the analogue crop >>>> + * rectangle top-left coordinates and scaled in the [analogue crop to >>>> + * output frame] ratio to take into account binning/skipping on the >>>> + * sensor. >>>> + */ >>>> + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop >>>> + .topLeft()); >>>> + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size()); >>>> + >>>> + /* >>>> + * The crop rectangle should be: >>>> + * 1. At least as big as ispMinCropSize_, once that's been >>>> + * enlarged to the same aspect ratio. >> Does this mean the hardware doesn' support sensor resolutions smaller >> than 640x480 ? >> > Yes, the TRM reports: > > Minimum supported resolution: 640 (w) x 480 (h) > > Now I wonder if is this the minimum supported sensor input resolution or the > ISP minimum output size ? > > If 640x480 represents the minimum allowed input size, then it > shouldn't be here. We crop on the resizer's input, not on the ISP > input, so (unless there's a minimum resizer's crop limit) we shouldn't > care about this here, but rather filter out all sensor modes smaller > than 640x480 during validate(). > > If 640x480 represnts the minimum output resolution, then we should fix > > constexpr Size kMaliC55MinSize = { 128, 128 }; > > Dan do you have any idea here ? I think it's both minimum supported input and output; I discussed with Arm smaller input framesizes (in the context of the TPG at the time) and the conclusion from the discussion was not to support smaller than 640x480. > >>>> + * 2. With the same mid-point, if possible. >> Why so ? Isn't the user in control of that ? > It is, if you look at > > Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); > Size size = ispCrop.size().expandedTo(minSize); > ispCrop = size.centeredTo(ispCrop.center()) > .enclosedIn(Rectangle(sensorInfo.outputSize)); > > 'size' is the ScalerCrop size expanded to the minimum accepted ISP crop > size, and we're centering it inside 'ispCrop.center()' and > 'ispCrop' already has a top-left corner set by the user relatively to > the native pixel array size (and re-scaled in the analogueCrop-to-outputSize > ratio). > > So, if ispCrop.size() > minSize then > > Size size = ispCrop.size().expandedTo(minSize); > > will give you ispCrop.size() and the rectangle resulting from > centering 'size' inside 'ispCrop' will be 'ispCrop' again. > > If ispCrop.size() < minSize then ispCrop will be expanded to minSize > and the resulting Rectangle will be centered over ispCrop. > > Does this match your understanding ? > > However, there is one thing missing in this patch: > > * Create a version of the crop rectangle aligned to the analogue crop > * rectangle top-left coordinates > */ > Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop > .topLeft()); > > Assumes the ScalerCrop is contained in the analogueCrop rectangle, > otherwise the here above 'translatedBy' will give you a negative > top-left corner. > > This is actually correct if we assume controls::ScalerCropMaximum > is set to be the AnalogueCrop (so all ScalerCrop will be contained in > AnalogueCrop). RPi does set ScalerCropMaximum, this patch doesn't. > > I presume however we should actually make sure the actual value of > ScalerCrop is at least contained in the AnalogueCrop instead of > assuming the application does the right thing. In case ScalerCrop > > AnalogueCrop should we adjust it and return its value without applying > any crop ? An helper in CameraSensor could actually help addressing > this for all pipelines > >>>> + * 3. But it can't go outside the sensor area. >>>> + */ >>>> + Rectangle ispMinCrop{ 0, 0, 640, 480 }; >>> Especially if it's used multiple times. >>> >>>> + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); >>>> + Size size = ispCrop.size().expandedTo(minSize); >>>> + ispCrop = size.centeredTo(ispCrop.center()) >>>> + .enclosedIn(Rectangle(sensorInfo.outputSize)); >>>> + >>>> + /* >>>> + * As the resizer can't upscale, the crop rectangle has to be larger >>>> + * than the larger stream output size. >>>> + */ >>>> + Size maxYuvSize; >>>> + for (MaliC55Pipe &pipe : pipes_) { >>>> + >>> Probably delete that blank line. >>> > I like it! :) > >>>> + if (!pipe.stream) >>>> + continue; >>>> + >>>> + const StreamConfiguration &config = pipe.stream->configuration(); >>>> + if (isFormatRaw(config.pixelFormat)) { >>>> + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream"; >>>> + return; >>>> + } >>>> + >>>> + Size streamSize = config.size; >>>> + if (streamSize.width > maxYuvSize.width) >>>> + maxYuvSize.width = streamSize.width; >>>> + if (streamSize.height > maxYuvSize.height) >>>> + maxYuvSize.height = streamSize.height; >>>> + } >>>> + >>>> + ispCrop.size().expandTo(maxYuvSize); >>>> + >>>> + /* >>>> + * Now apply the scaler crop to each enabled output. This overrides the >>>> + * crop configuration performed at configure() time and can cause >>>> + * square pixels if the crop rectangle and scaler output FOV ratio are >>> Can cause 'non square' pixels perhaps? >>> > Ah! I always said "square pixels" to indicate this situation, > but in my mind "square pixels" were good. So I thought it was an > expression in English I didn't fully understand :) I'll fix it. > >>>> + * different. >>>> + */ >>>> + for (MaliC55Pipe &pipe : pipes_) { >>>> + >>>> + if (!pipe.stream) >>>> + continue; >>>> + >>>> + /* Create a copy to avoid setSelection() to modify ispCrop. */ >>> 'to prevent setSelection() from modifying ispCrop' >>> >>>> + Rectangle pipeCrop = ispCrop; >>>> + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop); >>>> + if (ret) { >>>> + LOG(MaliC55, Error) >>>> + << "Failed to apply crop to " >>>> + << (pipe.stream == &data->frStream_ ? >>>> + "FR" : "DS") << " pipe"; >>>> + return; >>>> + } >>>> + } >> The effective ScalerCrop value should be reported in metadata. >> > true that > >>>> +} >>>> + >>>> int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) >>>> { >>>> int ret; >>>> @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) >>>> return ret; >>>> } >>>> >>>> + /* >>>> + * Some controls need to be applied immediately, as in example, >>>> + * the ScalerCrop one. >>>> + * >>>> + * \todo Move it buffer queue time (likely after the IPA has filled in >>>> + * the parameters buffer) once we have plumbed the IPA loop in. >>>> + */ >>>> + applyScalerCrop(camera, request->controls()); >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) >>>> if (data->init()) >>>> return false; >>>> >>>> - /* \todo: Init properties and controls. */ >>>> + /* \todo: Init properties. */ >>>> + >>>> + data->updateControls(); >>>> >>>> registerMaliCamera(std::move(data), sensor->name()); >>>> } >> -- >> Regards, >> >> Laurent Pinchart
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp index c4f1afbc..2c34f3e9 100644 --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp @@ -100,6 +100,8 @@ public: PixelFormat bestRawFormat() const; + void updateControls(); + PixelFormat adjustRawFormat(const PixelFormat &pixFmt) const; Size adjustRawSizes(const PixelFormat &pixFmt, const Size &rawSize) const; @@ -245,6 +247,27 @@ PixelFormat MaliC55CameraData::bestRawFormat() const return rawFormat; } +void MaliC55CameraData::updateControls() +{ + if (!sensor_) + return; + + IPACameraSensorInfo sensorInfo; + int ret = sensor_->sensorInfo(&sensorInfo); + if (ret) { + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; + return; + } + + ControlInfoMap::Map controls; + Rectangle ispMinCrop{ 0, 0, 640, 480 }; + controls[&controls::ScalerCrop] = + ControlInfo(ispMinCrop, sensorInfo.analogCrop, + sensorInfo.analogCrop); + + controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); +} + /* * Make sure the provided raw pixel format is supported and adjust it to * one of the supported ones if it's not. @@ -515,6 +538,8 @@ private: const StreamConfiguration &config, V4L2SubdeviceFormat &subdevFormat); + void applyScalerCrop(Camera *camera, const ControlList &controls); + void registerMaliCamera(std::unique_ptr<MaliC55CameraData> data, const std::string &name); bool registerTPGCamera(MediaLink *link); @@ -828,6 +853,8 @@ int PipelineHandlerMaliC55::configure(Camera *camera, pipe->stream = stream; } + data->updateControls(); + return 0; } @@ -875,6 +902,104 @@ void PipelineHandlerMaliC55::stopDevice([[maybe_unused]] Camera *camera) } } +void PipelineHandlerMaliC55::applyScalerCrop(Camera *camera, + const ControlList &controls) +{ + MaliC55CameraData *data = cameraData(camera); + + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); + if (!scalerCrop) + return; + + if (!data->sensor_) { + LOG(MaliC55, Error) << "ScalerCrop not supported for TPG"; + return; + } + + Rectangle nativeCrop = *scalerCrop; + + IPACameraSensorInfo sensorInfo; + int ret = data->sensor_->sensorInfo(&sensorInfo); + if (ret) { + LOG(MaliC55, Error) << "Failed to retrieve sensor info"; + return; + } + + /* + * The ScalerCrop rectangle re-scaling in the ISP crop rectangle + * comes straight from the RPi pipeline handler. + * + * Create a version of the crop rectangle aligned to the analogue crop + * rectangle top-left coordinates and scaled in the [analogue crop to + * output frame] ratio to take into account binning/skipping on the + * sensor. + */ + Rectangle ispCrop = nativeCrop.translatedBy(-sensorInfo.analogCrop + .topLeft()); + ispCrop.scaleBy(sensorInfo.outputSize, sensorInfo.analogCrop.size()); + + /* + * The crop rectangle should be: + * 1. At least as big as ispMinCropSize_, once that's been + * enlarged to the same aspect ratio. + * 2. With the same mid-point, if possible. + * 3. But it can't go outside the sensor area. + */ + Rectangle ispMinCrop{ 0, 0, 640, 480 }; + Size minSize = ispMinCrop.size().expandedToAspectRatio(nativeCrop.size()); + Size size = ispCrop.size().expandedTo(minSize); + ispCrop = size.centeredTo(ispCrop.center()) + .enclosedIn(Rectangle(sensorInfo.outputSize)); + + /* + * As the resizer can't upscale, the crop rectangle has to be larger + * than the larger stream output size. + */ + Size maxYuvSize; + for (MaliC55Pipe &pipe : pipes_) { + + if (!pipe.stream) + continue; + + const StreamConfiguration &config = pipe.stream->configuration(); + if (isFormatRaw(config.pixelFormat)) { + LOG(MaliC55, Debug) << "Cannot crop with a RAW stream"; + return; + } + + Size streamSize = config.size; + if (streamSize.width > maxYuvSize.width) + maxYuvSize.width = streamSize.width; + if (streamSize.height > maxYuvSize.height) + maxYuvSize.height = streamSize.height; + } + + ispCrop.size().expandTo(maxYuvSize); + + /* + * Now apply the scaler crop to each enabled output. This overrides the + * crop configuration performed at configure() time and can cause + * square pixels if the crop rectangle and scaler output FOV ratio are + * different. + */ + for (MaliC55Pipe &pipe : pipes_) { + + if (!pipe.stream) + continue; + + /* Create a copy to avoid setSelection() to modify ispCrop. */ + Rectangle pipeCrop = ispCrop; + ret = pipe.resizer->setSelection(0, V4L2_SEL_TGT_CROP, &pipeCrop); + if (ret) { + LOG(MaliC55, Error) + << "Failed to apply crop to " + << (pipe.stream == &data->frStream_ ? + "FR" : "DS") << " pipe"; + return; + } + } +} + int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) { int ret; @@ -887,6 +1012,15 @@ int PipelineHandlerMaliC55::queueRequestDevice(Camera *camera, Request *request) return ret; } + /* + * Some controls need to be applied immediately, as in example, + * the ScalerCrop one. + * + * \todo Move it buffer queue time (likely after the IPA has filled in + * the parameters buffer) once we have plumbed the IPA loop in. + */ + applyScalerCrop(camera, request->controls()); + return 0; } @@ -965,7 +1099,9 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) if (data->init()) return false; - /* \todo: Init properties and controls. */ + /* \todo: Init properties. */ + + data->updateControls(); registerMaliCamera(std::move(data), sensor->name()); }