[libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_subdevice: Implement ENUM_FRAME_SIZES
Jacopo Mondi
jacopo at jmondi.org
Tue Feb 26 09:27:36 CET 2019
Hi Niklas,
thanks for review.
On Tue, Feb 26, 2019 at 03:39:42AM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-02-25 13:10:33 +0100, 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 | 8 +++
> > src/libcamera/v4l2_subdevice.cpp | 93 ++++++++++++++++++++++++++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index fcfbee5af106..cb033a76933c 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,16 +40,22 @@ public:
> > int setCrop(unsigned int pad, Rectangle *rect);
> > int setCompose(unsigned int pad, Rectangle *rect);
> >
> > + std::vector<V4L2SubdeviceFormat> &formats(unsigned int pad);
>
> As you return a referent to the internal data structure should it not
> at lest be const?
Possibly, yes. I'll try to make it const.
>
> > int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> > int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> >
> > private:
> > + int listPadFormats(unsigned int pad,
> > + std::vector<V4L2SubdeviceFormat> *formats);
> > + void listFormats();
> > int setSelection(unsigned int pad, unsigned int target,
> > Rectangle *rect);
> >
> > const MediaEntity *entity_;
> > std::string deviceNode_;
> > 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 ebf87f0124cb..0e9c654579dc 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>
> > +
> > #include <fcntl.h>
> > #include <string.h>
> > #include <sys/ioctl.h>
> > @@ -116,6 +120,8 @@ int V4L2Subdevice::open()
> > }
> > fd_ = ret;
> >
> > + listFormats();
> > +
>
> I would inline the function here, but I won't push it as its mostly
> taste.
>
Actually not sure I like it.
> > return 0;
> > }
> >
> > @@ -178,6 +184,17 @@ 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
> > + * \param[in] pad The 0-indexed pad number to enumerate formats on
> > + *
> > + * \return A vector of image formats, or an empty vector on error
> > + */
> > +std::vector<V4L2SubdeviceFormat> &V4L2Subdevice::formats(unsigned int pad)
> > +{
> > + return formats_[pad];
>
> Maybe add a bounds checking on pad here? What would happen if a pad that
> don't exists is requested?
>
My understanding is that an empty vector is constructed and returned
in such a case (and gets stored in the formats map, this is a side
effect we might want to avoid maybe).
> > +}
> > +
> > /**
> > * \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 +259,82 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format)
> > return 0;
> > }
> >
> > +int V4L2Subdevice::listPadFormats(unsigned int pad,
> > + std::vector<V4L2SubdeviceFormat> *formats)
> > +{
> > + struct v4l2_subdev_frame_size_enum sizeEnum = {};
> > + struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > + int ret;
> > +
> > + mbusEnum.index = 0;
> > + mbusEnum.pad = pad;
> > + mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +
> > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum))) {
> > + sizeEnum.index = 0;
> > + sizeEnum.code = mbusEnum.code;
> > + sizeEnum.pad = pad;
> > + sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +
> > + while (!(ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_FRAME_SIZE,
> > + &sizeEnum))) {
> > +
> > + /* Store the minimum and maximum reported sizes. */
> > + V4L2SubdeviceFormat minFormat = {
> > + .mbus_code = mbusEnum.code,
> > + .width = sizeEnum.min_width,
> > + .height = sizeEnum.min_height,
> > + };
> > + formats->push_back(minFormat);
> > +
> > + V4L2SubdeviceFormat maxFormat = {
> > + .mbus_code = mbusEnum.code,
> > + .width = sizeEnum.max_width,
> > + .height = sizeEnum.max_height,
> > + };
> > + formats->push_back(maxFormat);
> > +
> > + sizeEnum.index++;
> > + }
> > +
> > + if (-errno != -EINVAL) {
> > + LOG(V4L2Subdev, Error)
> > + << "Unable to enumerate format on pad " << pad
> > + << " of " << deviceNode_ << ": "
> > + << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + mbusEnum.index++;
> > + }
> > +
> > + if (-errno != -EINVAL) {
> > + LOG(V4L2Subdev, Error)
> > + << "Unable to enumerate format on pad " << pad
> > + << " of " << deviceNode_ << ": " << strerror(-ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void V4L2Subdevice::listFormats()
> > +{
> > + int ret;
> > +
> > + for (MediaPad *pad : entity_->pads()) {
> > + std::vector<V4L2SubdeviceFormat> formats = {};
> > + ret = listPadFormats(pad->index(), &formats);
>
> Can't listPadFormats() return the vector instead of taking a pointer as
> an argument? With copy elision there would be no performance impact.
>
> > + if (ret) {
> > + formats = {};
> > + formats_[pad->index()] = formats;
> > + continue;
> > + }
> > +
> > + formats_[pad->index()] = formats;
>
> This can be simplified, how about
>
> for (MediaPad *pad : entity_->pads()) {
> std::vector<V4L2SubdeviceFormat> formats = {};
> ret = listPadFormats(pad->index(), &formats);
> if (ret)
> formats = {}
>
> formats_[pad->index()] = formats;
> }
>
>
> Or with the return type of listPadFormats() changed
>
> for (MediaPad *pad : entity_->pads())
> formats_[pad->index()] = listPadFormats(pad->index());
>
>
Thanks for both suggestions, it looks better.
Anyway, as soon as I've run this code on a 'real' device, I realized
it needs to handle gracefully the case where that ioctl is not
implemented (-ENOTTY), so this code will likely be re-writted in v3.
Thanks
j
> > + }
> > +}
> > +
> > int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> > Rectangle *rect)
> > {
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20190226/46440a23/attachment-0001.sig>
More information about the libcamera-devel
mailing list