[libcamera-devel] [PATCH v3 2/8] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Feb 27 18:51:20 CET 2019
Hi Jacopo,
Thank you for the patch.
On Wed, Feb 27, 2019 at 09:18:04AM +0100, Jacopo Mondi wrote:
> On Tue, Feb 26, 2019 at 11:19:40PM +0000, Kieran Bingham wrote:
> > On 26/02/2019 16:26, Jacopo Mondi wrote:
> >> Implement enumFormat() methods to enumerate the available image
> >> resolutions on the subdevice.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> src/libcamera/include/v4l2_subdevice.h | 9 ++
> >> src/libcamera/v4l2_subdevice.cpp | 118 +++++++++++++++++++++++++
> >> 2 files changed, 127 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> >> index eac699a06109..6b21308d2087 100644
> >> --- a/src/libcamera/include/v4l2_subdevice.h
> >> +++ b/src/libcamera/include/v4l2_subdevice.h
> >> @@ -7,7 +7,9 @@
> >> #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__
> >> #define __LIBCAMERA_V4L2_SUBDEVICE_H__
> >>
> >> +#include <map>
> >> #include <string>
> >> +#include <vector>
> >>
> >> #include "media_object.h"
> >>
> >> @@ -38,15 +40,22 @@ public:
> >> int setCrop(unsigned int pad, Rectangle *rect);
> >> int setCompose(unsigned int pad, Rectangle *rect);
> >>
> >> + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad);
Shouldn't you return a const reference ? And make this function const ?
> >> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >> int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >>
> >> private:
> >> + int listPadSizes(unsigned int pad, unsigned int mbus_code,
> >> + std::vector<V4L2SubdeviceFormat> *formats);
> >> + std::vector<V4L2SubdeviceFormat> listPadFormats(unsigned int pad);
> >
> > The word 'list' seems a bit redundant in 'listPadSizes()' and
> > 'listPadFormats()' ... and seems a bit hungarian notation to me?
>
> I interpreted "list" as a verb, so the function name reads like "list
> all formats on a pad". Is it weird to you?
How about s/list/enumerate/ to remove all ambiguity ?
> > Other than that, I trust that the tests that follow make sure the code
> > is doing something sane :)
> >
> > I can't see anything jump out at me yet.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >> +
> >> int setSelection(unsigned int pad, unsigned int target,
> >> Rectangle *rect);
> >>
> >> const MediaEntity *entity_;
> >> int fd_;
> >> +
> >> + std::map<unsigned int, std::vector<V4L2SubdeviceFormat>> formats_;
> >> };
> >>
> >> } /* namespace libcamera */
> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> >> index 56ecf3851cb0..f81a521f9e2a 100644
> >> --- a/src/libcamera/v4l2_subdevice.cpp
> >> +++ b/src/libcamera/v4l2_subdevice.cpp
> >> @@ -5,6 +5,10 @@
> >> * v4l2_subdevice.cpp - V4L2 Subdevice
> >> */
> >>
> >> +#include <map>
> >> +#include <string>
> >> +#include <vector>
> >> +
Not strictly needed as the header includes them.
> >> #include <fcntl.h>
> >> #include <string.h>
> >> #include <sys/ioctl.h>
> >> @@ -116,6 +120,9 @@ int V4L2Subdevice::open()
> >> }
> >> fd_ = ret;
> >>
> >> + for (MediaPad *pad : entity_->pads())
> >> + formats_[pad->index()] = listPadFormats(pad->index());
> >> +
Should you do a formats_.clear() on close() ?
> >> return 0;
> >> }
> >>
> >> @@ -178,6 +185,25 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> >> return setSelection(pad, V4L2_SEL_TGT_COMPOSE, rect);
> >> }
> >>
> >> +/**
> >> + * \brief List the sub-device image resolutions and formats on \a pad
s/List/Retrieve/ ?
> >> + * \param[in] pad The 0-indexed pad number to enumerate formats on
"to retrieve formats for" ?
> >> + *
> >> + * \return A vector of image formats, or an empty vector if the pad does not
> >> + * exist
> >> + */
> >> +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad)
> >> +{
> >> + /*
> >> + * If pad does not exist, return an empty vector at position
> >> + * pads().size()
> >> + */
This will prevent differentiating between a non-existing pad and a pad
that has no format. How about returning a pointer instead of a reference
?
> >> + if (pad > entity_->pads().size())
> >> + pad = entity_->pads().size();
> >> +
> >> + return formats_[pad];
> >> +}
> >> +
> >> /**
> >> * \brief Retrieve the image format set on one of the V4L2 subdevice pads
> >> * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> >> @@ -242,6 +268,98 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> >> return 0;
> >> }
> >>
> >> +int V4L2Subdevice::listPadSizes(unsigned int pad, unsigned int mbus_code,
> >> + std::vector<V4L2SubdeviceFormat> *formats)
> >> +{
> >> + struct v4l2_subdev_frame_size_enum sizeEnum = {};
> >> + int ret;
> >> +
> >> + sizeEnum.index = 0;
> >> + sizeEnum.pad = pad;
> >> + sizeEnum.code = mbus_code;
> >> + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >> +
> >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))) {
Assignments in conditional expressions are discouraged. How about
while (1) {
ret = ioctl(...);
if (ret < 0)
break;
> >> + V4L2SubdeviceFormat minFormat = {
> >> + .mbus_code = mbus_code,
> >> + .width = sizeEnum.min_width,
> >> + .height = sizeEnum.min_height,
> >> + };
> >> + formats->push_back(minFormat);
You can use emplace_back() instead of creating a local temporary
variable.
> >> +
> >> + /*
> >> + * Most subdevices report discrete frame resolutions, where
> >> + * min and max sizes are identical. For continue frame
> >> + * resolutions, store the min and max sizes interval.
> >> + */
That's a bit of a hack. How is a caller supposed to tell if two
consecutive entries in the list are min, max values or discrete values ?
How about creating a SizeRange class (in geometry.h) to store min and
max width and height, and using a std::vector<SizeRange> ? That would
also have the benefit of not storing the mbus code in all entries.
> >> + if (sizeEnum.min_width == sizeEnum.max_width &&
> >> + sizeEnum.min_height == sizeEnum.max_height) {
> >> + sizeEnum.index++;
> >> + continue;
> >> + }
> >> +
> >> + V4L2SubdeviceFormat maxFormat = {
> >> + .mbus_code = mbus_code,
> >> + .width = sizeEnum.max_width,
> >> + .height = sizeEnum.max_height,
> >> + };
> >> + formats->push_back(maxFormat);
emplace_back() here too.
> >> +
> >> + sizeEnum.index++;
> >> + }
> >> +
> >> + if (ret && (errno != EINVAL && errno != ENOTTY)) {
> >> + LOG(V4L2Subdev, Error)
> >> + << "Unable to enumerate format on pad " << pad
> >> + << " of " << deviceName() << ": "
> >> + << strerror(errno);
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +std::vector<V4L2SubdeviceFormat> V4L2Subdevice::listPadFormats(unsigned int pad)
> >> +{
> >> + struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> >> + std::vector<V4L2SubdeviceFormat> formats = {};
> >> + int ret;
> >> +
> >> + mbusEnum.pad = pad;
> >> + mbusEnum.index = 0;
> >> + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >> +
> >> + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) {
Same comment here.
> >> + ret = listPadSizes(pad, mbusEnum.code, &formats);
How about making format a std::map<unsigned int, std:vector<SizeRange>>
to store the mbus code ?
> >> + if (ret)
> >> + return formats;
> >> +
> >> + mbusEnum.index++;
> >> + }
> >> +
> >> + if (ret && (errno != EINVAL && errno != ENOTTY)) {
> >> + LOG(V4L2Subdev, Error)
> >> + << "Unable to enumerate format on pad " << pad
> >> + << " of " << deviceName() << ": " << strerror(-ret);
> >> + return formats;
> >> + }
> >> +
> >> + if (mbusEnum.index == 0 && ret && errno == EINVAL) {
> >> + /*
> >> + * The subdevice might not support ENUM_MBUS_CODE but
> >> + * might support ENUM_FRAME_SIZES.
> >> + */
Can that happen ?
> >> + struct V4L2SubdeviceFormat subdevFormat;
> >> + ret = getFormat(pad, &subdevFormat);
> >> + if (ret)
> >> + return formats;
> >> +
> >> + listPadSizes(pad, subdevFormat.mbus_code, &formats);
> >> + }
> >> +
> >> + return formats;
> >> +}
> >> +
> >> int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> >> Rectangle *rect)
> >> {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list