| Message ID | 20210105190522.682324-8-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote: > Register the ScalerCrop control in the ImgU pipeline handler computed > by using the Viewfinder [1280x720] pipeline output configuration and > the sensor resolution as parameters. > > The ScalerCrop control limits should be updated everytime a new > configuration is applied to the sensor. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++-- > 1 file changed, 51 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 418301b33a5e..f1329ffb0463 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > +static constexpr Size IPU3ViewfinderSize(1280, 720); > > static const ControlInfoMap::Map IPU3Controls = { > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > * capped to the maximum sensor resolution and aligned > * to the ImgU output constraints. > */ > - size = sensorResolution.boundedTo({ 1280, 720 }) > + size = sensorResolution.boundedTo(IPU3ViewfinderSize) An unrelated change, but a good one. Could you briefly mention it in the commit message ? > .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > IMGU_OUTPUT_HEIGHT_ALIGN); > pixelFormat = formats::NV12; > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data) > */ > int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > { > - const CameraSensor *sensor = data->cio2_.sensor(); > + CameraSensor *sensor = data->cio2_.sensor(); > CameraSensorInfo sensorInfo{}; > > int ret = sensor->sensorInfo(&sensorInfo); > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > defExposure); > > + /* > + * Compute the scaler crop limits. > + * > + * \todo The scaler crop limits depend on the sensor configuration. It > + * should be updated when a new configuration is applied. To initialize > + * the control use the 'Viewfinder' configuration (1280x720) as the > + * pipeline output resolution and the full sensor size as input frame > + * (see the todo note in the validation function). s/the validation function/IPU3CameraConfiguration::validate()/ I'm not sure what todo note you're referring too though. > + */ > + > + /* > + * The maximum scaler crop rectangle is the analogue crop used to > + * produce the maximum frame size. > + */ > + V4L2SubdeviceFormat sensorFormat; > + sensorFormat.size = sensor->resolution(); > + ret = sensor->setFormat(&sensorFormat); > + if (ret) > + return ret; > + > + /* Re-fetch the sensor info updated to use the largest resolution. */ > + ret = sensor->sensorInfo(&sensorInfo); > + if (ret) > + return ret; > + > + const Rectangle &analogueCrop = sensorInfo.analogCrop; > + Rectangle maxCrop = analogueCrop; > + > + /* > + * The minimum crop rectangle is the default viewfinder configuration > + * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel Just rescaled, or does the ImgU apply cropping along the pipeline ? If it does (or just if you're not sure), a \todo comment would be good. > + * array coordinates. As the ImgU cannot up-scale, the minimum selection > + * rectangle has to be as large as the desired pipeline output size. > + * > + * The top-left corner position is not relevant as the minimum crop > + * rectangle can be placed anywhere inside the analogue crop region. I would have sworn we had documented it as being (0,0) as a convention, but I don't see that in the control documentation. As it's not relevant we can set it to anything, but I believe we should standardize the behaviour. What do you think would be best, (0,0), the same (x,y) position as the maximium value, or centering the minimum rectangle in the maximum rectangle ? The code otherwise looks fine. > + */ > + const Size &sensorOutput = sensorInfo.outputSize; > + Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize) > + .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > + IMGU_OUTPUT_HEIGHT_ALIGN); > + Rectangle minCrop(minCropSize); > + minCrop.scaledBy(analogueCrop.size(), sensorOutput); > + minCrop.x = analogueCrop.x; > + minCrop.y = analogueCrop.y; > + > + controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); > + > data->controlInfo_ = std::move(controls); > > return 0;
Hi Laurent, On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote: > > Register the ScalerCrop control in the ImgU pipeline handler computed > > by using the Viewfinder [1280x720] pipeline output configuration and > > the sensor resolution as parameters. > > > > The ScalerCrop control limits should be updated everytime a new > > configuration is applied to the sensor. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++-- > > 1 file changed, 51 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 418301b33a5e..f1329ffb0463 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > +static constexpr Size IPU3ViewfinderSize(1280, 720); > > > > static const ControlInfoMap::Map IPU3Controls = { > > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > * capped to the maximum sensor resolution and aligned > > * to the ImgU output constraints. > > */ > > - size = sensorResolution.boundedTo({ 1280, 720 }) > > + size = sensorResolution.boundedTo(IPU3ViewfinderSize) > > An unrelated change, but a good one. Could you briefly mention it in the > commit message ? > Why do you think it's unrelated ? As I need the Viewfinder resolution to be used in multiple places, I just have defined it and used it here as well. > > .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > IMGU_OUTPUT_HEIGHT_ALIGN); > > pixelFormat = formats::NV12; > > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data) > > */ > > int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > { > > - const CameraSensor *sensor = data->cio2_.sensor(); > > + CameraSensor *sensor = data->cio2_.sensor(); > > CameraSensorInfo sensorInfo{}; > > > > int ret = sensor->sensorInfo(&sensorInfo); > > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > > defExposure); > > > > + /* > > + * Compute the scaler crop limits. > > + * > > + * \todo The scaler crop limits depend on the sensor configuration. It > > + * should be updated when a new configuration is applied. To initialize > > + * the control use the 'Viewfinder' configuration (1280x720) as the > > + * pipeline output resolution and the full sensor size as input frame > > + * (see the todo note in the validation function). > > s/the validation function/IPU3CameraConfiguration::validate()/ > > I'm not sure what todo note you're referring too though. The one that says we currently run with the full frame size as ImgU input. Otherwise, if a smaller frame is used, the limits should be updated accordingly. validate() : * \todo The image sensor frame size should be selected to optimize * operations based on the sizes of the requested streams. However such * a selection makes the pipeline configuration procedure fail for small * resolutions (for example: 640x480 with OV5670) and causes the capture * operations to stall for some stream size combinations (see the * commit message of the patch that introduced this comment for more * failure examples). * > > > + */ > > + > > + /* > > + * The maximum scaler crop rectangle is the analogue crop used to > > + * produce the maximum frame size. > > + */ > > + V4L2SubdeviceFormat sensorFormat; > > + sensorFormat.size = sensor->resolution(); > > + ret = sensor->setFormat(&sensorFormat); > > + if (ret) > > + return ret; > > + > > + /* Re-fetch the sensor info updated to use the largest resolution. */ > > + ret = sensor->sensorInfo(&sensorInfo); > > + if (ret) > > + return ret; > > + > > + const Rectangle &analogueCrop = sensorInfo.analogCrop; > > + Rectangle maxCrop = analogueCrop; > > + > > + /* > > + * The minimum crop rectangle is the default viewfinder configuration > > + * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel > > Just rescaled, or does the ImgU apply cropping along the pipeline ? If > it does (or just if you're not sure), a \todo comment would be good. > I read this question as: "do we need to take into account any additional pixels row/columns required by the ImgU for its processing" ? Am I right ? In this case I think it's plausible, as in validate() we apply a margin of 64 columns and 32 rows to the CIO2 output frame size to calculate the stream size limits. unsigned int limit; limit = utils::alignDown(cio2Configuration_.size.width - 1, IMGU_OUTPUT_WIDTH_MARGIN); cfg->size.width = std::clamp(cfg->size.width, IMGU_OUTPUT_MIN_SIZE.width, limit); limit = utils::alignDown(cio2Configuration_.size.height - 1, IMGU_OUTPUT_HEIGHT_MARGIN); cfg->size.height = std::clamp(cfg->size.height, IMGU_OUTPUT_MIN_SIZE.height, limit); cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN, IMGU_OUTPUT_HEIGHT_ALIGN); It might make sense to apply the same margins here by adding the same margins to the viewfinder sizes > > + * array coordinates. As the ImgU cannot up-scale, the minimum selection > > + * rectangle has to be as large as the desired pipeline output size. > > + * > > + * The top-left corner position is not relevant as the minimum crop > > + * rectangle can be placed anywhere inside the analogue crop region. > > I would have sworn we had documented it as being (0,0) as a convention, > but I don't see that in the control documentation. As it's not relevant > we can set it to anything, but I believe we should standardize the > behaviour. What do you think would be best, (0,0), the same (x,y) > position as the maximium value, or centering the minimum rectangle in > the maximum rectangle ? I guess it also depends on the platform capabilities. Some platforms might only be capable of center-zoom (I suspect so as Android has a property to report the zooming capabilities). Centering it would be safer option to me. does it make sense ? > > The code otherwise looks fine. > > > + */ > > + const Size &sensorOutput = sensorInfo.outputSize; > > + Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize) > > + .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > + IMGU_OUTPUT_HEIGHT_ALIGN); Also, aligning the ViewfinderSize downTo() might result in a size smaller than the input size. It does not happen as the viewfinder sizes are aligned already > > + Rectangle minCrop(minCropSize); > > + minCrop.scaledBy(analogueCrop.size(), sensorOutput); > > + minCrop.x = analogueCrop.x; > > + minCrop.y = analogueCrop.y; > > + > > + controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); > > + > > data->controlInfo_ = std::move(controls); > > > > return 0; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Jan 18, 2021 at 11:46:38AM +0100, Jacopo Mondi wrote: > On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote: > > On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote: > > > Register the ScalerCrop control in the ImgU pipeline handler computed > > > by using the Viewfinder [1280x720] pipeline output configuration and > > > the sensor resolution as parameters. > > > > > > The ScalerCrop control limits should be updated everytime a new > > > configuration is applied to the sensor. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++-- > > > 1 file changed, 51 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 418301b33a5e..f1329ffb0463 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; > > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > > +static constexpr Size IPU3ViewfinderSize(1280, 720); > > > > > > static const ControlInfoMap::Map IPU3Controls = { > > > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > > > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > * capped to the maximum sensor resolution and aligned > > > * to the ImgU output constraints. > > > */ > > > - size = sensorResolution.boundedTo({ 1280, 720 }) > > > + size = sensorResolution.boundedTo(IPU3ViewfinderSize) > > > > An unrelated change, but a good one. Could you briefly mention it in the > > commit message ? > > Why do you think it's unrelated ? As I need the Viewfinder resolution > to be used in multiple places, I just have defined it and used it here > as well. Not completely unrelated indeed. > > > .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > > IMGU_OUTPUT_HEIGHT_ALIGN); > > > pixelFormat = formats::NV12; > > > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data) > > > */ > > > int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > > { > > > - const CameraSensor *sensor = data->cio2_.sensor(); > > > + CameraSensor *sensor = data->cio2_.sensor(); > > > CameraSensorInfo sensorInfo{}; > > > > > > int ret = sensor->sensorInfo(&sensorInfo); > > > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > > > defExposure); > > > > > > + /* > > > + * Compute the scaler crop limits. > > > + * > > > + * \todo The scaler crop limits depend on the sensor configuration. It > > > + * should be updated when a new configuration is applied. To initialize > > > + * the control use the 'Viewfinder' configuration (1280x720) as the > > > + * pipeline output resolution and the full sensor size as input frame > > > + * (see the todo note in the validation function). > > > > s/the validation function/IPU3CameraConfiguration::validate()/ > > > > I'm not sure what todo note you're referring too though. > > The one that says we currently run with the full frame size as ImgU > input. Otherwise, if a smaller frame is used, the limits should be > updated accordingly. > > validate() : > > * \todo The image sensor frame size should be selected to optimize > * operations based on the sizes of the requested streams. However such > * a selection makes the pipeline configuration procedure fail for small > * resolutions (for example: 640x480 with OV5670) and causes the capture > * operations to stall for some stream size combinations (see the > * commit message of the patch that introduced this comment for more > * failure examples). > * Thanks. > > > + */ > > > + > > > + /* > > > + * The maximum scaler crop rectangle is the analogue crop used to > > > + * produce the maximum frame size. > > > + */ > > > + V4L2SubdeviceFormat sensorFormat; > > > + sensorFormat.size = sensor->resolution(); > > > + ret = sensor->setFormat(&sensorFormat); > > > + if (ret) > > > + return ret; > > > + > > > + /* Re-fetch the sensor info updated to use the largest resolution. */ > > > + ret = sensor->sensorInfo(&sensorInfo); > > > + if (ret) > > > + return ret; > > > + > > > + const Rectangle &analogueCrop = sensorInfo.analogCrop; > > > + Rectangle maxCrop = analogueCrop; > > > + > > > + /* > > > + * The minimum crop rectangle is the default viewfinder configuration > > > + * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel > > > > Just rescaled, or does the ImgU apply cropping along the pipeline ? If > > it does (or just if you're not sure), a \todo comment would be good. > > I read this question as: "do we need to take into account any > additional pixels row/columns required by the ImgU for its processing" ? > Am I right ? Yes that's right. > In this case I think it's plausible, as in validate() we apply a margin of > 64 columns and 32 rows to the CIO2 output frame size to calculate the stream > size limits. > > unsigned int limit; > limit = utils::alignDown(cio2Configuration_.size.width - 1, > IMGU_OUTPUT_WIDTH_MARGIN); > cfg->size.width = std::clamp(cfg->size.width, > IMGU_OUTPUT_MIN_SIZE.width, > limit); > > limit = utils::alignDown(cio2Configuration_.size.height - 1, > IMGU_OUTPUT_HEIGHT_MARGIN); > cfg->size.height = std::clamp(cfg->size.height, > IMGU_OUTPUT_MIN_SIZE.height, > limit); > > cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > IMGU_OUTPUT_HEIGHT_ALIGN); > > It might make sense to apply the same margins here by adding the same > margins to the viewfinder sizes Something needs to be done indeed. As we're not entirely sure what the hardware constraints are it's difficult to know for sure. We can start with a first implementation and a \todo comment to mention it has to be double-checked. > > > + * array coordinates. As the ImgU cannot up-scale, the minimum selection > > > + * rectangle has to be as large as the desired pipeline output size. > > > + * > > > + * The top-left corner position is not relevant as the minimum crop > > > + * rectangle can be placed anywhere inside the analogue crop region. > > > > I would have sworn we had documented it as being (0,0) as a convention, > > but I don't see that in the control documentation. As it's not relevant > > we can set it to anything, but I believe we should standardize the > > behaviour. What do you think would be best, (0,0), the same (x,y) > > position as the maximium value, or centering the minimum rectangle in > > the maximum rectangle ? > > I guess it also depends on the platform capabilities. Some platforms > might only be capable of center-zoom (I suspect so as Android has a > property to report the zooming capabilities). > > Centering it would be safer option to me. does it make sense ? Applications should really ignore the (x,y) offset so it shouldn't matter too much. Centering it seems a bit more logical, at the expense of a bit more complexity on the pipeline handler side. I don't mind much. Giving it a second though, and given your comment about centre-zoom, maybe the (x,y) offset could be used to indicate whether panning is possible. We could require (x,y) to be set to the smallest valid value, and when only centre-zoom is possible, the rectangle would then be centered. That way applications would know what type of zoom is possible. > > The code otherwise looks fine. > > > > > + */ > > > + const Size &sensorOutput = sensorInfo.outputSize; > > > + Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize) > > > + .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > > + IMGU_OUTPUT_HEIGHT_ALIGN); > > Also, aligning the ViewfinderSize downTo() might result in a size > smaller than the input size. It does not happen as the viewfinder > sizes are aligned already > > > > + Rectangle minCrop(minCropSize); > > > + minCrop.scaledBy(analogueCrop.size(), sensorOutput); > > > + minCrop.x = analogueCrop.x; > > > + minCrop.y = analogueCrop.y; > > > + > > > + controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); > > > + > > > data->controlInfo_ = std::move(controls); > > > > > > return 0;
Hi Laurent, On Tue, Jan 19, 2021 at 09:17:10AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Jan 18, 2021 at 11:46:38AM +0100, Jacopo Mondi wrote: > > On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote: > > > On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote: > > > > Register the ScalerCrop control in the ImgU pipeline handler computed > > > > by using the Viewfinder [1280x720] pipeline output configuration and > > > > the sensor resolution as parameters. > > > > > > > > The ScalerCrop control limits should be updated everytime a new > > > > configuration is applied to the sensor. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++-- > > > > 1 file changed, 51 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index 418301b33a5e..f1329ffb0463 100644 > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; > > > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > > > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > > > +static constexpr Size IPU3ViewfinderSize(1280, 720); > > > > > > > > static const ControlInfoMap::Map IPU3Controls = { > > > > { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, > > > > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > > * capped to the maximum sensor resolution and aligned > > > > * to the ImgU output constraints. > > > > */ > > > > - size = sensorResolution.boundedTo({ 1280, 720 }) > > > > + size = sensorResolution.boundedTo(IPU3ViewfinderSize) > > > > > > An unrelated change, but a good one. Could you briefly mention it in the > > > commit message ? > > > > Why do you think it's unrelated ? As I need the Viewfinder resolution > > to be used in multiple places, I just have defined it and used it here > > as well. > > Not completely unrelated indeed. > > > > > .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > > > IMGU_OUTPUT_HEIGHT_ALIGN); > > > > pixelFormat = formats::NV12; > > > > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data) > > > > */ > > > > int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > > > { > > > > - const CameraSensor *sensor = data->cio2_.sensor(); > > > > + CameraSensor *sensor = data->cio2_.sensor(); > > > > CameraSensorInfo sensorInfo{}; > > > > > > > > int ret = sensor->sensorInfo(&sensorInfo); > > > > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > > > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > > > > defExposure); > > > > > > > > + /* > > > > + * Compute the scaler crop limits. > > > > + * > > > > + * \todo The scaler crop limits depend on the sensor configuration. It > > > > + * should be updated when a new configuration is applied. To initialize > > > > + * the control use the 'Viewfinder' configuration (1280x720) as the > > > > + * pipeline output resolution and the full sensor size as input frame > > > > + * (see the todo note in the validation function). > > > > > > s/the validation function/IPU3CameraConfiguration::validate()/ > > > > > > I'm not sure what todo note you're referring too though. > > > > The one that says we currently run with the full frame size as ImgU > > input. Otherwise, if a smaller frame is used, the limits should be > > updated accordingly. > > > > validate() : > > > > * \todo The image sensor frame size should be selected to optimize > > * operations based on the sizes of the requested streams. However such > > * a selection makes the pipeline configuration procedure fail for small > > * resolutions (for example: 640x480 with OV5670) and causes the capture > > * operations to stall for some stream size combinations (see the > > * commit message of the patch that introduced this comment for more > > * failure examples). > > * > > Thanks. > > > > > + */ > > > > + > > > > + /* > > > > + * The maximum scaler crop rectangle is the analogue crop used to > > > > + * produce the maximum frame size. > > > > + */ > > > > + V4L2SubdeviceFormat sensorFormat; > > > > + sensorFormat.size = sensor->resolution(); > > > > + ret = sensor->setFormat(&sensorFormat); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* Re-fetch the sensor info updated to use the largest resolution. */ > > > > + ret = sensor->sensorInfo(&sensorInfo); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + const Rectangle &analogueCrop = sensorInfo.analogCrop; > > > > + Rectangle maxCrop = analogueCrop; > > > > + > > > > + /* > > > > + * The minimum crop rectangle is the default viewfinder configuration > > > > + * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel > > > > > > Just rescaled, or does the ImgU apply cropping along the pipeline ? If > > > it does (or just if you're not sure), a \todo comment would be good. > > > > I read this question as: "do we need to take into account any > > additional pixels row/columns required by the ImgU for its processing" ? > > Am I right ? > > Yes that's right. > > > In this case I think it's plausible, as in validate() we apply a margin of > > 64 columns and 32 rows to the CIO2 output frame size to calculate the stream > > size limits. > > > > unsigned int limit; > > limit = utils::alignDown(cio2Configuration_.size.width - 1, > > IMGU_OUTPUT_WIDTH_MARGIN); > > cfg->size.width = std::clamp(cfg->size.width, > > IMGU_OUTPUT_MIN_SIZE.width, > > limit); > > > > limit = utils::alignDown(cio2Configuration_.size.height - 1, > > IMGU_OUTPUT_HEIGHT_MARGIN); > > cfg->size.height = std::clamp(cfg->size.height, > > IMGU_OUTPUT_MIN_SIZE.height, > > limit); > > > > cfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > IMGU_OUTPUT_HEIGHT_ALIGN); > > > > It might make sense to apply the same margins here by adding the same > > margins to the viewfinder sizes > > Something needs to be done indeed. As we're not entirely sure what the > hardware constraints are it's difficult to know for sure. We can start > with a first implementation and a \todo comment to mention it has to be > double-checked. > I reworked the implementation and made sure the calculated minimum rectangle gives me a size that is suitable for the pipeline configuration tool, so I hope I'm on the right track. > > > > + * array coordinates. As the ImgU cannot up-scale, the minimum selection > > > > + * rectangle has to be as large as the desired pipeline output size. > > > > + * > > > > + * The top-left corner position is not relevant as the minimum crop > > > > + * rectangle can be placed anywhere inside the analogue crop region. > > > > > > I would have sworn we had documented it as being (0,0) as a convention, > > > but I don't see that in the control documentation. As it's not relevant > > > we can set it to anything, but I believe we should standardize the > > > behaviour. What do you think would be best, (0,0), the same (x,y) > > > position as the maximium value, or centering the minimum rectangle in > > > the maximum rectangle ? > > > > I guess it also depends on the platform capabilities. Some platforms > > might only be capable of center-zoom (I suspect so as Android has a > > property to report the zooming capabilities). > > > > Centering it would be safer option to me. does it make sense ? > > Applications should really ignore the (x,y) offset so it shouldn't > matter too much. Centering it seems a bit more logical, at the expense > of a bit more complexity on the pipeline handler side. I don't mind > much. > > Giving it a second though, and given your comment about centre-zoom, > maybe the (x,y) offset could be used to indicate whether panning is > possible. We could require (x,y) to be set to the smallest valid value, > and when only centre-zoom is possible, the rectangle would then be > centered. That way applications would know what type of zoom is > possible. > I agree that (0,0) might be used to indicate it is possible to freely pan in the largest crop rectangle. We're free to decide what to do, as currently the ScalerCrop control limits are not used by regular applications but just by the HAL to calculate the digital zoom limits. Otherwise the ScalerCropMaximum property is used. Once we move to updating the ControlInfo limits per each configuration and drop ScalerCropMaximum we'll have to describe the policy we have chosen in the control documentation. > > > The code otherwise looks fine. > > > > > > > + */ > > > > + const Size &sensorOutput = sensorInfo.outputSize; > > > > + Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize) > > > > + .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, > > > > + IMGU_OUTPUT_HEIGHT_ALIGN); > > > > Also, aligning the ViewfinderSize downTo() might result in a size > > smaller than the input size. It does not happen as the viewfinder > > sizes are aligned already > > > > > > + Rectangle minCrop(minCropSize); > > > > + minCrop.scaledBy(analogueCrop.size(), sensorOutput); > > > > + minCrop.x = analogueCrop.x; > > > > + minCrop.y = analogueCrop.y; > > > > + > > > > + controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); > > > > + > > > > data->controlInfo_ = std::move(controls); > > > > > > > > return 0; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 418301b33a5e..f1329ffb0463 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; +static constexpr Size IPU3ViewfinderSize(1280, 720); static const ControlInfoMap::Map IPU3Controls = { { &controls::draft::PipelineDepth, ControlInfo(2, 3) }, @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, * capped to the maximum sensor resolution and aligned * to the ImgU output constraints. */ - size = sensorResolution.boundedTo({ 1280, 720 }) + size = sensorResolution.boundedTo(IPU3ViewfinderSize) .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, IMGU_OUTPUT_HEIGHT_ALIGN); pixelFormat = formats::NV12; @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data) */ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) { - const CameraSensor *sensor = data->cio2_.sensor(); + CameraSensor *sensor = data->cio2_.sensor(); CameraSensorInfo sensorInfo{}; int ret = sensor->sensorInfo(&sensorInfo); @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, defExposure); + /* + * Compute the scaler crop limits. + * + * \todo The scaler crop limits depend on the sensor configuration. It + * should be updated when a new configuration is applied. To initialize + * the control use the 'Viewfinder' configuration (1280x720) as the + * pipeline output resolution and the full sensor size as input frame + * (see the todo note in the validation function). + */ + + /* + * The maximum scaler crop rectangle is the analogue crop used to + * produce the maximum frame size. + */ + V4L2SubdeviceFormat sensorFormat; + sensorFormat.size = sensor->resolution(); + ret = sensor->setFormat(&sensorFormat); + if (ret) + return ret; + + /* Re-fetch the sensor info updated to use the largest resolution. */ + ret = sensor->sensorInfo(&sensorInfo); + if (ret) + return ret; + + const Rectangle &analogueCrop = sensorInfo.analogCrop; + Rectangle maxCrop = analogueCrop; + + /* + * The minimum crop rectangle is the default viewfinder configuration + * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel + * array coordinates. As the ImgU cannot up-scale, the minimum selection + * rectangle has to be as large as the desired pipeline output size. + * + * The top-left corner position is not relevant as the minimum crop + * rectangle can be placed anywhere inside the analogue crop region. + */ + const Size &sensorOutput = sensorInfo.outputSize; + Size minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize) + .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN, + IMGU_OUTPUT_HEIGHT_ALIGN); + Rectangle minCrop(minCropSize); + minCrop.scaledBy(analogueCrop.size(), sensorOutput); + minCrop.x = analogueCrop.x; + minCrop.y = analogueCrop.y; + + controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); + data->controlInfo_ = std::move(controls); return 0;
Register the ScalerCrop control in the ImgU pipeline handler computed by using the Viewfinder [1280x720] pipeline output configuration and the sensor resolution as parameters. The ScalerCrop control limits should be updated everytime a new configuration is applied to the sensor. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-)