[libcamera-devel] [PATCH v3 2/8] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES
Jacopo Mondi
jacopo at jmondi.org
Thu Feb 28 09:44:31 CET 2019
HI Laurent,
thanks for your comments
On Wed, Feb 27, 2019 at 07:51:20PM +0200, Laurent Pinchart wrote:
> 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 ?
>
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?
>
> 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?
>
> > >> +
> > >> + /*
> > >> + * 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.
> > >> + 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.
Thanks
j
> > >> + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190228/22d95682/attachment-0001.sig>
More information about the libcamera-devel
mailing list