| Message ID | 20201127151704.22034-1-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo Thanks for tackling this one again, glad it's not me! Just one or two minor observations, and one more significant thing. On Fri, 27 Nov 2020 at 15:17, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Currently the properties::ScalerCropMaximum property reports the maximum > valid Rectangle for the controls::ScalerCrop control, which can be > combined with the sensor pixel array size in order to obtain the minimum > available zoom factor. > > It is also useful to be able to calculate the maximum available zoom > factor, and in order to do so the minimum valid crop rectangle has to > be reported. > > Transform the ScalerCropMaximum property in ScalerCropLimits, which > reports both the maximum and minimum crop limits and port the only user > of such a property (Raspberry Pi) to use it. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/control_ids.yaml | 2 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++--- > src/libcamera/property_ids.yaml | 38 ++++++++++++++----- > 3 files changed, 38 insertions(+), 16 deletions(-) > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index c8874fa91965..11623fc56589 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -528,6 +528,6 @@ controls: > a binning or skipping mode. > > This control is only present when the pipeline supports scaling. Its > - maximum valid value is given by the properties::ScalerCropMaximum > + valid value limits are given by the properties::ScalerCropLimits > property, and the two can be used to implement digital zoom. > ... > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 00a40c558bfa..cac88ca65ba5 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > /* > - * Update the ScalerCropMaximum to the correct value for this camera mode. > - * For us, it's the same as the "analogue crop". > + * Update the ScalerCropLimits to the correct values for this camera > + * mode. For us, it's the same as the "analogue crop" and the ISP > + * minimum input size. > * > * \todo Make this property the ScalerCrop maximum value when dynamic > * controls are available and set it at validate() time > */ > - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop); > + data->properties_.set(properties::ScalerCropLimits, > + { data->sensorInfo_.analogCrop, > + Rectangle(data->ispMinCropSize_) }); So I think there might be a problem here, which is that ispMinCropSize_ is in units of the sensor output pixels (which might be binned), and the property wants them in sensor native pixels. Therefore we'll need to transform them up to the analog_crop size, maybe something like this: Rectangle sensorMinCrop = Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(), sensorFormat.size); data->properties_.set(properties::ScalerCropLimits, { data->sensorInfo_.analogCrop, sensorMinCrop.size() }); but I haven't tried it and it's probably full of typos. A little part of me also wonders what the offset, sorry, *top left* of the minimum Rectangle means. Presumably it doesn't mean anything? > > return ret; > } > @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > data->properties_ = data->sensor_->properties(); > > /* > - * Set a default value for the ScalerCropMaximum property to show > + * Set default values for the ScalerCropLimits property to show > * that we support its use, however, initialise it to zero because > * it's not meaningful until a camera mode has been chosen. > */ > - data->properties_.set(properties::ScalerCropMaximum, Rectangle{}); > + data->properties_.set(properties::ScalerCropLimits, > + { Rectangle{}, Rectangle{} }); > > /* > * We cache three things about the sensor in relation to transforms > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > index 64e88f0361d6..9a3ffcd05eeb 100644 > --- a/src/libcamera/property_ids.yaml > +++ b/src/libcamera/property_ids.yaml > @@ -663,19 +663,37 @@ controls: > \todo Rename this property to ActiveAreas once we will have property > categories (i.e. Properties::PixelArray::ActiveAreas) > > - - ScalerCropMaximum: > + - ScalerCropLimits: > type: Rectangle > + size: [2] > description: | > - The maximum valid rectangle for the controls::ScalerCrop control. This > - reflects the minimum mandatory cropping applied in the camera sensor and > - the rest of the pipeline. Just as the ScalerCrop control, it defines a > - rectangle taken from the sensor's active pixel array. > - > - This property is valid only after the camera has been successfully > - configured and its value may change whenever a new configuration is > + The maximum and minimum valid rectangles for the controls::ScalerCrop > + control, specified in this order. > + > + This two rectangles respectively reflect the minimum and maximum Should this be "maximum and minimum" rather than "minimum and maximum", having just previously described the order like that? > + mandatory cropping applied in the camera sensor and the rest of the > + pipeline. Both Rectangles are defined relatively to the sensor's active > + pixel array (properties::PixelArrayActiveAreas). Do we need to mention that the top left of the minimum doesn't mean anything? > + > + Implementation details for the maximum valid crop rectangle. > + The maximum valid crop rectangle depends on the camera configuration as > + pipelines are free to adjust the frame size requested from the sensor > + and used to produced the final image streams. The largest possible crop s/produced/produce/ > + rectangle is then limited by the size of the sensor's active pixel area > + portion used to produce the sensor output frame. > + > + Implementation details for the minimum valid crop rectangle. > + If a pipeline cannot up-scale the minimum valid crop rectangle is equal I think you need a comma after "up-scale" (my internal language parser crashed when I reached "is equal"!) > + to the smallest available stream resolution (plus any border pixels > + required by the ISP for image processing) part of the current camera > + configuration. If a pipeline can up-scale, the minimum valid crop Ah, it's right here! :) Thanks again, and best regards David > + rectangle is equal to the smallest frame size accepted by the ISP input. > + > + The Rectangles are valid only after the camera has been successfully > + configured and their value may change whenever a new configuration is > applied. > > - \todo Turn this property into a "maximum control value" for the > - ScalerCrop control once "dynamic" controls have been implemented. > + \todo Express this property as the limits for the ScalerCrop control > + once "dynamic" controls have been implemented. > > ... > -- > 2.29.1 >
Hi David, On Fri, Nov 27, 2020 at 04:53:22PM +0000, David Plowman wrote: > Hi Jacopo > > Thanks for tackling this one again, glad it's not me! Just one or two > minor observations, and one more significant thing. > Thanks for your precious inputs > On Fri, 27 Nov 2020 at 15:17, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Currently the properties::ScalerCropMaximum property reports the maximum > > valid Rectangle for the controls::ScalerCrop control, which can be > > combined with the sensor pixel array size in order to obtain the minimum > > available zoom factor. > > > > It is also useful to be able to calculate the maximum available zoom > > factor, and in order to do so the minimum valid crop rectangle has to > > be reported. > > > > Transform the ScalerCropMaximum property in ScalerCropLimits, which > > reports both the maximum and minimum crop limits and port the only user > > of such a property (Raspberry Pi) to use it. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/control_ids.yaml | 2 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++--- > > src/libcamera/property_ids.yaml | 38 ++++++++++++++----- > > 3 files changed, 38 insertions(+), 16 deletions(-) > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index c8874fa91965..11623fc56589 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -528,6 +528,6 @@ controls: > > a binning or skipping mode. > > > > This control is only present when the pipeline supports scaling. Its > > - maximum valid value is given by the properties::ScalerCropMaximum > > + valid value limits are given by the properties::ScalerCropLimits > > property, and the two can be used to implement digital zoom. > > ... > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 00a40c558bfa..cac88ca65ba5 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > > > /* > > - * Update the ScalerCropMaximum to the correct value for this camera mode. > > - * For us, it's the same as the "analogue crop". > > + * Update the ScalerCropLimits to the correct values for this camera > > + * mode. For us, it's the same as the "analogue crop" and the ISP > > + * minimum input size. > > * > > * \todo Make this property the ScalerCrop maximum value when dynamic > > * controls are available and set it at validate() time > > */ > > - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop); > > + data->properties_.set(properties::ScalerCropLimits, > > + { data->sensorInfo_.analogCrop, > > + Rectangle(data->ispMinCropSize_) }); > > So I think there might be a problem here, which is that > ispMinCropSize_ is in units of the sensor output pixels (which might > be binned), and the property wants them in sensor native pixels. > Therefore we'll need to transform them up to the analog_crop size, > maybe something like this: Mmmm, I made the resoning that the ISP input was an absolute value. Ie if your ISP does not accept anything smaller than 100x100, you can't specify a rectangle smaller than this on the sensor pixel array. But I get what you mean here: with a 2x2 binning in use, selecting a (theoretically valid) 100x100 rectangle on the pixel array will give you an output size of 50x50 (I presume it's more complex than this, but plese bear with me with this simplified example). > > Rectangle sensorMinCrop = > Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(), > sensorFormat.size); I think the idea is right. I wonder if we should not use sensorInfo_.ouputSize and not sensorFormat.size, as the latter is the format to be applied to unicam > data->properties_.set(properties::ScalerCropLimits, { > data->sensorInfo_.analogCrop, sensorMinCrop.size() }); > > but I haven't tried it and it's probably full of typos. > > A little part of me also wonders what the offset, sorry, *top left* of > the minimum Rectangle means. Presumably it doesn't mean anything? Yes, the min limit would be better expressed as a Size, but I liked the symmetry of having two rectangles. I should better specify the top-left corner position is not relevant ? > > > > > return ret; > > } > > @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > data->properties_ = data->sensor_->properties(); > > > > /* > > - * Set a default value for the ScalerCropMaximum property to show > > + * Set default values for the ScalerCropLimits property to show > > * that we support its use, however, initialise it to zero because > > * it's not meaningful until a camera mode has been chosen. > > */ > > - data->properties_.set(properties::ScalerCropMaximum, Rectangle{}); > > + data->properties_.set(properties::ScalerCropLimits, > > + { Rectangle{}, Rectangle{} }); > > > > /* > > * We cache three things about the sensor in relation to transforms > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index 64e88f0361d6..9a3ffcd05eeb 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -663,19 +663,37 @@ controls: > > \todo Rename this property to ActiveAreas once we will have property > > categories (i.e. Properties::PixelArray::ActiveAreas) > > > > - - ScalerCropMaximum: > > + - ScalerCropLimits: > > type: Rectangle > > + size: [2] > > description: | > > - The maximum valid rectangle for the controls::ScalerCrop control. This > > - reflects the minimum mandatory cropping applied in the camera sensor and > > - the rest of the pipeline. Just as the ScalerCrop control, it defines a > > - rectangle taken from the sensor's active pixel array. > > - > > - This property is valid only after the camera has been successfully > > - configured and its value may change whenever a new configuration is > > + The maximum and minimum valid rectangles for the controls::ScalerCrop > > + control, specified in this order. > > + > > + This two rectangles respectively reflect the minimum and maximum > > Should this be "maximum and minimum" rather than "minimum and > maximum", having just previously described the order like that? Mmm, what I meant was: ScalerCropLimits[0] = larger valid ScalerCrop rectangle = minimum possible zoom/crop ScalerCropLimits[1] = smaller valid ScalerCrop rectangle = maximum possible zoom/crop Would it be less confusing if in the text description I use "The larger and smaller valid rectangles" in place of "The maximum and minimum valid rectangles" ? > > > + mandatory cropping applied in the camera sensor and the rest of the > > + pipeline. Both Rectangles are defined relatively to the sensor's active > > + pixel array (properties::PixelArrayActiveAreas). > > Do we need to mention that the top left of the minimum doesn't mean anything? > Yes we should make it clear. Or use a Size, but I like these two being rectangles part of a single property. What do you think ? > > + > > + Implementation details for the maximum valid crop rectangle. > > + The maximum valid crop rectangle depends on the camera configuration as > > + pipelines are free to adjust the frame size requested from the sensor > > + and used to produced the final image streams. The largest possible crop > > s/produced/produce/ > > > + rectangle is then limited by the size of the sensor's active pixel area > > + portion used to produce the sensor output frame. > > + > > + Implementation details for the minimum valid crop rectangle. > > + If a pipeline cannot up-scale the minimum valid crop rectangle is equal > > I think you need a comma after "up-scale" (my internal language parser > crashed when I reached "is equal"!) > Ah yes indeed. > > + to the smallest available stream resolution (plus any border pixels > > + required by the ISP for image processing) part of the current camera > > + configuration. If a pipeline can up-scale, the minimum valid crop > > Ah, it's right here! :) > > Thanks again, and best regards Thank you. From the top of your mind, as you know your platform better than me, can you suggest a configuration (for imx219) that would let me test the correctness of the reported limits ? I'm thinking about a mode that uses binning and where the lower limit has to be scaled back to the sensor's native pixel array coordinates. Thanks j > David > > > + rectangle is equal to the smallest frame size accepted by the ISP input. > > + > > + The Rectangles are valid only after the camera has been successfully > > + configured and their value may change whenever a new configuration is > > applied. > > > > - \todo Turn this property into a "maximum control value" for the > > - ScalerCrop control once "dynamic" controls have been implemented. > > + \todo Express this property as the limits for the ScalerCrop control > > + once "dynamic" controls have been implemented. > > > > ... > > -- > > 2.29.1 > >
Hi again Jacopo Thanks for the update! On Sat, 28 Nov 2020 at 10:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Fri, Nov 27, 2020 at 04:53:22PM +0000, David Plowman wrote: > > Hi Jacopo > > > > Thanks for tackling this one again, glad it's not me! Just one or two > > minor observations, and one more significant thing. > > > > Thanks for your precious inputs > > > On Fri, 27 Nov 2020 at 15:17, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Currently the properties::ScalerCropMaximum property reports the maximum > > > valid Rectangle for the controls::ScalerCrop control, which can be > > > combined with the sensor pixel array size in order to obtain the minimum > > > available zoom factor. > > > > > > It is also useful to be able to calculate the maximum available zoom > > > factor, and in order to do so the minimum valid crop rectangle has to > > > be reported. > > > > > > Transform the ScalerCropMaximum property in ScalerCropLimits, which > > > reports both the maximum and minimum crop limits and port the only user > > > of such a property (Raspberry Pi) to use it. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/control_ids.yaml | 2 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++--- > > > src/libcamera/property_ids.yaml | 38 ++++++++++++++----- > > > 3 files changed, 38 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > index c8874fa91965..11623fc56589 100644 > > > --- a/src/libcamera/control_ids.yaml > > > +++ b/src/libcamera/control_ids.yaml > > > @@ -528,6 +528,6 @@ controls: > > > a binning or skipping mode. > > > > > > This control is only present when the pipeline supports scaling. Its > > > - maximum valid value is given by the properties::ScalerCropMaximum > > > + valid value limits are given by the properties::ScalerCropLimits > > > property, and the two can be used to implement digital zoom. > > > ... > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 00a40c558bfa..cac88ca65ba5 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > > > > > /* > > > - * Update the ScalerCropMaximum to the correct value for this camera mode. > > > - * For us, it's the same as the "analogue crop". > > > + * Update the ScalerCropLimits to the correct values for this camera > > > + * mode. For us, it's the same as the "analogue crop" and the ISP > > > + * minimum input size. > > > * > > > * \todo Make this property the ScalerCrop maximum value when dynamic > > > * controls are available and set it at validate() time > > > */ > > > - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop); > > > + data->properties_.set(properties::ScalerCropLimits, > > > + { data->sensorInfo_.analogCrop, > > > + Rectangle(data->ispMinCropSize_) }); > > > > So I think there might be a problem here, which is that > > ispMinCropSize_ is in units of the sensor output pixels (which might > > be binned), and the property wants them in sensor native pixels. > > Therefore we'll need to transform them up to the analog_crop size, > > maybe something like this: > > Mmmm, I made the resoning that the ISP input was an absolute value. > Ie if your ISP does not accept anything smaller than 100x100, you > can't specify a rectangle smaller than this on the sensor pixel array. > > But I get what you mean here: with a 2x2 binning in use, selecting a > (theoretically valid) 100x100 rectangle on the pixel array will give > you an output size of 50x50 (I presume it's more complex than this, > but plese bear with me with this simplified example). > > > > > Rectangle sensorMinCrop = > > Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(), > > sensorFormat.size); > > I think the idea is right. > I wonder if we should not use sensorInfo_.ouputSize and not > sensorFormat.size, as the latter is the format to be applied to unicam The important thing is to recover the scaling factors in the sensor, that is, how many input pixels does one output pixel correspond to? So I think what you've said sounds correct, as we don't want unicam to have any effect. > > > data->properties_.set(properties::ScalerCropLimits, { > > data->sensorInfo_.analogCrop, sensorMinCrop.size() }); > > > > but I haven't tried it and it's probably full of typos. > > > > A little part of me also wonders what the offset, sorry, *top left* of > > the minimum Rectangle means. Presumably it doesn't mean anything? > > Yes, the min limit would be better expressed as a Size, but I liked > the symmetry of having two rectangles. I should better specify the > top-left corner position is not relevant ? Well, maybe just a mention somewhere, just to calm down people like me! > > > > > > > > > return ret; > > > } > > > @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > data->properties_ = data->sensor_->properties(); > > > > > > /* > > > - * Set a default value for the ScalerCropMaximum property to show > > > + * Set default values for the ScalerCropLimits property to show > > > * that we support its use, however, initialise it to zero because > > > * it's not meaningful until a camera mode has been chosen. > > > */ > > > - data->properties_.set(properties::ScalerCropMaximum, Rectangle{}); > > > + data->properties_.set(properties::ScalerCropLimits, > > > + { Rectangle{}, Rectangle{} }); > > > > > > /* > > > * We cache three things about the sensor in relation to transforms > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > > index 64e88f0361d6..9a3ffcd05eeb 100644 > > > --- a/src/libcamera/property_ids.yaml > > > +++ b/src/libcamera/property_ids.yaml > > > @@ -663,19 +663,37 @@ controls: > > > \todo Rename this property to ActiveAreas once we will have property > > > categories (i.e. Properties::PixelArray::ActiveAreas) > > > > > > - - ScalerCropMaximum: > > > + - ScalerCropLimits: > > > type: Rectangle > > > + size: [2] > > > description: | > > > - The maximum valid rectangle for the controls::ScalerCrop control. This > > > - reflects the minimum mandatory cropping applied in the camera sensor and > > > - the rest of the pipeline. Just as the ScalerCrop control, it defines a > > > - rectangle taken from the sensor's active pixel array. > > > - > > > - This property is valid only after the camera has been successfully > > > - configured and its value may change whenever a new configuration is > > > + The maximum and minimum valid rectangles for the controls::ScalerCrop > > > + control, specified in this order. > > > + > > > + This two rectangles respectively reflect the minimum and maximum > > > > Should this be "maximum and minimum" rather than "minimum and > > maximum", having just previously described the order like that? > > Mmm, what I meant was: > ScalerCropLimits[0] = larger valid ScalerCrop rectangle > = minimum possible zoom/crop > ScalerCropLimits[1] = smaller valid ScalerCrop rectangle > = maximum possible zoom/crop > > Would it be less confusing if in the text description I use > "The larger and smaller valid rectangles" in place of > "The maximum and minimum valid rectangles" ? > > > > > > + mandatory cropping applied in the camera sensor and the rest of the > > > + pipeline. Both Rectangles are defined relatively to the sensor's active > > > + pixel array (properties::PixelArrayActiveAreas). > > > > Do we need to mention that the top left of the minimum doesn't mean anything? > > > > Yes we should make it clear. > > Or use a Size, but I like these two being rectangles part of a single > property. What do you think ? I agree. Having both as rectangles in the same property feels nice. That meaningless top left coordinate is just a bit weird, but I guess we don't worry about it! > > > > + > > > + Implementation details for the maximum valid crop rectangle. > > > + The maximum valid crop rectangle depends on the camera configuration as > > > + pipelines are free to adjust the frame size requested from the sensor > > > + and used to produced the final image streams. The largest possible crop > > > > s/produced/produce/ > > > > > + rectangle is then limited by the size of the sensor's active pixel area > > > + portion used to produce the sensor output frame. > > > + > > > + Implementation details for the minimum valid crop rectangle. > > > + If a pipeline cannot up-scale the minimum valid crop rectangle is equal > > > > I think you need a comma after "up-scale" (my internal language parser > > crashed when I reached "is equal"!) > > > > Ah yes indeed. > > > > + to the smallest available stream resolution (plus any border pixels > > > + required by the ISP for image processing) part of the current camera > > > + configuration. If a pipeline can up-scale, the minimum valid crop > > > > Ah, it's right here! :) > > > > Thanks again, and best regards > > Thank you. > > From the top of your mind, as you know your platform better than me, > can you suggest a configuration (for imx219) that would let me test > the correctness of the reported limits ? I'm thinking about a mode > that uses binning and where the lower limit has to be scaled back to > the sensor's native pixel array coordinates. So I believe all our sensors (ov5647, imx219, imx477) will run in 2x2 binning mode when started just by typing "qcam". So, as the minimum crop size is 64x64 into the ISP, we should get 128x128. If you run "qcam -s role=viewfinder -s role=raw" we should get the full resolution, so the rectangle should come out as 64x64. (I do wonder whether we should be placing a restriction of, say, 16x, for upscaling in our pipeline for other reasons - but I think that's our business and doesn't affect the discussion here.) Thanks and best regards David > > Thanks > j > > > David > > > > > + rectangle is equal to the smallest frame size accepted by the ISP input. > > > + > > > + The Rectangles are valid only after the camera has been successfully > > > + configured and their value may change whenever a new configuration is > > > applied. > > > > > > - \todo Turn this property into a "maximum control value" for the > > > - ScalerCrop control once "dynamic" controls have been implemented. > > > + \todo Express this property as the limits for the ScalerCrop control > > > + once "dynamic" controls have been implemented. > > > > > > ... > > > -- > > > 2.29.1 > > >
Hi David, On Mon, Nov 30, 2020 at 09:42:54AM +0000, David Plowman wrote: > Hi again Jacopo > > Thanks for the update! > > On Sat, 28 Nov 2020 at 10:26, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David, > > > > On Fri, Nov 27, 2020 at 04:53:22PM +0000, David Plowman wrote: > > > Hi Jacopo > > > > > > Thanks for tackling this one again, glad it's not me! Just one or two > > > minor observations, and one more significant thing. > > > > > > > Thanks for your precious inputs > > > > > On Fri, 27 Nov 2020 at 15:17, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Currently the properties::ScalerCropMaximum property reports the maximum > > > > valid Rectangle for the controls::ScalerCrop control, which can be > > > > combined with the sensor pixel array size in order to obtain the minimum > > > > available zoom factor. > > > > > > > > It is also useful to be able to calculate the maximum available zoom > > > > factor, and in order to do so the minimum valid crop rectangle has to > > > > be reported. > > > > > > > > Transform the ScalerCropMaximum property in ScalerCropLimits, which > > > > reports both the maximum and minimum crop limits and port the only user > > > > of such a property (Raspberry Pi) to use it. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/control_ids.yaml | 2 +- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++--- > > > > src/libcamera/property_ids.yaml | 38 ++++++++++++++----- > > > > 3 files changed, 38 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > > > index c8874fa91965..11623fc56589 100644 > > > > --- a/src/libcamera/control_ids.yaml > > > > +++ b/src/libcamera/control_ids.yaml > > > > @@ -528,6 +528,6 @@ controls: > > > > a binning or skipping mode. > > > > > > > > This control is only present when the pipeline supports scaling. Its > > > > - maximum valid value is given by the properties::ScalerCropMaximum > > > > + valid value limits are given by the properties::ScalerCropLimits > > > > property, and the two can be used to implement digital zoom. > > > > ... > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 00a40c558bfa..cac88ca65ba5 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > > LOG(RPI, Error) << "Failed to configure the IPA: " << ret; > > > > > > > > /* > > > > - * Update the ScalerCropMaximum to the correct value for this camera mode. > > > > - * For us, it's the same as the "analogue crop". > > > > + * Update the ScalerCropLimits to the correct values for this camera > > > > + * mode. For us, it's the same as the "analogue crop" and the ISP > > > > + * minimum input size. > > > > * > > > > * \todo Make this property the ScalerCrop maximum value when dynamic > > > > * controls are available and set it at validate() time > > > > */ > > > > - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop); > > > > + data->properties_.set(properties::ScalerCropLimits, > > > > + { data->sensorInfo_.analogCrop, > > > > + Rectangle(data->ispMinCropSize_) }); > > > > > > So I think there might be a problem here, which is that > > > ispMinCropSize_ is in units of the sensor output pixels (which might > > > be binned), and the property wants them in sensor native pixels. > > > Therefore we'll need to transform them up to the analog_crop size, > > > maybe something like this: > > > > Mmmm, I made the resoning that the ISP input was an absolute value. > > Ie if your ISP does not accept anything smaller than 100x100, you > > can't specify a rectangle smaller than this on the sensor pixel array. > > > > But I get what you mean here: with a 2x2 binning in use, selecting a > > (theoretically valid) 100x100 rectangle on the pixel array will give > > you an output size of 50x50 (I presume it's more complex than this, > > but plese bear with me with this simplified example). > > > > > > > > Rectangle sensorMinCrop = > > > Rectangle(data->ispMinCropSize_).scaledBy(data->sensorInfo_.analogCrop.size(), > > > sensorFormat.size); > > > > I think the idea is right. > > I wonder if we should not use sensorInfo_.ouputSize and not > > sensorFormat.size, as the latter is the format to be applied to unicam > > The important thing is to recover the scaling factors in the sensor, > that is, how many input pixels does one output pixel correspond to? So > I think what you've said sounds correct, as we don't want unicam to > have any effect. > > > > > > data->properties_.set(properties::ScalerCropLimits, { > > > data->sensorInfo_.analogCrop, sensorMinCrop.size() }); > > > > > > but I haven't tried it and it's probably full of typos. > > > > > > A little part of me also wonders what the offset, sorry, *top left* of > > > the minimum Rectangle means. Presumably it doesn't mean anything? > > > > Yes, the min limit would be better expressed as a Size, but I liked > > the symmetry of having two rectangles. I should better specify the > > top-left corner position is not relevant ? > > Well, maybe just a mention somewhere, just to calm down people like me! > > > > > > > > > > > > > > return ret; > > > > } > > > > @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > > data->properties_ = data->sensor_->properties(); > > > > > > > > /* > > > > - * Set a default value for the ScalerCropMaximum property to show > > > > + * Set default values for the ScalerCropLimits property to show > > > > * that we support its use, however, initialise it to zero because > > > > * it's not meaningful until a camera mode has been chosen. > > > > */ > > > > - data->properties_.set(properties::ScalerCropMaximum, Rectangle{}); > > > > + data->properties_.set(properties::ScalerCropLimits, > > > > + { Rectangle{}, Rectangle{} }); > > > > > > > > /* > > > > * We cache three things about the sensor in relation to transforms > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > > > index 64e88f0361d6..9a3ffcd05eeb 100644 > > > > --- a/src/libcamera/property_ids.yaml > > > > +++ b/src/libcamera/property_ids.yaml > > > > @@ -663,19 +663,37 @@ controls: > > > > \todo Rename this property to ActiveAreas once we will have property > > > > categories (i.e. Properties::PixelArray::ActiveAreas) > > > > > > > > - - ScalerCropMaximum: > > > > + - ScalerCropLimits: > > > > type: Rectangle > > > > + size: [2] > > > > description: | > > > > - The maximum valid rectangle for the controls::ScalerCrop control. This > > > > - reflects the minimum mandatory cropping applied in the camera sensor and > > > > - the rest of the pipeline. Just as the ScalerCrop control, it defines a > > > > - rectangle taken from the sensor's active pixel array. > > > > - > > > > - This property is valid only after the camera has been successfully > > > > - configured and its value may change whenever a new configuration is > > > > + The maximum and minimum valid rectangles for the controls::ScalerCrop > > > > + control, specified in this order. > > > > + > > > > + This two rectangles respectively reflect the minimum and maximum > > > > > > Should this be "maximum and minimum" rather than "minimum and > > > maximum", having just previously described the order like that? > > > > Mmm, what I meant was: > > ScalerCropLimits[0] = larger valid ScalerCrop rectangle > > = minimum possible zoom/crop > > ScalerCropLimits[1] = smaller valid ScalerCrop rectangle > > = maximum possible zoom/crop > > > > Would it be less confusing if in the text description I use > > "The larger and smaller valid rectangles" in place of > > "The maximum and minimum valid rectangles" ? > > > > > > > > > + mandatory cropping applied in the camera sensor and the rest of the > > > > + pipeline. Both Rectangles are defined relatively to the sensor's active > > > > + pixel array (properties::PixelArrayActiveAreas). > > > > > > Do we need to mention that the top left of the minimum doesn't mean anything? > > > > > > > Yes we should make it clear. > > > > Or use a Size, but I like these two being rectangles part of a single > > property. What do you think ? > > I agree. Having both as rectangles in the same property feels nice. > That meaningless top left coordinate is just a bit weird, but I guess > we don't worry about it! > > > > > > > + > > > > + Implementation details for the maximum valid crop rectangle. > > > > + The maximum valid crop rectangle depends on the camera configuration as > > > > + pipelines are free to adjust the frame size requested from the sensor > > > > + and used to produced the final image streams. The largest possible crop > > > > > > s/produced/produce/ > > > > > > > + rectangle is then limited by the size of the sensor's active pixel area > > > > + portion used to produce the sensor output frame. > > > > + > > > > + Implementation details for the minimum valid crop rectangle. > > > > + If a pipeline cannot up-scale the minimum valid crop rectangle is equal > > > > > > I think you need a comma after "up-scale" (my internal language parser > > > crashed when I reached "is equal"!) > > > > > > > Ah yes indeed. > > > > > > + to the smallest available stream resolution (plus any border pixels > > > > + required by the ISP for image processing) part of the current camera > > > > + configuration. If a pipeline can up-scale, the minimum valid crop > > > > > > Ah, it's right here! :) > > > > > > Thanks again, and best regards > > > > Thank you. > > > > From the top of your mind, as you know your platform better than me, > > can you suggest a configuration (for imx219) that would let me test > > the correctness of the reported limits ? I'm thinking about a mode > > that uses binning and where the lower limit has to be scaled back to > > the sensor's native pixel array coordinates. > > So I believe all our sensors (ov5647, imx219, imx477) will run in 2x2 > binning mode when started just by typing "qcam". So, as the minimum > crop size is 64x64 into the ISP, we should get 128x128. Thanks, I've just tested the above and I get: -srole=viewfinder (2x2 binning) Selected mode: 1640x1232-pRAA ScalerCropLimits: max = (0x0)/3280x2464 min = (0x0)/128x128 -srole=viewfinder, -srole=raw (no binning) Selected mode: 3280x2464-pRAA ScalerCropLimits: max = (0x0)/3280x2464 min = (0x0)/64x64 I'll report this in the v2 commit message maybe Thanks j > > If you run "qcam -s role=viewfinder -s role=raw" we should get the > full resolution, so the rectangle should come out as 64x64. > > (I do wonder whether we should be placing a restriction of, say, 16x, > for upscaling in our pipeline for other reasons - but I think that's > our business and doesn't affect the discussion here.) > > Thanks and best regards > David > > > > > Thanks > > j > > > > > David > > > > > > > + rectangle is equal to the smallest frame size accepted by the ISP input. > > > > + > > > > + The Rectangles are valid only after the camera has been successfully > > > > + configured and their value may change whenever a new configuration is > > > > applied. > > > > > > > > - \todo Turn this property into a "maximum control value" for the > > > > - ScalerCrop control once "dynamic" controls have been implemented. > > > > + \todo Express this property as the limits for the ScalerCrop control > > > > + once "dynamic" controls have been implemented. > > > > > > > > ... > > > > -- > > > > 2.29.1 > > > >
diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index c8874fa91965..11623fc56589 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -528,6 +528,6 @@ controls: a binning or skipping mode. This control is only present when the pipeline supports scaling. Its - maximum valid value is given by the properties::ScalerCropMaximum + valid value limits are given by the properties::ScalerCropLimits property, and the two can be used to implement digital zoom. ... diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 00a40c558bfa..cac88ca65ba5 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -702,13 +702,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) LOG(RPI, Error) << "Failed to configure the IPA: " << ret; /* - * Update the ScalerCropMaximum to the correct value for this camera mode. - * For us, it's the same as the "analogue crop". + * Update the ScalerCropLimits to the correct values for this camera + * mode. For us, it's the same as the "analogue crop" and the ISP + * minimum input size. * * \todo Make this property the ScalerCrop maximum value when dynamic * controls are available and set it at validate() time */ - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop); + data->properties_.set(properties::ScalerCropLimits, + { data->sensorInfo_.analogCrop, + Rectangle(data->ispMinCropSize_) }); return ret; } @@ -937,11 +940,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) data->properties_ = data->sensor_->properties(); /* - * Set a default value for the ScalerCropMaximum property to show + * Set default values for the ScalerCropLimits property to show * that we support its use, however, initialise it to zero because * it's not meaningful until a camera mode has been chosen. */ - data->properties_.set(properties::ScalerCropMaximum, Rectangle{}); + data->properties_.set(properties::ScalerCropLimits, + { Rectangle{}, Rectangle{} }); /* * We cache three things about the sensor in relation to transforms diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml index 64e88f0361d6..9a3ffcd05eeb 100644 --- a/src/libcamera/property_ids.yaml +++ b/src/libcamera/property_ids.yaml @@ -663,19 +663,37 @@ controls: \todo Rename this property to ActiveAreas once we will have property categories (i.e. Properties::PixelArray::ActiveAreas) - - ScalerCropMaximum: + - ScalerCropLimits: type: Rectangle + size: [2] description: | - The maximum valid rectangle for the controls::ScalerCrop control. This - reflects the minimum mandatory cropping applied in the camera sensor and - the rest of the pipeline. Just as the ScalerCrop control, it defines a - rectangle taken from the sensor's active pixel array. - - This property is valid only after the camera has been successfully - configured and its value may change whenever a new configuration is + The maximum and minimum valid rectangles for the controls::ScalerCrop + control, specified in this order. + + This two rectangles respectively reflect the minimum and maximum + mandatory cropping applied in the camera sensor and the rest of the + pipeline. Both Rectangles are defined relatively to the sensor's active + pixel array (properties::PixelArrayActiveAreas). + + Implementation details for the maximum valid crop rectangle. + The maximum valid crop rectangle depends on the camera configuration as + pipelines are free to adjust the frame size requested from the sensor + and used to produced the final image streams. The largest possible crop + rectangle is then limited by the size of the sensor's active pixel area + portion used to produce the sensor output frame. + + Implementation details for the minimum valid crop rectangle. + If a pipeline cannot up-scale the minimum valid crop rectangle is equal + to the smallest available stream resolution (plus any border pixels + required by the ISP for image processing) part of the current camera + configuration. If a pipeline can up-scale, the minimum valid crop + rectangle is equal to the smallest frame size accepted by the ISP input. + + The Rectangles are valid only after the camera has been successfully + configured and their value may change whenever a new configuration is applied. - \todo Turn this property into a "maximum control value" for the - ScalerCrop control once "dynamic" controls have been implemented. + \todo Express this property as the limits for the ScalerCrop control + once "dynamic" controls have been implemented. ...
Currently the properties::ScalerCropMaximum property reports the maximum valid Rectangle for the controls::ScalerCrop control, which can be combined with the sensor pixel array size in order to obtain the minimum available zoom factor. It is also useful to be able to calculate the maximum available zoom factor, and in order to do so the minimum valid crop rectangle has to be reported. Transform the ScalerCropMaximum property in ScalerCropLimits, which reports both the maximum and minimum crop limits and port the only user of such a property (Raspberry Pi) to use it. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/control_ids.yaml | 2 +- .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++--- src/libcamera/property_ids.yaml | 38 ++++++++++++++----- 3 files changed, 38 insertions(+), 16 deletions(-) -- 2.29.1