| Message ID | 20190827095008.11405-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > Retrieve the camera sensor pixel array sizes and the active pixel area > using the V4L2Subdevice selection API. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/geometry.h | 1 + > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > src/libcamera/geometry.cpp | 24 +++++++++++++ > src/libcamera/include/camera_sensor.h | 4 +++ > 4 files changed, 80 insertions(+) I think you should split the changes to geometry.{cpp,h} into its own patch. > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index 52f4d010de76..b91fcfa1dd17 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -42,6 +42,7 @@ struct Size { > unsigned int height; > > const std::string toString() const; > + bool contains(const Rectangle &rectangle) const; > }; > > bool operator==(const Size &lhs, const Size &rhs); > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index fc7fdcdcaf5b..b6ccaae0930b 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -128,6 +128,28 @@ int CameraSensor::init() > } > rotation_ = value; > > + /* > + * Retrieve and store the sensor pixel array size and active area > + * rectangle. > + */ > + Rectangle rect; > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > + if (ret) > + return ret; > + pixelArray_.width = rect.w; > + pixelArray_.height = rect.h; > + > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > + if (ret) > + return ret; > + > + if (!pixelArray_.contains(rect)) { > + LOG(CameraSensor, Error) > + << "Invalid camera sensor pixel array dimension"; > + return -EINVAL; > + } > + activeArea_ = rect; > + > /* Enumerate and cache media bus codes and sizes. */ > const ImageFormats formats = subdev_->formats(0); > if (formats.isEmpty()) { > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > return sizes_.back(); > } > > +/** > + * \fn CameraSensor::pixelArray() > + * \brief Retrieve the camera sensor pixel array dimension > + * \return The number of horizontal and vertical pixel units installed on the > + * camera sensor > + * > + * The returned pixel array size is the number of pixels units physically > + * installed on the sensor, including non-active ones, such as pixels used > + * for calibration or testing. > + */ > + > +/** > + * \fn CameraSensor::activeArea() > + * \brief Retrieve the active pixels area > + * \return A rectangle describing the active pixel area > + * > + * The returned rectangle is contained in the larger pixel array area, > + * returned by the CameraSensor::pixelArray() operation. > + * > + * \todo This operation collides with CameraSensor::resolution() as they > + * both reports the same information (bugs in the driver code apart). > + * Also, for some sensors, the maximum available resolution might depend on > + * the image size ratio (ie the maximumx resolution for images in 4:3 format > + * is different from the maximum available resolution for 16:9 format). This > + * has to be sorted out as well. Should we remove CameraSensor::resolution() in favor if new more "correct" way of finding out the sensor resolution? > + * > + * \sa Size::contains() > + */ > + > /** > * \brief Retrieve the best sensor format for a desired output > * \param[in] mbusCodes The list of acceptable media bus codes > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index 32b0faeadc63..9b60768a57a7 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -116,6 +116,30 @@ const std::string Size::toString() const > return std::to_string(width) + "x" + std::to_string(height); > } > > +/** > + * \fn Size::contains() > + * \param rectangle The rectangle to verify > + * \return True if \a rectangle is contained in the area represented by this > + * Size, false otherwise Return comes at the end of the documentation block. > + * > + * Return true if \a rectangle is completely contained in the area represented > + * by the width and height of this Size, with its top left corner assumed to be > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > + * horizontal and vertical positive displacement from the Size top left corner, > + * from where the rectangle's horizontal and vertical sizes are calculated. > + */ > +bool Size::contains(const Rectangle &rectangle) const > +{ > + if (rectangle.x < 0 || rectangle.y < 0) > + return false; > + > + if (rectangle.x + rectangle.w > width || > + rectangle.y + rectangle.h > height) > + return false; > + > + return true; > +} > + > /** > * \brief Compare sizes for equality > * \return True if the two sizes are equal, false otherwise > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index 32d39127b275..abf5344cf9d8 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -38,6 +38,8 @@ public: > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > const std::vector<Size> &sizes() const { return sizes_; } > const Size &resolution() const; > + const Size pixelArray() const { return pixelArray_; } > + const Rectangle activeArea() const { return activeArea_; } > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > const Size &size) const; > @@ -59,6 +61,8 @@ private: > > CameraLocation location_; > unsigned int rotation_; > + Size pixelArray_; > + Rectangle activeArea_; > }; > > } /* namespace libcamera */ > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > > Retrieve the camera sensor pixel array sizes and the active pixel area > > using the V4L2Subdevice selection API. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/geometry.h | 1 + > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > > src/libcamera/geometry.cpp | 24 +++++++++++++ > > src/libcamera/include/camera_sensor.h | 4 +++ > > 4 files changed, 80 insertions(+) > > I think you should split the changes to geometry.{cpp,h} into its own > patch. > Mmmm, I know we stand on two opposite sides here... the change is rather trivial and it makes sense to me to show its usage in the same patch to avoid people jumping from one patch to another during review. I could split if this bothers you, but it's not worth it in my opinion > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > index 52f4d010de76..b91fcfa1dd17 100644 > > --- a/include/libcamera/geometry.h > > +++ b/include/libcamera/geometry.h > > @@ -42,6 +42,7 @@ struct Size { > > unsigned int height; > > > > const std::string toString() const; > > + bool contains(const Rectangle &rectangle) const; > > }; > > > > bool operator==(const Size &lhs, const Size &rhs); > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index fc7fdcdcaf5b..b6ccaae0930b 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -128,6 +128,28 @@ int CameraSensor::init() > > } > > rotation_ = value; > > > > + /* > > + * Retrieve and store the sensor pixel array size and active area > > + * rectangle. > > + */ > > + Rectangle rect; > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > > + if (ret) > > + return ret; > > + pixelArray_.width = rect.w; > > + pixelArray_.height = rect.h; > > + > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > + if (ret) > > + return ret; > > + > > + if (!pixelArray_.contains(rect)) { > > + LOG(CameraSensor, Error) > > + << "Invalid camera sensor pixel array dimension"; > > + return -EINVAL; > > + } > > + activeArea_ = rect; > > + > > /* Enumerate and cache media bus codes and sizes. */ > > const ImageFormats formats = subdev_->formats(0); > > if (formats.isEmpty()) { > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > > return sizes_.back(); > > } > > > > +/** > > + * \fn CameraSensor::pixelArray() > > + * \brief Retrieve the camera sensor pixel array dimension > > + * \return The number of horizontal and vertical pixel units installed on the > > + * camera sensor > > + * > > + * The returned pixel array size is the number of pixels units physically > > + * installed on the sensor, including non-active ones, such as pixels used > > + * for calibration or testing. > > + */ > > + > > +/** > > + * \fn CameraSensor::activeArea() > > + * \brief Retrieve the active pixels area > > + * \return A rectangle describing the active pixel area > > + * > > + * The returned rectangle is contained in the larger pixel array area, > > + * returned by the CameraSensor::pixelArray() operation. > > + * > > + * \todo This operation collides with CameraSensor::resolution() as they > > + * both reports the same information (bugs in the driver code apart). > > + * Also, for some sensors, the maximum available resolution might depend on > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format > > + * is different from the maximum available resolution for 16:9 format). This > > + * has to be sorted out as well. > > Should we remove CameraSensor::resolution() in favor if new more > "correct" way of finding out the sensor resolution? > Good question.. another option would be to get the maximum resolution in regard to a specific aspect ratio... I would say I should better look into the current usages of this operation... > > + * > > + * \sa Size::contains() > > + */ > > + > > /** > > * \brief Retrieve the best sensor format for a desired output > > * \param[in] mbusCodes The list of acceptable media bus codes > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > index 32b0faeadc63..9b60768a57a7 100644 > > --- a/src/libcamera/geometry.cpp > > +++ b/src/libcamera/geometry.cpp > > @@ -116,6 +116,30 @@ const std::string Size::toString() const > > return std::to_string(width) + "x" + std::to_string(height); > > } > > > > +/** > > + * \fn Size::contains() > > + * \param rectangle The rectangle to verify > > + * \return True if \a rectangle is contained in the area represented by this > > + * Size, false otherwise > > Return comes at the end of the documentation block. > True, thanks for spotting > > + * > > + * Return true if \a rectangle is completely contained in the area represented > > + * by the width and height of this Size, with its top left corner assumed to be > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > > + * horizontal and vertical positive displacement from the Size top left corner, > > + * from where the rectangle's horizontal and vertical sizes are calculated. > > + */ > > +bool Size::contains(const Rectangle &rectangle) const > > +{ > > + if (rectangle.x < 0 || rectangle.y < 0) > > + return false; > > + > > + if (rectangle.x + rectangle.w > width || > > + rectangle.y + rectangle.h > height) > > + return false; > > + > > + return true; > > +} > > + > > /** > > * \brief Compare sizes for equality > > * \return True if the two sizes are equal, false otherwise > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index 32d39127b275..abf5344cf9d8 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -38,6 +38,8 @@ public: > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > > const std::vector<Size> &sizes() const { return sizes_; } > > const Size &resolution() const; > > + const Size pixelArray() const { return pixelArray_; } > > + const Rectangle activeArea() const { return activeArea_; } > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > const Size &size) const; > > @@ -59,6 +61,8 @@ private: > > > > CameraLocation location_; > > unsigned int rotation_; > > + Size pixelArray_; > > + Rectangle activeArea_; > > }; > > > > } /* namespace libcamera */ > > -- > > 2.23.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > > > Retrieve the camera sensor pixel array sizes and the active pixel area > > > using the V4L2Subdevice selection API. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/geometry.h | 1 + > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > > > src/libcamera/geometry.cpp | 24 +++++++++++++ > > > src/libcamera/include/camera_sensor.h | 4 +++ > > > 4 files changed, 80 insertions(+) > > > > I think you should split the changes to geometry.{cpp,h} into its own > > patch. > > > Mmmm, I know we stand on two opposite sides here... the change is > rather trivial and it makes sense to me to show its usage in the same > patch to avoid people jumping from one patch to another during review. > I could split if this bothers you, but it's not worth it in my opinion I agree this is a fight you and I will enjoy for many years I hope, until I win in the end of course ;-P I don't feel strongly about it so don't spend energy on it if you don't want to for trivial patches. For mid to large size patches I feel very strongly on this topic tho. I know you feel it's easier to review larger patches and not "jump" between multiple ones. For me it's the opposite, I spend 4 times longer reviewing a patch like this because my warning light go off asking why the patch do two or more things. I try to find the reason for this but can't, and once I realise the patch do more then one thing without it strictly needing to I comment on it to release my frustration ;-) I think this is some sort of meta-bikesheeding discussion, not sure if that is a good thing or not :-) > > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > index 52f4d010de76..b91fcfa1dd17 100644 > > > --- a/include/libcamera/geometry.h > > > +++ b/include/libcamera/geometry.h > > > @@ -42,6 +42,7 @@ struct Size { > > > unsigned int height; > > > > > > const std::string toString() const; > > > + bool contains(const Rectangle &rectangle) const; > > > }; > > > > > > bool operator==(const Size &lhs, const Size &rhs); > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index fc7fdcdcaf5b..b6ccaae0930b 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -128,6 +128,28 @@ int CameraSensor::init() > > > } > > > rotation_ = value; > > > > > > + /* > > > + * Retrieve and store the sensor pixel array size and active area > > > + * rectangle. > > > + */ > > > + Rectangle rect; > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > > > + if (ret) > > > + return ret; > > > + pixelArray_.width = rect.w; > > > + pixelArray_.height = rect.h; > > > + > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > > + if (ret) > > > + return ret; > > > + > > > + if (!pixelArray_.contains(rect)) { > > > + LOG(CameraSensor, Error) > > > + << "Invalid camera sensor pixel array dimension"; > > > + return -EINVAL; > > > + } > > > + activeArea_ = rect; > > > + > > > /* Enumerate and cache media bus codes and sizes. */ > > > const ImageFormats formats = subdev_->formats(0); > > > if (formats.isEmpty()) { > > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > > > return sizes_.back(); > > > } > > > > > > +/** > > > + * \fn CameraSensor::pixelArray() > > > + * \brief Retrieve the camera sensor pixel array dimension > > > + * \return The number of horizontal and vertical pixel units installed on the > > > + * camera sensor > > > + * > > > + * The returned pixel array size is the number of pixels units physically > > > + * installed on the sensor, including non-active ones, such as pixels used > > > + * for calibration or testing. > > > + */ > > > + > > > +/** > > > + * \fn CameraSensor::activeArea() > > > + * \brief Retrieve the active pixels area > > > + * \return A rectangle describing the active pixel area > > > + * > > > + * The returned rectangle is contained in the larger pixel array area, > > > + * returned by the CameraSensor::pixelArray() operation. > > > + * > > > + * \todo This operation collides with CameraSensor::resolution() as they > > > + * both reports the same information (bugs in the driver code apart). > > > + * Also, for some sensors, the maximum available resolution might depend on > > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format > > > + * is different from the maximum available resolution for 16:9 format). This > > > + * has to be sorted out as well. > > > > Should we remove CameraSensor::resolution() in favor if new more > > "correct" way of finding out the sensor resolution? > > > > Good question.. another option would be to get the maximum resolution > in regard to a specific aspect ratio... I would say I should better > look into the current usages of this operation... > > > > + * > > > + * \sa Size::contains() > > > + */ > > > + > > > /** > > > * \brief Retrieve the best sensor format for a desired output > > > * \param[in] mbusCodes The list of acceptable media bus codes > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > index 32b0faeadc63..9b60768a57a7 100644 > > > --- a/src/libcamera/geometry.cpp > > > +++ b/src/libcamera/geometry.cpp > > > @@ -116,6 +116,30 @@ const std::string Size::toString() const > > > return std::to_string(width) + "x" + std::to_string(height); > > > } > > > > > > +/** > > > + * \fn Size::contains() > > > + * \param rectangle The rectangle to verify > > > + * \return True if \a rectangle is contained in the area represented by this > > > + * Size, false otherwise > > > > Return comes at the end of the documentation block. > > > > True, thanks for spotting > > > > + * > > > + * Return true if \a rectangle is completely contained in the area represented > > > + * by the width and height of this Size, with its top left corner assumed to be > > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > > > + * horizontal and vertical positive displacement from the Size top left corner, > > > + * from where the rectangle's horizontal and vertical sizes are calculated. > > > + */ > > > +bool Size::contains(const Rectangle &rectangle) const > > > +{ > > > + if (rectangle.x < 0 || rectangle.y < 0) > > > + return false; > > > + > > > + if (rectangle.x + rectangle.w > width || > > > + rectangle.y + rectangle.h > height) > > > + return false; > > > + > > > + return true; > > > +} > > > + > > > /** > > > * \brief Compare sizes for equality > > > * \return True if the two sizes are equal, false otherwise > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > index 32d39127b275..abf5344cf9d8 100644 > > > --- a/src/libcamera/include/camera_sensor.h > > > +++ b/src/libcamera/include/camera_sensor.h > > > @@ -38,6 +38,8 @@ public: > > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > > > const std::vector<Size> &sizes() const { return sizes_; } > > > const Size &resolution() const; > > > + const Size pixelArray() const { return pixelArray_; } > > > + const Rectangle activeArea() const { return activeArea_; } > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > const Size &size) const; > > > @@ -59,6 +61,8 @@ private: > > > > > > CameraLocation location_; > > > unsigned int rotation_; > > > + Size pixelArray_; > > > + Rectangle activeArea_; > > > }; > > > > > > } /* namespace libcamera */ > > > -- > > > 2.23.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote: > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > > > > Retrieve the camera sensor pixel array sizes and the active pixel area > > > > using the V4L2Subdevice selection API. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > include/libcamera/geometry.h | 1 + > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > > > > src/libcamera/geometry.cpp | 24 +++++++++++++ > > > > src/libcamera/include/camera_sensor.h | 4 +++ > > > > 4 files changed, 80 insertions(+) > > > > > > I think you should split the changes to geometry.{cpp,h} into its own > > > patch. > > > > > Mmmm, I know we stand on two opposite sides here... the change is > > rather trivial and it makes sense to me to show its usage in the same > > patch to avoid people jumping from one patch to another during review. > > I could split if this bothers you, but it's not worth it in my opinion > > I agree this is a fight you and I will enjoy for many years I hope, > until I win in the end of course ;-P I don't feel strongly about it so > don't spend energy on it if you don't want to for trivial patches. For > mid to large size patches I feel very strongly on this topic tho. > > I know you feel it's easier to review larger patches and not "jump" > between multiple ones. For me it's the opposite, I spend 4 times longer > reviewing a patch like this because my warning light go off asking why > the patch do two or more things. I try to find the reason for this but > can't, and once I realise the patch do more then one thing without it > strictly needing to I comment on it to release my frustration ;-) > > I think this is some sort of meta-bikesheeding discussion, not sure if > that is a good thing or not :-) > So I'm glad we have demonstrated we're both right here (or both wrong) as by definition bikeshedding discussions are made to disappoint all the involved parties. For bigger patches I agree, but please let me keep this one the way it is... One less metaphysical question below.. > > > > > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > > index 52f4d010de76..b91fcfa1dd17 100644 > > > > --- a/include/libcamera/geometry.h > > > > +++ b/include/libcamera/geometry.h > > > > @@ -42,6 +42,7 @@ struct Size { > > > > unsigned int height; > > > > > > > > const std::string toString() const; > > > > + bool contains(const Rectangle &rectangle) const; > > > > }; > > > > > > > > bool operator==(const Size &lhs, const Size &rhs); > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > index fc7fdcdcaf5b..b6ccaae0930b 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -128,6 +128,28 @@ int CameraSensor::init() > > > > } > > > > rotation_ = value; > > > > > > > > + /* > > > > + * Retrieve and store the sensor pixel array size and active area > > > > + * rectangle. > > > > + */ > > > > + Rectangle rect; > > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > > > > + if (ret) > > > > + return ret; > > > > + pixelArray_.width = rect.w; > > > > + pixelArray_.height = rect.h; > > > > + > > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (!pixelArray_.contains(rect)) { > > > > + LOG(CameraSensor, Error) > > > > + << "Invalid camera sensor pixel array dimension"; > > > > + return -EINVAL; > > > > + } > > > > + activeArea_ = rect; I hoped for a comment here, as what's here happening is that we're validating the kernel driver reported parameters. Is this something we want to do or should we assume the kernel side is, as we all know very well, perfect ? Thanks j > > > > + > > > > /* Enumerate and cache media bus codes and sizes. */ > > > > const ImageFormats formats = subdev_->formats(0); > > > > if (formats.isEmpty()) { > > > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > > > > return sizes_.back(); > > > > } > > > > > > > > +/** > > > > + * \fn CameraSensor::pixelArray() > > > > + * \brief Retrieve the camera sensor pixel array dimension > > > > + * \return The number of horizontal and vertical pixel units installed on the > > > > + * camera sensor > > > > + * > > > > + * The returned pixel array size is the number of pixels units physically > > > > + * installed on the sensor, including non-active ones, such as pixels used > > > > + * for calibration or testing. > > > > + */ > > > > + > > > > +/** > > > > + * \fn CameraSensor::activeArea() > > > > + * \brief Retrieve the active pixels area > > > > + * \return A rectangle describing the active pixel area > > > > + * > > > > + * The returned rectangle is contained in the larger pixel array area, > > > > + * returned by the CameraSensor::pixelArray() operation. > > > > + * > > > > + * \todo This operation collides with CameraSensor::resolution() as they > > > > + * both reports the same information (bugs in the driver code apart). > > > > + * Also, for some sensors, the maximum available resolution might depend on > > > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format > > > > + * is different from the maximum available resolution for 16:9 format). This > > > > + * has to be sorted out as well. > > > > > > Should we remove CameraSensor::resolution() in favor if new more > > > "correct" way of finding out the sensor resolution? > > > > > > > Good question.. another option would be to get the maximum resolution > > in regard to a specific aspect ratio... I would say I should better > > look into the current usages of this operation... > > > > > > + * > > > > + * \sa Size::contains() > > > > + */ > > > > + > > > > /** > > > > * \brief Retrieve the best sensor format for a desired output > > > > * \param[in] mbusCodes The list of acceptable media bus codes > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > > index 32b0faeadc63..9b60768a57a7 100644 > > > > --- a/src/libcamera/geometry.cpp > > > > +++ b/src/libcamera/geometry.cpp > > > > @@ -116,6 +116,30 @@ const std::string Size::toString() const > > > > return std::to_string(width) + "x" + std::to_string(height); > > > > } > > > > > > > > +/** > > > > + * \fn Size::contains() > > > > + * \param rectangle The rectangle to verify > > > > + * \return True if \a rectangle is contained in the area represented by this > > > > + * Size, false otherwise > > > > > > Return comes at the end of the documentation block. > > > > > > > True, thanks for spotting > > > > > > + * > > > > + * Return true if \a rectangle is completely contained in the area represented > > > > + * by the width and height of this Size, with its top left corner assumed to be > > > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > > > > + * horizontal and vertical positive displacement from the Size top left corner, > > > > + * from where the rectangle's horizontal and vertical sizes are calculated. > > > > + */ > > > > +bool Size::contains(const Rectangle &rectangle) const > > > > +{ > > > > + if (rectangle.x < 0 || rectangle.y < 0) > > > > + return false; > > > > + > > > > + if (rectangle.x + rectangle.w > width || > > > > + rectangle.y + rectangle.h > height) > > > > + return false; > > > > + > > > > + return true; > > > > +} > > > > + > > > > /** > > > > * \brief Compare sizes for equality > > > > * \return True if the two sizes are equal, false otherwise > > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > > index 32d39127b275..abf5344cf9d8 100644 > > > > --- a/src/libcamera/include/camera_sensor.h > > > > +++ b/src/libcamera/include/camera_sensor.h > > > > @@ -38,6 +38,8 @@ public: > > > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > > > > const std::vector<Size> &sizes() const { return sizes_; } > > > > const Size &resolution() const; > > > > + const Size pixelArray() const { return pixelArray_; } > > > > + const Rectangle activeArea() const { return activeArea_; } > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > const Size &size) const; > > > > @@ -59,6 +61,8 @@ private: > > > > > > > > CameraLocation location_; > > > > unsigned int rotation_; > > > > + Size pixelArray_; > > > > + Rectangle activeArea_; > > > > }; > > > > > > > > } /* namespace libcamera */ > > > > -- > > > > 2.23.0 > > > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-08-29 11:07:36 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote: > > Hi Jacopo, > > > > On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote: > > > Hi Niklas, > > > > > > On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote: > > > > Hi Jacopo, > > > > > > > > Thanks for your work. > > > > > > > > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > > > > > Retrieve the camera sensor pixel array sizes and the active pixel area > > > > > using the V4L2Subdevice selection API. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > include/libcamera/geometry.h | 1 + > > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > > > > > src/libcamera/geometry.cpp | 24 +++++++++++++ > > > > > src/libcamera/include/camera_sensor.h | 4 +++ > > > > > 4 files changed, 80 insertions(+) > > > > > > > > I think you should split the changes to geometry.{cpp,h} into its own > > > > patch. > > > > > > > Mmmm, I know we stand on two opposite sides here... the change is > > > rather trivial and it makes sense to me to show its usage in the same > > > patch to avoid people jumping from one patch to another during review. > > > I could split if this bothers you, but it's not worth it in my opinion > > > > I agree this is a fight you and I will enjoy for many years I hope, > > until I win in the end of course ;-P I don't feel strongly about it so > > don't spend energy on it if you don't want to for trivial patches. For > > mid to large size patches I feel very strongly on this topic tho. > > > > I know you feel it's easier to review larger patches and not "jump" > > between multiple ones. For me it's the opposite, I spend 4 times longer > > reviewing a patch like this because my warning light go off asking why > > the patch do two or more things. I try to find the reason for this but > > can't, and once I realise the patch do more then one thing without it > > strictly needing to I comment on it to release my frustration ;-) > > > > I think this is some sort of meta-bikesheeding discussion, not sure if > > that is a good thing or not :-) > > > So I'm glad we have demonstrated we're both right here (or both wrong) > as by definition bikeshedding discussions are made to disappoint all > the involved parties. :-) > > For bigger patches I agree, but please let me keep this one the way it > is... Absolutely, no worries. Sorry if that was not clear in my previous mail that I do not feel strongly about it in regard to this patch. > > One less metaphysical question below.. > > > > > > > > > > > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > > > > index 52f4d010de76..b91fcfa1dd17 100644 > > > > > --- a/include/libcamera/geometry.h > > > > > +++ b/include/libcamera/geometry.h > > > > > @@ -42,6 +42,7 @@ struct Size { > > > > > unsigned int height; > > > > > > > > > > const std::string toString() const; > > > > > + bool contains(const Rectangle &rectangle) const; > > > > > }; > > > > > > > > > > bool operator==(const Size &lhs, const Size &rhs); > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > index fc7fdcdcaf5b..b6ccaae0930b 100644 > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > @@ -128,6 +128,28 @@ int CameraSensor::init() > > > > > } > > > > > rotation_ = value; > > > > > > > > > > + /* > > > > > + * Retrieve and store the sensor pixel array size and active area > > > > > + * rectangle. > > > > > + */ > > > > > + Rectangle rect; > > > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > > > > > + if (ret) > > > > > + return ret; > > > > > + pixelArray_.width = rect.w; > > > > > + pixelArray_.height = rect.h; > > > > > + > > > > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + if (!pixelArray_.contains(rect)) { > > > > > + LOG(CameraSensor, Error) > > > > > + << "Invalid camera sensor pixel array dimension"; > > > > > + return -EINVAL; > > > > > + } > > > > > + activeArea_ = rect; > > I hoped for a comment here, as what's here happening is that we're > validating the kernel driver reported parameters. Is this something > we want to do or should we assume the kernel side is, as we all know > very well, perfect ? I'm divided, it's good to have checks but I also thinks the kernel should report the correct thing. And if not and things so south it should be fixed in the kernel. > > Thanks > j > > > > > > + > > > > > /* Enumerate and cache media bus codes and sizes. */ > > > > > const ImageFormats formats = subdev_->formats(0); > > > > > if (formats.isEmpty()) { > > > > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > > > > > return sizes_.back(); > > > > > } > > > > > > > > > > +/** > > > > > + * \fn CameraSensor::pixelArray() > > > > > + * \brief Retrieve the camera sensor pixel array dimension > > > > > + * \return The number of horizontal and vertical pixel units installed on the > > > > > + * camera sensor > > > > > + * > > > > > + * The returned pixel array size is the number of pixels units physically > > > > > + * installed on the sensor, including non-active ones, such as pixels used > > > > > + * for calibration or testing. > > > > > + */ > > > > > + > > > > > +/** > > > > > + * \fn CameraSensor::activeArea() > > > > > + * \brief Retrieve the active pixels area > > > > > + * \return A rectangle describing the active pixel area > > > > > + * > > > > > + * The returned rectangle is contained in the larger pixel array area, > > > > > + * returned by the CameraSensor::pixelArray() operation. > > > > > + * > > > > > + * \todo This operation collides with CameraSensor::resolution() as they > > > > > + * both reports the same information (bugs in the driver code apart). > > > > > + * Also, for some sensors, the maximum available resolution might depend on > > > > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format > > > > > + * is different from the maximum available resolution for 16:9 format). This > > > > > + * has to be sorted out as well. > > > > > > > > Should we remove CameraSensor::resolution() in favor if new more > > > > "correct" way of finding out the sensor resolution? > > > > > > > > > > Good question.. another option would be to get the maximum resolution > > > in regard to a specific aspect ratio... I would say I should better > > > look into the current usages of this operation... > > > > > > > > + * > > > > > + * \sa Size::contains() > > > > > + */ > > > > > + > > > > > /** > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > * \param[in] mbusCodes The list of acceptable media bus codes > > > > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > > > > index 32b0faeadc63..9b60768a57a7 100644 > > > > > --- a/src/libcamera/geometry.cpp > > > > > +++ b/src/libcamera/geometry.cpp > > > > > @@ -116,6 +116,30 @@ const std::string Size::toString() const > > > > > return std::to_string(width) + "x" + std::to_string(height); > > > > > } > > > > > > > > > > +/** > > > > > + * \fn Size::contains() > > > > > + * \param rectangle The rectangle to verify > > > > > + * \return True if \a rectangle is contained in the area represented by this > > > > > + * Size, false otherwise > > > > > > > > Return comes at the end of the documentation block. > > > > > > > > > > True, thanks for spotting > > > > > > > > + * > > > > > + * Return true if \a rectangle is completely contained in the area represented > > > > > + * by the width and height of this Size, with its top left corner assumed to be > > > > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > > > > > + * horizontal and vertical positive displacement from the Size top left corner, > > > > > + * from where the rectangle's horizontal and vertical sizes are calculated. > > > > > + */ > > > > > +bool Size::contains(const Rectangle &rectangle) const > > > > > +{ > > > > > + if (rectangle.x < 0 || rectangle.y < 0) > > > > > + return false; > > > > > + > > > > > + if (rectangle.x + rectangle.w > width || > > > > > + rectangle.y + rectangle.h > height) > > > > > + return false; > > > > > + > > > > > + return true; > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Compare sizes for equality > > > > > * \return True if the two sizes are equal, false otherwise > > > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > > > index 32d39127b275..abf5344cf9d8 100644 > > > > > --- a/src/libcamera/include/camera_sensor.h > > > > > +++ b/src/libcamera/include/camera_sensor.h > > > > > @@ -38,6 +38,8 @@ public: > > > > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > > > > > const std::vector<Size> &sizes() const { return sizes_; } > > > > > const Size &resolution() const; > > > > > + const Size pixelArray() const { return pixelArray_; } > > > > > + const Rectangle activeArea() const { return activeArea_; } > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > const Size &size) const; > > > > > @@ -59,6 +61,8 @@ private: > > > > > > > > > > CameraLocation location_; > > > > > unsigned int rotation_; > > > > > + Size pixelArray_; > > > > > + Rectangle activeArea_; > > > > > }; > > > > > > > > > > } /* namespace libcamera */ > > > > > -- > > > > > 2.23.0 > > > > > > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hello, On Thu, Aug 29, 2019 at 04:27:22PM +0200, Niklas Söderlund wrote: > On 2019-08-29 11:07:36 +0200, Jacopo Mondi wrote: > > On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote: > >> On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote: > >>> On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote: > >>>> On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote: > >>>>> Retrieve the camera sensor pixel array sizes and the active pixel area s/sizes/size/ > >>>>> using the V4L2Subdevice selection API. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> include/libcamera/geometry.h | 1 + > >>>>> src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ > >>>>> src/libcamera/geometry.cpp | 24 +++++++++++++ > >>>>> src/libcamera/include/camera_sensor.h | 4 +++ > >>>>> 4 files changed, 80 insertions(+) > >>>> > >>>> I think you should split the changes to geometry.{cpp,h} into its own > >>>> patch. > >>>> > >>> Mmmm, I know we stand on two opposite sides here... the change is > >>> rather trivial and it makes sense to me to show its usage in the same > >>> patch to avoid people jumping from one patch to another during review. > >>> I could split if this bothers you, but it's not worth it in my opinion > >> > >> I agree this is a fight you and I will enjoy for many years I hope, > >> until I win in the end of course ;-P I don't feel strongly about it so > >> don't spend energy on it if you don't want to for trivial patches. For > >> mid to large size patches I feel very strongly on this topic tho. > >> > >> I know you feel it's easier to review larger patches and not "jump" > >> between multiple ones. For me it's the opposite, I spend 4 times longer > >> reviewing a patch like this because my warning light go off asking why > >> the patch do two or more things. I try to find the reason for this but > >> can't, and once I realise the patch do more then one thing without it > >> strictly needing to I comment on it to release my frustration ;-) > >> > >> I think this is some sort of meta-bikesheeding discussion, not sure if > >> that is a good thing or not :-) > >> > > So I'm glad we have demonstrated we're both right here (or both wrong) > > as by definition bikeshedding discussions are made to disappoint all > > the involved parties. > > :-) I'm afraid I will be as disappointed as Niklas here :-) My preference would also have been to split the geometry change. > > For bigger patches I agree, but please let me keep this one the way it > > is... > > Absolutely, no worries. Sorry if that was not clear in my previous mail > that I do not feel strongly about it in regard to this patch. > > > One less metaphysical question below.. > > > >>>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > >>>>> index 52f4d010de76..b91fcfa1dd17 100644 > >>>>> --- a/include/libcamera/geometry.h > >>>>> +++ b/include/libcamera/geometry.h > >>>>> @@ -42,6 +42,7 @@ struct Size { > >>>>> unsigned int height; > >>>>> > >>>>> const std::string toString() const; > >>>>> + bool contains(const Rectangle &rectangle) const; > >>>>> }; > >>>>> > >>>>> bool operator==(const Size &lhs, const Size &rhs); > >>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > >>>>> index fc7fdcdcaf5b..b6ccaae0930b 100644 > >>>>> --- a/src/libcamera/camera_sensor.cpp > >>>>> +++ b/src/libcamera/camera_sensor.cpp > >>>>> @@ -128,6 +128,28 @@ int CameraSensor::init() > >>>>> } > >>>>> rotation_ = value; > >>>>> > >>>>> + /* > >>>>> + * Retrieve and store the sensor pixel array size and active area > >>>>> + * rectangle. > >>>>> + */ > >>>>> + Rectangle rect; > >>>>> + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + pixelArray_.width = rect.w; > >>>>> + pixelArray_.height = rect.h; A Rectangle::size() method would be nice, you could then write pixelArray_ = rect.size(); > >>>>> + > >>>>> + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + if (!pixelArray_.contains(rect)) { > >>>>> + LOG(CameraSensor, Error) > >>>>> + << "Invalid camera sensor pixel array dimension"; > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + activeArea_ = rect; > > > > I hoped for a comment here, as what's here happening is that we're > > validating the kernel driver reported parameters. Is this something > > we want to do or should we assume the kernel side is, as we all know > > very well, perfect ? > > I'm divided, it's good to have checks but I also thinks the kernel > should report the correct thing. And if not and things so south it > should be fixed in the kernel. I wouldn't have thought about guarding against this personally. I don't mind the check much, but I wonder how far we should go checking that the kernel behaves correctly. > >>>>> + > >>>>> /* Enumerate and cache media bus codes and sizes. */ > >>>>> const ImageFormats formats = subdev_->formats(0); > >>>>> if (formats.isEmpty()) { > >>>>> @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const > >>>>> return sizes_.back(); > >>>>> } > >>>>> > >>>>> +/** > >>>>> + * \fn CameraSensor::pixelArray() > >>>>> + * \brief Retrieve the camera sensor pixel array dimension > >>>>> + * \return The number of horizontal and vertical pixel units installed on the > >>>>> + * camera sensor > >>>>> + * > >>>>> + * The returned pixel array size is the number of pixels units physically > >>>>> + * installed on the sensor, including non-active ones, such as pixels used > >>>>> + * for calibration or testing. I think we need a bit more details here, or possibly in the documentation of the CameraSensor class itself. We should have a diagram of how we model a sensor, and show when the pixel array, active area and resolution are. Furthermore, I would mention optical black pixels explicitly. To be precise, wouldn't they qualify as active pixels, but not exposed. What other non-exposed or non-active pixels would we have (and what is a non-active pixel) ? > >>>>> + */ > >>>>> + > >>>>> +/** > >>>>> + * \fn CameraSensor::activeArea() > >>>>> + * \brief Retrieve the active pixels area > >>>>> + * \return A rectangle describing the active pixel area > >>>>> + * > >>>>> + * The returned rectangle is contained in the larger pixel array area, > >>>>> + * returned by the CameraSensor::pixelArray() operation. > >>>>> + * > >>>>> + * \todo This operation collides with CameraSensor::resolution() as they > >>>>> + * both reports the same information (bugs in the driver code apart). > >>>>> + * Also, for some sensors, the maximum available resolution might depend on > >>>>> + * the image size ratio (ie the maximumx resolution for images in 4:3 format > >>>>> + * is different from the maximum available resolution for 16:9 format). This > >>>>> + * has to be sorted out as well. > >>>> > >>>> Should we remove CameraSensor::resolution() in favor if new more > >>>> "correct" way of finding out the sensor resolution? > >>> > >>> Good question.. another option would be to get the maximum resolution > >>> in regard to a specific aspect ratio... I would say I should better > >>> look into the current usages of this operation... Could you have a look ? I think we should address this todo item already if possible. > >>>>> + * > >>>>> + * \sa Size::contains() Is this useful ? > >>>>> + */ > >>>>> + > >>>>> /** > >>>>> * \brief Retrieve the best sensor format for a desired output > >>>>> * \param[in] mbusCodes The list of acceptable media bus codes > >>>>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > >>>>> index 32b0faeadc63..9b60768a57a7 100644 > >>>>> --- a/src/libcamera/geometry.cpp > >>>>> +++ b/src/libcamera/geometry.cpp > >>>>> @@ -116,6 +116,30 @@ const std::string Size::toString() const > >>>>> return std::to_string(width) + "x" + std::to_string(height); > >>>>> } > >>>>> > >>>>> +/** > >>>>> + * \fn Size::contains() > >>>>> + * \param rectangle The rectangle to verify > >>>>> + * \return True if \a rectangle is contained in the area represented by this > >>>>> + * Size, false otherwise > >>>> > >>>> Return comes at the end of the documentation block. > >>> > >>> True, thanks for spotting And the \brief is missing. > >>>>> + * > >>>>> + * Return true if \a rectangle is completely contained in the area represented > >>>>> + * by the width and height of this Size, with its top left corner assumed to be > >>>>> + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the > >>>>> + * horizontal and vertical positive displacement from the Size top left corner, > >>>>> + * from where the rectangle's horizontal and vertical sizes are calculated. I don't think this is the way to go. A rectangle "contained" in a size seems weird, and I would have intuitively thought this would check the rectangle size against the size. A better API would be to construct a Rectangle from a Point and a Size (as constructing it from just a size seems weird and ill-defined too), and add a bool Rectangle::contains(const Rectangle &rect) method. You will also need to define if rectangles that have overlapping borders would be considered as being contained or not. Could a.contains(b) && b.contains(a) ever be true ? You can add both the Point class, the new Rectangle constructor and the Rectangle::contains() method in the same patch if you split them from this one ;-) > >>>>> + */ > >>>>> +bool Size::contains(const Rectangle &rectangle) const > >>>>> +{ > >>>>> + if (rectangle.x < 0 || rectangle.y < 0) > >>>>> + return false; > >>>>> + > >>>>> + if (rectangle.x + rectangle.w > width || > >>>>> + rectangle.y + rectangle.h > height) > >>>>> + return false; > >>>>> + > >>>>> + return true; > >>>>> +} > >>>>> + > >>>>> /** > >>>>> * \brief Compare sizes for equality > >>>>> * \return True if the two sizes are equal, false otherwise > >>>>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > >>>>> index 32d39127b275..abf5344cf9d8 100644 > >>>>> --- a/src/libcamera/include/camera_sensor.h > >>>>> +++ b/src/libcamera/include/camera_sensor.h > >>>>> @@ -38,6 +38,8 @@ public: > >>>>> const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > >>>>> const std::vector<Size> &sizes() const { return sizes_; } > >>>>> const Size &resolution() const; > >>>>> + const Size pixelArray() const { return pixelArray_; } > >>>>> + const Rectangle activeArea() const { return activeArea_; } > >>>>> > >>>>> V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > >>>>> const Size &size) const; > >>>>> @@ -59,6 +61,8 @@ private: > >>>>> > >>>>> CameraLocation location_; > >>>>> unsigned int rotation_; > >>>>> + Size pixelArray_; > >>>>> + Rectangle activeArea_; > >>>>> }; > >>>>> > >>>>> } /* namespace libcamera */
diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h index 52f4d010de76..b91fcfa1dd17 100644 --- a/include/libcamera/geometry.h +++ b/include/libcamera/geometry.h @@ -42,6 +42,7 @@ struct Size { unsigned int height; const std::string toString() const; + bool contains(const Rectangle &rectangle) const; }; bool operator==(const Size &lhs, const Size &rhs); diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index fc7fdcdcaf5b..b6ccaae0930b 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -128,6 +128,28 @@ int CameraSensor::init() } rotation_ = value; + /* + * Retrieve and store the sensor pixel array size and active area + * rectangle. + */ + Rectangle rect; + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect); + if (ret) + return ret; + pixelArray_.width = rect.w; + pixelArray_.height = rect.h; + + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect); + if (ret) + return ret; + + if (!pixelArray_.contains(rect)) { + LOG(CameraSensor, Error) + << "Invalid camera sensor pixel array dimension"; + return -EINVAL; + } + activeArea_ = rect; + /* Enumerate and cache media bus codes and sizes. */ const ImageFormats formats = subdev_->formats(0); if (formats.isEmpty()) { @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const return sizes_.back(); } +/** + * \fn CameraSensor::pixelArray() + * \brief Retrieve the camera sensor pixel array dimension + * \return The number of horizontal and vertical pixel units installed on the + * camera sensor + * + * The returned pixel array size is the number of pixels units physically + * installed on the sensor, including non-active ones, such as pixels used + * for calibration or testing. + */ + +/** + * \fn CameraSensor::activeArea() + * \brief Retrieve the active pixels area + * \return A rectangle describing the active pixel area + * + * The returned rectangle is contained in the larger pixel array area, + * returned by the CameraSensor::pixelArray() operation. + * + * \todo This operation collides with CameraSensor::resolution() as they + * both reports the same information (bugs in the driver code apart). + * Also, for some sensors, the maximum available resolution might depend on + * the image size ratio (ie the maximumx resolution for images in 4:3 format + * is different from the maximum available resolution for 16:9 format). This + * has to be sorted out as well. + * + * \sa Size::contains() + */ + /** * \brief Retrieve the best sensor format for a desired output * \param[in] mbusCodes The list of acceptable media bus codes diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp index 32b0faeadc63..9b60768a57a7 100644 --- a/src/libcamera/geometry.cpp +++ b/src/libcamera/geometry.cpp @@ -116,6 +116,30 @@ const std::string Size::toString() const return std::to_string(width) + "x" + std::to_string(height); } +/** + * \fn Size::contains() + * \param rectangle The rectangle to verify + * \return True if \a rectangle is contained in the area represented by this + * Size, false otherwise + * + * Return true if \a rectangle is completely contained in the area represented + * by the width and height of this Size, with its top left corner assumed to be + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the + * horizontal and vertical positive displacement from the Size top left corner, + * from where the rectangle's horizontal and vertical sizes are calculated. + */ +bool Size::contains(const Rectangle &rectangle) const +{ + if (rectangle.x < 0 || rectangle.y < 0) + return false; + + if (rectangle.x + rectangle.w > width || + rectangle.y + rectangle.h > height) + return false; + + return true; +} + /** * \brief Compare sizes for equality * \return True if the two sizes are equal, false otherwise diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h index 32d39127b275..abf5344cf9d8 100644 --- a/src/libcamera/include/camera_sensor.h +++ b/src/libcamera/include/camera_sensor.h @@ -38,6 +38,8 @@ public: const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } const std::vector<Size> &sizes() const { return sizes_; } const Size &resolution() const; + const Size pixelArray() const { return pixelArray_; } + const Rectangle activeArea() const { return activeArea_; } V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, const Size &size) const; @@ -59,6 +61,8 @@ private: CameraLocation location_; unsigned int rotation_; + Size pixelArray_; + Rectangle activeArea_; }; } /* namespace libcamera */
Retrieve the camera sensor pixel array sizes and the active pixel area using the V4L2Subdevice selection API. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/geometry.h | 1 + src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++ src/libcamera/geometry.cpp | 24 +++++++++++++ src/libcamera/include/camera_sensor.h | 4 +++ 4 files changed, 80 insertions(+)