[PATCH 7/9] libcamera: v4l2_subdevice: Add stream support to get/set functions
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Feb 27 17:48:35 CET 2024
Hi Laurent
On Tue, Feb 27, 2024 at 04:09:51PM +0200, Laurent Pinchart wrote:
> Extend the V4L2Subdevice API with stream support for the functions that
> get and set formats and selection rectangles. Add a Stream structure to
> identify a subdev pad and stream, and use it to extend the V4L2Subdevice
> functions that get and set formats and selection rectangles with stream
> support.
>
> To preserve the existing pad-based API, implement overloaded functions
> that wrap the new stream-based API. This allows callers that are not
> stream-aware to use a simpler pad-based API, instead of having to
> explicitly set the stream number to 0 in all API calls.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/v4l2_subdevice.h | 41 +++-
> src/libcamera/v4l2_subdevice.cpp | 232 ++++++++++++++------
> 2 files changed, 202 insertions(+), 71 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 1115cfa55594..65003394a984 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -80,6 +80,11 @@ public:
> ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,
> };
>
> + struct Stream {
> + unsigned int pad;
> + unsigned int stream;
> + };
> +
> class Routing : public std::vector<struct v4l2_subdev_route>
> {
> public:
> @@ -93,17 +98,39 @@ public:
>
> const MediaEntity *entity() const { return entity_; }
>
> - int getSelection(unsigned int pad, unsigned int target,
> + int getSelection(const Stream &stream, unsigned int target,
> Rectangle *rect);
> - int setSelection(unsigned int pad, unsigned int target,
> + int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> + {
> + return getSelection({ pad, 0 }, target, rect);
> + }
> + int setSelection(const Stream &stream, unsigned int target,
> Rectangle *rect);
> + int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)
> + {
> + return setSelection({ pad, 0 }, target, rect);
> + }
>
> - Formats formats(unsigned int pad);
> + Formats formats(const Stream &stream);
> + Formats formats(unsigned int pad)
> + {
> + return formats({ pad, 0 });
> + }
>
> + int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> + Whence whence = ActiveFormat);
> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> + Whence whence = ActiveFormat)
> + {
> + return getFormat({ pad, 0 }, format, whence);
> + }
> + int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> Whence whence = ActiveFormat);
> int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> - Whence whence = ActiveFormat);
> + Whence whence = ActiveFormat)
> + {
> + return setFormat({ pad, 0 }, format, whence);
> + }
>
> int getRouting(Routing *routing, Whence whence = ActiveFormat);
> int setRouting(Routing *routing, Whence whence = ActiveFormat);
> @@ -123,8 +150,8 @@ private:
> std::optional<ColorSpace>
> toColorSpace(const v4l2_mbus_framefmt &format) const;
>
> - std::vector<unsigned int> enumPadCodes(unsigned int pad);
> - std::vector<SizeRange> enumPadSizes(unsigned int pad,
> + std::vector<unsigned int> enumPadCodes(const Stream &stream);
> + std::vector<SizeRange> enumPadSizes(const Stream &stream,
> unsigned int code);
>
> const MediaEntity *entity_;
> @@ -133,4 +160,6 @@ private:
> struct V4L2SubdeviceCapability caps_;
> };
>
> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);
> +
> } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 19ddecf714c6..efe8b0f793e9 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -814,6 +814,35 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)
> * \brief The format operation applies to TRY formats
> */
>
> +/**
> + * \class V4L2Subdevice::Stream
> + * \brief V4L2 subdevice stream
> + *
> + * This class identifies a subdev stream, by bundling the pad number with the
> + * stream number. It is used in all stream-aware functions of the V4L2Subdevice
> + * class to identify the stream the functions operate on.
> + *
> + * \var V4L2Subdevice::Stream::pad
> + * \brief The 0-indexed pad number
> + *
> + * \var V4L2Subdevice::Stream::stream
> + * \brief The stream number
> + */
> +
> +/**
> + * \brief Insert a text representation of a V4L2Subdevice::Stream into an
> + * output stream
What is the alignment rule when we break \brief ? I see different ones
being used in this patch
> + * \param[in] out The output stream
> + * \param[in] stream The V4L2Subdevice::Stream
> + * \return The output stream \a out
> + */
> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)
> +{
> + out << stream.pad << "/" << stream.stream;
> +
> + return out;
> +}
> +
> /**
> * \class V4L2Subdevice::Routing
> * \brief V4L2 subdevice routing table
> @@ -879,7 +908,10 @@ int V4L2Subdevice::open()
> return ret;
> }
>
> - /* If the subdev supports streams, enable the streams API. */
> + /*
> + * If the subdev supports streams, enable the streams API, and retrieve
> + * and cache the routing table.
Does it happen in this patch ?
> + */
> if (caps_.hasStreams()) {
> struct v4l2_subdev_client_capability clientCaps{};
> clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
> @@ -905,7 +937,7 @@ int V4L2Subdevice::open()
>
> /**
> * \brief Get selection rectangle \a rect for \a target
> - * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> + * \param[in] stream The stream the rectangle is retrieved from
> * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> * \param[out] rect The retrieved selection rectangle
> *
> @@ -913,13 +945,14 @@ int V4L2Subdevice::open()
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,
> Rectangle *rect)
> {
> struct v4l2_subdev_selection sel = {};
>
> sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - sel.pad = pad;
> + sel.pad = stream.pad;
> + sel.stream = stream.stream;
> sel.target = target;
> sel.flags = 0;
>
> @@ -927,7 +960,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> if (ret < 0) {
> LOG(V4L2, Error)
> << "Unable to get rectangle " << target << " on pad "
> - << pad << ": " << strerror(-ret);
> + << stream.pad << "/" << stream.stream << ": "
Can't you use operator<<(Stream) ?
> + << strerror(-ret);
> return ret;
> }
>
> @@ -939,9 +973,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> return 0;
> }
>
> +/**
> + * \fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> + * Rectangle *rect)
> + * \brief Get selection rectangle \a rect for \a target
> + * \param[in] pad The 0-indexed pad number the rectangle is retrieved from
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[out] rect The retrieved selection rectangle
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> /**
> * \brief Set selection rectangle \a rect for \a target
> - * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[in] stream The stream the rectangle is to be applied to
> * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> * \param[inout] rect The selection rectangle to be applied
> *
> @@ -949,13 +994,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,
> Rectangle *rect)
> {
> struct v4l2_subdev_selection sel = {};
>
> sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - sel.pad = pad;
> + sel.pad = stream.pad;
> + sel.stream = stream.stream;
> sel.target = target;
> sel.flags = 0;
>
> @@ -968,7 +1014,8 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> if (ret < 0) {
> LOG(V4L2, Error)
> << "Unable to set rectangle " << target << " on pad "
> - << pad << ": " << strerror(-ret);
> + << stream.pad << "/" << stream.stream << ": "
ditto
> + << strerror(-ret);
> return ret;
> }
>
> @@ -979,26 +1026,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
>
> return 0;
> }
> +
> /**
> - * \brief Enumerate all media bus codes and frame sizes on a \a pad
> - * \param[in] pad The 0-indexed pad number to enumerate formats on
> + * \fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> + * Rectangle *rect)
> + * \brief Set selection rectangle \a rect for \a target
> + * \param[in] pad The 0-indexed pad number the rectangle is to be applied to
> + * \param[in] target The selection target defined by the V4L2_SEL_TGT_* flags
> + * \param[inout] rect The selection rectangle to be applied
> + *
> + * \todo Define a V4L2SelectionTarget enum for the selection target
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \brief Enumerate all media bus codes and frame sizes on a \a stream
> + * \param[in] stream The stream to enumerate formats for
> *
> * Enumerate all media bus codes and frame sizes supported by the subdevice on
> - * a \a pad.
> + * a \a stream.
> *
> * \return A list of the supported device formats
> */
> -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)
> {
> Formats formats;
>
> - if (pad >= entity_->pads().size()) {
> - LOG(V4L2, Error) << "Invalid pad: " << pad;
> + if (stream.pad >= entity_->pads().size()) {
> + LOG(V4L2, Error) << "Invalid pad: " << stream.pad;
> return {};
> }
>
> - for (unsigned int code : enumPadCodes(pad)) {
> - std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> + for (unsigned int code : enumPadCodes(stream)) {
> + std::vector<SizeRange> sizes = enumPadSizes(stream, code);
> if (sizes.empty())
> return {};
>
> @@ -1006,7 +1067,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> if (!inserted.second) {
> LOG(V4L2, Error)
> << "Could not add sizes for media bus code "
> - << code << " on pad " << pad;
> + << code << " on pad " << stream.pad;
> return {};
> }
> }
> @@ -1014,6 +1075,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> return formats;
> }
>
> +/**
> + * \fn V4L2Subdevice::formats(unsigned int pad)
> + * \brief Enumerate all media bus codes and frame sizes on a \a pad
> + * \param[in] pad The 0-indexed pad number to enumerate formats on
> + *
> + * Enumerate all media bus codes and frame sizes supported by the subdevice on
> + * a \a pad
> + *
> + * \return A list of the supported device formats
> + */
> +
> std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> {
> /*
> @@ -1045,25 +1117,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
> }
>
> /**
> - * \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
> + * \brief Retrieve the image format set on one of the V4L2 subdevice streams
> + * \param[in] stream The stream the format is to be retrieved from
> * \param[out] format The image bus format
> * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> Whence whence)
> {
> struct v4l2_subdev_format subdevFmt = {};
> subdevFmt.which = whence;
> - subdevFmt.pad = pad;
> + subdevFmt.pad = stream.pad;
> + subdevFmt.stream = stream.stream;
>
> int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);
> if (ret) {
> LOG(V4L2, Error)
> - << "Unable to get format on pad " << pad
> - << ": " << strerror(-ret);
> + << "Unable to get format on pad " << stream.pad << "/"
> + << stream.stream << ": " << strerror(-ret);
> return ret;
> }
>
> @@ -1076,6 +1149,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> }
>
> /**
> + * \fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> + * Whence whence)
> + * \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
> + * \param[out] format The image bus format
> + * \param[in] whence The format to get, \ref V4L2Subdevice::ActiveFormat
> + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \brief Set an image format on one of the V4L2 subdevice pads
> + * \param[in] stream The stream the format is to be applied to
> + * \param[inout] format The image bus format to apply to the stream
> + * \param[in] whence The format to set, \ref V4L2Subdevice::ActiveFormat
> + * "ActiveFormat" or \ref V4L2Subdevice::TryFormat "TryFormat"
> + *
> + * Apply the requested image format to the desired stream and return the
> + * actually applied format parameters, as getFormat() would do.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,
> + Whence whence)
> +{
> + struct v4l2_subdev_format subdevFmt = {};
> + subdevFmt.which = whence;
> + subdevFmt.pad = stream.pad;
> + subdevFmt.stream = stream.stream;
> + subdevFmt.format.width = format->size.width;
> + subdevFmt.format.height = format->size.height;
> + subdevFmt.format.code = format->code;
> + subdevFmt.format.field = V4L2_FIELD_NONE;
> + if (format->colorSpace) {
> + fromColorSpace(format->colorSpace, subdevFmt.format);
> +
> + /* The CSC flag is only applicable to source pads. */
> + if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)
> + subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> + }
> +
> + int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> + if (ret) {
> + LOG(V4L2, Error)
> + << "Unable to set format on pad " << stream.pad << "/"
> + << stream.stream << ": " << strerror(-ret);
> + return ret;
> + }
> +
> + format->size.width = subdevFmt.format.width;
> + format->size.height = subdevFmt.format.height;
> + format->code = subdevFmt.format.code;
> + format->colorSpace = toColorSpace(subdevFmt.format);
> +
> + return 0;
> +}
> +
> +/**
> + * \fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> + * Whence whence)
> * \brief Set an image format on one of the V4L2 subdevice pads
> * \param[in] pad The 0-indexed pad number the format is to be applied to
> * \param[inout] format The image bus format to apply to the subdevice's pad
> @@ -1087,39 +1220,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> - Whence whence)
> -{
> - struct v4l2_subdev_format subdevFmt = {};
> - subdevFmt.which = whence;
> - subdevFmt.pad = pad;
> - subdevFmt.format.width = format->size.width;
> - subdevFmt.format.height = format->size.height;
> - subdevFmt.format.code = format->code;
> - subdevFmt.format.field = V4L2_FIELD_NONE;
> - if (format->colorSpace) {
> - fromColorSpace(format->colorSpace, subdevFmt.format);
> -
> - /* The CSC flag is only applicable to source pads. */
> - if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> - subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> - }
> -
> - int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> - if (ret) {
> - LOG(V4L2, Error)
> - << "Unable to set format on pad " << pad
> - << ": " << strerror(-ret);
> - return ret;
> - }
> -
> - format->size.width = subdevFmt.format.width;
> - format->size.height = subdevFmt.format.height;
> - format->code = subdevFmt.format.code;
> - format->colorSpace = toColorSpace(subdevFmt.format);
> -
> - return 0;
> -}
>
> /**
> * \brief Retrieve the subdevice's internal routing table
> @@ -1282,14 +1382,15 @@ std::string V4L2Subdevice::logPrefix() const
> return "'" + entity_->name() + "'";
> }
>
> -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)
> {
> std::vector<unsigned int> codes;
> int ret;
>
> for (unsigned int index = 0; ; index++) {
> struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> - mbusEnum.pad = pad;
> + mbusEnum.pad = stream.pad;
> + mbusEnum.stream = stream.stream;
> mbusEnum.index = index;
> mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> @@ -1302,15 +1403,15 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
>
> if (ret < 0 && ret != -EINVAL) {
> LOG(V4L2, Error)
> - << "Unable to enumerate formats on pad " << pad
> - << ": " << strerror(-ret);
> + << "Unable to enumerate formats on pad " << stream.pad
> + << "/" << stream.stream << ": " << strerror(-ret);
> return {};
> }
>
> return codes;
> }
>
> -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,
> unsigned int code)
> {
> std::vector<SizeRange> sizes;
> @@ -1319,7 +1420,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> for (unsigned int index = 0;; index++) {
> struct v4l2_subdev_frame_size_enum sizeEnum = {};
> sizeEnum.index = index;
> - sizeEnum.pad = pad;
> + sizeEnum.pad = stream.pad;
> + sizeEnum.stream = stream.stream;
> sizeEnum.code = code;
> sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> @@ -1333,8 +1435,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>
> if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> LOG(V4L2, Error)
> - << "Unable to enumerate sizes on pad " << pad
> - << ": " << strerror(-ret);
> + << "Unable to enumerate sizes on pad " << stream.pad
> + << "/" << stream.stream << ": " << strerror(-ret);
> return {};
The only question I have is if we should allow the V4L2Subdevice API
to expose a pad-only interface or we should bite the bullet and move
it to use Stream. The reason is that, a open() time
/* If the subdev supports streams, enable the streams API. */
if (caps_.hasStreams()) {
struct v4l2_subdev_client_capability clientCaps{};
clientCaps.capabilities = V4L2_SUBDEV_CLIENT_CAP_STREAMS;
ret = ioctl(VIDIOC_SUBDEV_S_CLIENT_CAP, &clientCaps);
And allowing users to use the pad-based API would make stream == 0
implicitly. It's a lot of work though, not sure it's worth it ?
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list