[libcamera-devel] [PATCH v3 2/8] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 28 18:23:03 CET 2019
Hi Jacopo,
On Thu, Feb 28, 2019 at 09:44:31AM +0100, Jacopo Mondi wrote:
> On Wed, Feb 27, 2019 at 07:51:20PM +0200, Laurent Pinchart wrote:
> > 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 ?
>
> Ok
>
> >>> 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() ?
> >
>
> Good point. But do we want to enumerate formats everytime we open a
> video device, or do it once and store them through open/close.
>
> If I use a pointer to store the format vector (or map as you suggested)
> I can easly find out if that's the first time the subdevice got
> opened, and create the format list once.
>
> >>>> 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
> > ?
>
> Ack, as per the above point, having the formats as pointers makes it
> easier to find out if they have been ever enumerated or not.
>
> >>>> + 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
>
> Are they?
It's too easy to forget an = and type = instead of == in a conditional
expression, so forbidding assignments makes it easier to catch such
errors as they would always be invalid.
> >
> > while (1) {
> > ret = ioctl(...);
> > if (ret < 0)
> > break;
>
> Sure, no problem
>
> >>>> + 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.
>
> Thanks. How does that work on structures with only the default
> compiler-created constructor?
I think you can use an initializer list.
> >>>> +
> >>>> + /*
> >>>> + * 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.
>
> It really puzzled me how to better handle this. If we store min/max as
> well, how are users of the subdevice expected to handle this? They
> shall be aware that the formats can either be a single format with a
> min/max range, or an enumeration of discrete sizes. I mean, it's
> surely doable, I wonder how to express it clearly, in both
> documentation and code. It's not unexpected though, as it replicates
> the SUBDEV_ENUM_FRAME_SIZE behavior, so maybe it's enough to repeat
> the same here, but I wanted to avoid this ambiguity in our
> implementation. I guess it's not easily doable.
I don't think we can do that easily, and your attempt created an even
bigger problem :-)
> >>>> + 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 ?
>
> That's a good suggestion, I'll try and see how it looks like.
>
> >>>> + 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 ?
>
> Is there anything in the V4L2 APIs preventing this? I haven't find
> anything in the documentation that says so. I would be happy to drop
> this, if we consider this case non-standard and not worth supporting.
I think it would be a case of non-compliance, so I don't think we need
to support it.
> >>>> + 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