[libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor: Retrieve sensor sizes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 3 22:17:41 CEST 2019
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 at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list