[{"id":28975,"web_url":"https://patchwork.libcamera.org/comment/28975/","msgid":"<171049919958.1011926.5152771841307159861@ping.linuxembedded.co.uk>","date":"2024-03-15T10:39:59","subject":"Re: [PATCH v2 07/14] libcamera: v4l2_subdevice: Add stream support\n\tto get/set functions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-03-15 00:16:06)\n> Extend the V4L2Subdevice API with stream support for the functions that\n> get and set formats and selection rectangles. Add a Stream structure to\n> identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> functions that get and set formats and selection rectangles with stream\n> support.\n> \n> To preserve the existing pad-based API, implement overloaded functions\n> that wrap the new stream-based API. This allows callers that are not\n> stream-aware to use a simpler pad-based API, instead of having to\n> explicitly set the stream number to 0 in all API calls.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n> Changes since combined RFC:\n> \n> - Fix indentation in comments\n> - Use operator<<(Stream) everywhere\n> \n> Changes since v1:\n> \n> - Drop incorrect comment change\n> - Use operator<<(Stream)\n> - Add comparison operators\n> ---\n>  include/libcamera/internal/v4l2_subdevice.h |  58 ++++-\n>  src/libcamera/v4l2_subdevice.cpp            | 248 +++++++++++++++-----\n>  2 files changed, 238 insertions(+), 68 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 1115cfa55594..6cd36730371a 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -80,6 +80,21 @@ public:\n>                 ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n>         };\n>  \n> +       struct Stream {\n> +               Stream()\n> +                       : pad(0), stream(0)\n> +               {\n> +               }\n> +\n> +               Stream(unsigned int p, unsigned int s)\n> +                       : pad(p), stream(s)\n> +               {\n> +               }\n> +\n> +               unsigned int pad;\n> +               unsigned int stream;\n> +       };\n> +\n>         class Routing : public std::vector<struct v4l2_subdev_route>\n>         {\n>         public:\n> @@ -93,17 +108,39 @@ public:\n>  \n>         const MediaEntity *entity() const { return entity_; }\n>  \n> -       int getSelection(unsigned int pad, unsigned int target,\n> +       int getSelection(const Stream &stream, unsigned int target,\n>                          Rectangle *rect);\n> -       int setSelection(unsigned int pad, unsigned int target,\n> +       int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> +       {\n> +               return getSelection({ pad, 0 }, target, rect);\n\nAha nice indeed.\n\nI wonder if createing a constructor for Stream that accepts an integer\npad, and defaults the stream to 0 would remove the need even for the\noverloads, but I think \"automagcially converting int's to streams\" could\npotentially cause confusion, so I don't mind having the explicit\noverloads.\n\nIt would reduce the additional / repeated function documentation\nadditions though.\n\n\n> +       }\n> +       int setSelection(const Stream &stream, unsigned int target,\n>                          Rectangle *rect);\n> +       int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> +       {\n> +               return setSelection({ pad, 0 }, target, rect);\n> +       }\n>  \n> -       Formats formats(unsigned int pad);\n> +       Formats formats(const Stream &stream);\n> +       Formats formats(unsigned int pad)\n> +       {\n> +               return formats({ pad, 0 });\n> +       }\n>  \n> +       int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> +                     Whence whence = ActiveFormat);\n>         int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +                     Whence whence = ActiveFormat)\n> +       {\n> +               return getFormat({ pad, 0 }, format, whence);\n> +       }\n> +       int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n>                       Whence whence = ActiveFormat);\n>         int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> -                     Whence whence = ActiveFormat);\n> +                     Whence whence = ActiveFormat)\n> +       {\n> +               return setFormat({ pad, 0 }, format, whence);\n> +       }\n>  \n>         int getRouting(Routing *routing, Whence whence = ActiveFormat);\n>         int setRouting(Routing *routing, Whence whence = ActiveFormat);\n> @@ -123,8 +160,8 @@ private:\n>         std::optional<ColorSpace>\n>         toColorSpace(const v4l2_mbus_framefmt &format) const;\n>  \n> -       std::vector<unsigned int> enumPadCodes(unsigned int pad);\n> -       std::vector<SizeRange> enumPadSizes(unsigned int pad,\n> +       std::vector<unsigned int> enumPadCodes(const Stream &stream);\n> +       std::vector<SizeRange> enumPadSizes(const Stream &stream,\n>                                             unsigned int code);\n>  \n>         const MediaEntity *entity_;\n> @@ -133,4 +170,13 @@ private:\n>         struct V4L2SubdeviceCapability caps_;\n>  };\n>  \n> +bool operator==(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs);\n> +static inline bool operator!=(const V4L2Subdevice::Stream &lhs,\n> +                             const V4L2Subdevice::Stream &rhs)\n> +{\n> +       return !(lhs == rhs);\n> +}\n> +\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 66da1edbf542..cc079425bb16 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -842,6 +842,62 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n>   * \\brief The format operation applies to TRY formats\n>   */\n>  \n> +/**\n> + * \\class V4L2Subdevice::Stream\n> + * \\brief V4L2 subdevice stream\n> + *\n> + * This class identifies a subdev stream, by bundling the pad number with the\n> + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> + * class to identify the stream the functions operate on.\n> + *\n> + * \\var V4L2Subdevice::Stream::pad\n> + * \\brief The 0-indexed pad number\n> + *\n> + * \\var V4L2Subdevice::Stream::stream\n> + * \\brief The stream number\n> + */\n> +\n> +/**\n> + * \\fn V4L2Subdevice::Stream::Stream()\n> + * \\brief Construct a Stream with pad and stream set to 0\n> + */\n> +\n> +/**\n> + * \\fn V4L2Subdevice::Stream::Stream(unsigned int pad, unsigned int stream)\n> + * \\brief Construct a Stream with a given \\a pad and \\a stream number\n> + * \\param[in] pad The indexed pad number\n> + * \\param[in] stream The stream number\n> + */\n> +\n> +/**\n> + * \\brief Compare streams for equality\n> + * \\return True if the two streams are equal, false otherwise\n> + */\n> +bool operator==(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs)\n> +{\n> +       return lhs.pad == rhs.pad && lhs.stream == rhs.stream;\n> +}\n> +\n> +/**\n> + * \\fn bool operator!=(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs)\n> + * \\brief Compare streams for inequality\n> + * \\return True if the two streams are not equal, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> + * output stream\n> + * \\param[in] out The output stream\n> + * \\param[in] stream The V4L2Subdevice::Stream\n> + * \\return The output stream \\a out\n> + */\n> +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> +{\n> +       out << stream.pad << \"/\" << stream.stream;\n> +\n> +       return out;\n> +}\n> +\n>  /**\n>   * \\class V4L2Subdevice::Routing\n>   * \\brief V4L2 subdevice routing table\n> @@ -933,7 +989,7 @@ int V4L2Subdevice::open()\n>  \n>  /**\n>   * \\brief Get selection rectangle \\a rect for \\a target\n> - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> + * \\param[in] stream The stream the rectangle is retrieved from\n>   * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n>   * \\param[out] rect The retrieved selection rectangle\n>   *\n> @@ -941,13 +997,14 @@ int V4L2Subdevice::open()\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n>                                 Rectangle *rect)\n>  {\n>         struct v4l2_subdev_selection sel = {};\n>  \n>         sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> -       sel.pad = pad;\n> +       sel.pad = stream.pad;\n> +       sel.stream = stream.stream;\n>         sel.target = target;\n>         sel.flags = 0;\n>  \n> @@ -955,7 +1012,7 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n>         if (ret < 0) {\n>                 LOG(V4L2, Error)\n>                         << \"Unable to get rectangle \" << target << \" on pad \"\n> -                       << pad << \": \" << strerror(-ret);\n> +                       << stream << \": \" << strerror(-ret);\n>                 return ret;\n>         }\n>  \n> @@ -967,9 +1024,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n>         return 0;\n>  }\n>  \n> +/**\n> + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> + * Rectangle *rect)\n> + * \\brief Get selection rectangle \\a rect for \\a target\n> + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> + * \\param[out] rect The retrieved selection rectangle\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n>  /**\n>   * \\brief Set selection rectangle \\a rect for \\a target\n> - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[in] stream The stream the rectangle is to be applied to\n>   * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n>   * \\param[inout] rect The selection rectangle to be applied\n>   *\n> @@ -977,13 +1045,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n>                                 Rectangle *rect)\n>  {\n>         struct v4l2_subdev_selection sel = {};\n>  \n>         sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> -       sel.pad = pad;\n> +       sel.pad = stream.pad;\n> +       sel.stream = stream.stream;\n>         sel.target = target;\n>         sel.flags = 0;\n>  \n> @@ -996,7 +1065,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n>         if (ret < 0) {\n>                 LOG(V4L2, Error)\n>                         << \"Unable to set rectangle \" << target << \" on pad \"\n> -                       << pad << \": \" << strerror(-ret);\n> +                       << stream << \": \" << strerror(-ret);\n>                 return ret;\n>         }\n>  \n> @@ -1007,26 +1076,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n>  \n>         return 0;\n>  }\n> +\n>  /**\n> - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> + * Rectangle *rect)\n> + * \\brief Set selection rectangle \\a rect for \\a target\n> + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> + * \\param[inout] rect The selection rectangle to be applied\n> + *\n> + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> + * \\param[in] stream The stream to enumerate formats for\n>   *\n>   * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> - * a \\a pad.\n> + * a \\a stream.\n>   *\n>   * \\return A list of the supported device formats\n>   */\n> -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n>  {\n>         Formats formats;\n>  \n> -       if (pad >= entity_->pads().size()) {\n> -               LOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> +       if (stream.pad >= entity_->pads().size()) {\n> +               LOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n>                 return {};\n>         }\n>  \n> -       for (unsigned int code : enumPadCodes(pad)) {\n> -               std::vector<SizeRange> sizes = enumPadSizes(pad, code);\n> +       for (unsigned int code : enumPadCodes(stream)) {\n> +               std::vector<SizeRange> sizes = enumPadSizes(stream, code);\n>                 if (sizes.empty())\n>                         return {};\n>  \n> @@ -1034,7 +1117,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n>                 if (!inserted.second) {\n>                         LOG(V4L2, Error)\n>                                 << \"Could not add sizes for media bus code \"\n> -                               << code << \" on pad \" << pad;\n> +                               << code << \" on pad \" << stream.pad;\n>                         return {};\n>                 }\n>         }\n> @@ -1042,6 +1125,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n>         return formats;\n>  }\n>  \n> +/**\n> + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> + *\n> + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> + * a \\a pad\n> + *\n\n \\sa V4L2Subdevice::formats(Stream)\n\nor is that overkill? Probably.\n\n\n> + * \\return A list of the supported device formats\n> + */\n> +\n>  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n>  {\n>         /*\n> @@ -1073,25 +1167,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n>  }\n>  \n>  /**\n> - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> + * \\param[in] stream The stream the format is to be retrieved from\n>   * \\param[out] format The image bus format\n>   * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n>   * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n>                              Whence whence)\n>  {\n>         struct v4l2_subdev_format subdevFmt = {};\n>         subdevFmt.which = whence;\n> -       subdevFmt.pad = pad;\n> +       subdevFmt.pad = stream.pad;\n> +       subdevFmt.stream = stream.stream;\n>  \n>         int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n>         if (ret) {\n>                 LOG(V4L2, Error)\n> -                       << \"Unable to get format on pad \" << pad\n> -                       << \": \" << strerror(-ret);\n> +                       << \"Unable to get format on pad \" << stream << \": \"\n> +                       << strerror(-ret);\n>                 return ret;\n>         }\n>  \n> @@ -1104,6 +1199,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>  }\n>  \n>  /**\n> + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> + * Whence whence)\n> + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> + * \\param[out] format The image bus format\n> + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\brief Set an image format on one of the V4L2 subdevice pads\n> + * \\param[in] stream The stream the format is to be applied to\n> + * \\param[inout] format The image bus format to apply to the stream\n> + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> + *\n> + * Apply the requested image format to the desired stream and return the\n> + * actually applied format parameters, as getFormat() would do.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> +                            Whence whence)\n> +{\n> +       struct v4l2_subdev_format subdevFmt = {};\n> +       subdevFmt.which = whence;\n> +       subdevFmt.pad = stream.pad;\n> +       subdevFmt.stream = stream.stream;\n> +       subdevFmt.format.width = format->size.width;\n> +       subdevFmt.format.height = format->size.height;\n> +       subdevFmt.format.code = format->code;\n> +       subdevFmt.format.field = V4L2_FIELD_NONE;\n> +       if (format->colorSpace) {\n> +               fromColorSpace(format->colorSpace, subdevFmt.format);\n> +\n> +               /* The CSC flag is only applicable to source pads. */\n> +               if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> +                       subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> +       }\n> +\n> +       int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> +       if (ret) {\n> +               LOG(V4L2, Error)\n> +                       << \"Unable to set format on pad \" << stream << \": \"\n> +                       << strerror(-ret);\n> +               return ret;\n> +       }\n> +\n> +       format->size.width = subdevFmt.format.width;\n> +       format->size.height = subdevFmt.format.height;\n> +       format->code = subdevFmt.format.code;\n> +       format->colorSpace = toColorSpace(subdevFmt.format);\n> +\n> +       return 0;\n> +}\n> +\n> +/**\n> + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> + * Whence whence)\n>   * \\brief Set an image format on one of the V4L2 subdevice pads\n>   * \\param[in] pad The 0-indexed pad number the format is to be applied to\n>   * \\param[inout] format The image bus format to apply to the subdevice's pad\n> @@ -1115,39 +1270,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>   *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> -                            Whence whence)\n> -{\n> -       struct v4l2_subdev_format subdevFmt = {};\n> -       subdevFmt.which = whence;\n> -       subdevFmt.pad = pad;\n> -       subdevFmt.format.width = format->size.width;\n> -       subdevFmt.format.height = format->size.height;\n> -       subdevFmt.format.code = format->code;\n> -       subdevFmt.format.field = V4L2_FIELD_NONE;\n> -       if (format->colorSpace) {\n> -               fromColorSpace(format->colorSpace, subdevFmt.format);\n> -\n> -               /* The CSC flag is only applicable to source pads. */\n> -               if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> -                       subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> -       }\n> -\n> -       int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> -       if (ret) {\n> -               LOG(V4L2, Error)\n> -                       << \"Unable to set format on pad \" << pad\n> -                       << \": \" << strerror(-ret);\n> -               return ret;\n> -       }\n> -\n> -       format->size.width = subdevFmt.format.width;\n> -       format->size.height = subdevFmt.format.height;\n> -       format->code = subdevFmt.format.code;\n> -       format->colorSpace = toColorSpace(subdevFmt.format);\n> -\n> -       return 0;\n> -}\n>  \n>  /**\n>   * \\brief Retrieve the subdevice's internal routing table\n> @@ -1310,14 +1432,15 @@ std::string V4L2Subdevice::logPrefix() const\n>         return \"'\" + entity_->name() + \"'\";\n>  }\n>  \n> -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n> +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)\n>  {\n>         std::vector<unsigned int> codes;\n>         int ret;\n>  \n>         for (unsigned int index = 0; ; index++) {\n>                 struct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> -               mbusEnum.pad = pad;\n> +               mbusEnum.pad = stream.pad;\n> +               mbusEnum.stream = stream.stream;\n>                 mbusEnum.index = index;\n>                 mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>  \n> @@ -1330,7 +1453,7 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n>  \n>         if (ret < 0 && ret != -EINVAL) {\n>                 LOG(V4L2, Error)\n> -                       << \"Unable to enumerate formats on pad \" << pad\n> +                       << \"Unable to enumerate formats on pad \" << stream\n>                         << \": \" << strerror(-ret);\n>                 return {};\n>         }\n> @@ -1338,7 +1461,7 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n>         return codes;\n>  }\n>  \n> -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n> +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,\n>                                                    unsigned int code)\n>  {\n>         std::vector<SizeRange> sizes;\n> @@ -1347,7 +1470,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n>         for (unsigned int index = 0;; index++) {\n>                 struct v4l2_subdev_frame_size_enum sizeEnum = {};\n>                 sizeEnum.index = index;\n> -               sizeEnum.pad = pad;\n> +               sizeEnum.pad = stream.pad;\n> +               sizeEnum.stream = stream.stream;\n>                 sizeEnum.code = code;\n>                 sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n>  \n> @@ -1361,7 +1485,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n>  \n>         if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n>                 LOG(V4L2, Error)\n> -                       << \"Unable to enumerate sizes on pad \" << pad\n> +                       << \"Unable to enumerate sizes on pad \" << stream\n\nShould this now say \"on stream\"? Or do we really want to still say \"on\npad\" here?\n\nFine either way though and everything above is all optional too so:\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n>                         << \": \" << strerror(-ret);\n>                 return {};\n>         }\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 50D08BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 10:40:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 760F962C8C;\n\tFri, 15 Mar 2024 11:40:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E1686295D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 11:40:02 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E1BDB3;\n\tFri, 15 Mar 2024 11:39:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gfEi5ti9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710499178;\n\tbh=qIeEbuZxmLpe50CqClmYrS97xfE7Tym80NViH1s+dTU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=gfEi5ti9jPBscwdQG3i62Vj7/2d9PBX2U4363602FEtqR6rYCH5vqAYgzYq2nGs5i\n\t5N5xhYJuSWnsLhGuBbNGWGGp6Qilmfm8s4YqlWc4k6gwHgQf2/5Bl0BXNGLkukzTQY\n\tk4ek2fY2lO+oTCGnsw1qXMmpz2oIyjSoJYUoitO0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240315001613.2033-8-laurent.pinchart@ideasonboard.com>","References":"<20240315001613.2033-1-laurent.pinchart@ideasonboard.com>\n\t<20240315001613.2033-8-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH v2 07/14] libcamera: v4l2_subdevice: Add stream support\n\tto get/set functions","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Mar 2024 10:39:59 +0000","Message-ID":"<171049919958.1011926.5152771841307159861@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":28977,"web_url":"https://patchwork.libcamera.org/comment/28977/","msgid":"<20240315104449.GF2441@pendragon.ideasonboard.com>","date":"2024-03-15T10:44:49","subject":"Re: [PATCH v2 07/14] libcamera: v4l2_subdevice: Add stream support\n\tto get/set functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Fri, Mar 15, 2024 at 10:39:59AM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-03-15 00:16:06)\n> > Extend the V4L2Subdevice API with stream support for the functions that\n> > get and set formats and selection rectangles. Add a Stream structure to\n> > identify a subdev pad and stream, and use it to extend the V4L2Subdevice\n> > functions that get and set formats and selection rectangles with stream\n> > support.\n> > \n> > To preserve the existing pad-based API, implement overloaded functions\n> > that wrap the new stream-based API. This allows callers that are not\n> > stream-aware to use a simpler pad-based API, instead of having to\n> > explicitly set the stream number to 0 in all API calls.\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > ---\n> > Changes since combined RFC:\n> > \n> > - Fix indentation in comments\n> > - Use operator<<(Stream) everywhere\n> > \n> > Changes since v1:\n> > \n> > - Drop incorrect comment change\n> > - Use operator<<(Stream)\n> > - Add comparison operators\n> > ---\n> >  include/libcamera/internal/v4l2_subdevice.h |  58 ++++-\n> >  src/libcamera/v4l2_subdevice.cpp            | 248 +++++++++++++++-----\n> >  2 files changed, 238 insertions(+), 68 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> > index 1115cfa55594..6cd36730371a 100644\n> > --- a/include/libcamera/internal/v4l2_subdevice.h\n> > +++ b/include/libcamera/internal/v4l2_subdevice.h\n> > @@ -80,6 +80,21 @@ public:\n> >                 ActiveFormat = V4L2_SUBDEV_FORMAT_ACTIVE,\n> >         };\n> >  \n> > +       struct Stream {\n> > +               Stream()\n> > +                       : pad(0), stream(0)\n> > +               {\n> > +               }\n> > +\n> > +               Stream(unsigned int p, unsigned int s)\n> > +                       : pad(p), stream(s)\n> > +               {\n> > +               }\n> > +\n> > +               unsigned int pad;\n> > +               unsigned int stream;\n> > +       };\n> > +\n> >         class Routing : public std::vector<struct v4l2_subdev_route>\n> >         {\n> >         public:\n> > @@ -93,17 +108,39 @@ public:\n> >  \n> >         const MediaEntity *entity() const { return entity_; }\n> >  \n> > -       int getSelection(unsigned int pad, unsigned int target,\n> > +       int getSelection(const Stream &stream, unsigned int target,\n> >                          Rectangle *rect);\n> > -       int setSelection(unsigned int pad, unsigned int target,\n> > +       int getSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > +       {\n> > +               return getSelection({ pad, 0 }, target, rect);\n> \n> Aha nice indeed.\n> \n> I wonder if createing a constructor for Stream that accepts an integer\n> pad, and defaults the stream to 0 would remove the need even for the\n> overloads, but I think \"automagcially converting int's to streams\" could\n> potentially cause confusion, so I don't mind having the explicit\n> overloads.\n\nYou would need to write\n\n\tsubdev->getSelection({ padNumber }, ...);\n\nin the caller, which I don't think is very nice. Even with an implicit\nconstructor, you won't be able to write\n\n\tsubdev->getSelection(padNumber, ...);\n\n> It would reduce the additional / repeated function documentation\n> additions though.\n> \n> > +       }\n> > +       int setSelection(const Stream &stream, unsigned int target,\n> >                          Rectangle *rect);\n> > +       int setSelection(unsigned int pad, unsigned int target, Rectangle *rect)\n> > +       {\n> > +               return setSelection({ pad, 0 }, target, rect);\n> > +       }\n> >  \n> > -       Formats formats(unsigned int pad);\n> > +       Formats formats(const Stream &stream);\n> > +       Formats formats(unsigned int pad)\n> > +       {\n> > +               return formats({ pad, 0 });\n> > +       }\n> >  \n> > +       int getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > +                     Whence whence = ActiveFormat);\n> >         int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +                     Whence whence = ActiveFormat)\n> > +       {\n> > +               return getFormat({ pad, 0 }, format, whence);\n> > +       }\n> > +       int setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> >                       Whence whence = ActiveFormat);\n> >         int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > -                     Whence whence = ActiveFormat);\n> > +                     Whence whence = ActiveFormat)\n> > +       {\n> > +               return setFormat({ pad, 0 }, format, whence);\n> > +       }\n> >  \n> >         int getRouting(Routing *routing, Whence whence = ActiveFormat);\n> >         int setRouting(Routing *routing, Whence whence = ActiveFormat);\n> > @@ -123,8 +160,8 @@ private:\n> >         std::optional<ColorSpace>\n> >         toColorSpace(const v4l2_mbus_framefmt &format) const;\n> >  \n> > -       std::vector<unsigned int> enumPadCodes(unsigned int pad);\n> > -       std::vector<SizeRange> enumPadSizes(unsigned int pad,\n> > +       std::vector<unsigned int> enumPadCodes(const Stream &stream);\n> > +       std::vector<SizeRange> enumPadSizes(const Stream &stream,\n> >                                             unsigned int code);\n> >  \n> >         const MediaEntity *entity_;\n> > @@ -133,4 +170,13 @@ private:\n> >         struct V4L2SubdeviceCapability caps_;\n> >  };\n> >  \n> > +bool operator==(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs);\n> > +static inline bool operator!=(const V4L2Subdevice::Stream &lhs,\n> > +                             const V4L2Subdevice::Stream &rhs)\n> > +{\n> > +       return !(lhs == rhs);\n> > +}\n> > +\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream);\n> > +\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 66da1edbf542..cc079425bb16 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -842,6 +842,62 @@ std::ostream &operator<<(std::ostream &out, const V4L2SubdeviceFormat &f)\n> >   * \\brief The format operation applies to TRY formats\n> >   */\n> >  \n> > +/**\n> > + * \\class V4L2Subdevice::Stream\n> > + * \\brief V4L2 subdevice stream\n> > + *\n> > + * This class identifies a subdev stream, by bundling the pad number with the\n> > + * stream number. It is used in all stream-aware functions of the V4L2Subdevice\n> > + * class to identify the stream the functions operate on.\n> > + *\n> > + * \\var V4L2Subdevice::Stream::pad\n> > + * \\brief The 0-indexed pad number\n> > + *\n> > + * \\var V4L2Subdevice::Stream::stream\n> > + * \\brief The stream number\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::Stream::Stream()\n> > + * \\brief Construct a Stream with pad and stream set to 0\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::Stream::Stream(unsigned int pad, unsigned int stream)\n> > + * \\brief Construct a Stream with a given \\a pad and \\a stream number\n> > + * \\param[in] pad The indexed pad number\n> > + * \\param[in] stream The stream number\n> > + */\n> > +\n> > +/**\n> > + * \\brief Compare streams for equality\n> > + * \\return True if the two streams are equal, false otherwise\n> > + */\n> > +bool operator==(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs)\n> > +{\n> > +       return lhs.pad == rhs.pad && lhs.stream == rhs.stream;\n> > +}\n> > +\n> > +/**\n> > + * \\fn bool operator!=(const V4L2Subdevice::Stream &lhs, const V4L2Subdevice::Stream &rhs)\n> > + * \\brief Compare streams for inequality\n> > + * \\return True if the two streams are not equal, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Insert a text representation of a V4L2Subdevice::Stream into an\n> > + * output stream\n> > + * \\param[in] out The output stream\n> > + * \\param[in] stream The V4L2Subdevice::Stream\n> > + * \\return The output stream \\a out\n> > + */\n> > +std::ostream &operator<<(std::ostream &out, const V4L2Subdevice::Stream &stream)\n> > +{\n> > +       out << stream.pad << \"/\" << stream.stream;\n> > +\n> > +       return out;\n> > +}\n> > +\n> >  /**\n> >   * \\class V4L2Subdevice::Routing\n> >   * \\brief V4L2 subdevice routing table\n> > @@ -933,7 +989,7 @@ int V4L2Subdevice::open()\n> >  \n> >  /**\n> >   * \\brief Get selection rectangle \\a rect for \\a target\n> > - * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > + * \\param[in] stream The stream the rectangle is retrieved from\n> >   * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> >   * \\param[out] rect The retrieved selection rectangle\n> >   *\n> > @@ -941,13 +997,14 @@ int V4L2Subdevice::open()\n> >   *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > -int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > +int V4L2Subdevice::getSelection(const Stream &stream, unsigned int target,\n> >                                 Rectangle *rect)\n> >  {\n> >         struct v4l2_subdev_selection sel = {};\n> >  \n> >         sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > -       sel.pad = pad;\n> > +       sel.pad = stream.pad;\n> > +       sel.stream = stream.stream;\n> >         sel.target = target;\n> >         sel.flags = 0;\n> >  \n> > @@ -955,7 +1012,7 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> >         if (ret < 0) {\n> >                 LOG(V4L2, Error)\n> >                         << \"Unable to get rectangle \" << target << \" on pad \"\n> > -                       << pad << \": \" << strerror(-ret);\n> > +                       << stream << \": \" << strerror(-ret);\n> >                 return ret;\n> >         }\n> >  \n> > @@ -967,9 +1024,20 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> >         return 0;\n> >  }\n> >  \n> > +/**\n> > + * \\fn V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > + * Rectangle *rect)\n> > + * \\brief Get selection rectangle \\a rect for \\a target\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is retrieved from\n> > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > + * \\param[out] rect The retrieved selection rectangle\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> >  /**\n> >   * \\brief Set selection rectangle \\a rect for \\a target\n> > - * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[in] stream The stream the rectangle is to be applied to\n> >   * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> >   * \\param[inout] rect The selection rectangle to be applied\n> >   *\n> > @@ -977,13 +1045,14 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> >   *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > -int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > +int V4L2Subdevice::setSelection(const Stream &stream, unsigned int target,\n> >                                 Rectangle *rect)\n> >  {\n> >         struct v4l2_subdev_selection sel = {};\n> >  \n> >         sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> > -       sel.pad = pad;\n> > +       sel.pad = stream.pad;\n> > +       sel.stream = stream.stream;\n> >         sel.target = target;\n> >         sel.flags = 0;\n> >  \n> > @@ -996,7 +1065,7 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >         if (ret < 0) {\n> >                 LOG(V4L2, Error)\n> >                         << \"Unable to set rectangle \" << target << \" on pad \"\n> > -                       << pad << \": \" << strerror(-ret);\n> > +                       << stream << \": \" << strerror(-ret);\n> >                 return ret;\n> >         }\n> >  \n> > @@ -1007,26 +1076,40 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> >  \n> >         return 0;\n> >  }\n> > +\n> >  /**\n> > - * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > - * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > + * \\fn V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,\n> > + * Rectangle *rect)\n> > + * \\brief Set selection rectangle \\a rect for \\a target\n> > + * \\param[in] pad The 0-indexed pad number the rectangle is to be applied to\n> > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > + * \\param[inout] rect The selection rectangle to be applied\n> > + *\n> > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Enumerate all media bus codes and frame sizes on a \\a stream\n> > + * \\param[in] stream The stream to enumerate formats for\n> >   *\n> >   * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > - * a \\a pad.\n> > + * a \\a stream.\n> >   *\n> >   * \\return A list of the supported device formats\n> >   */\n> > -V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> > +V4L2Subdevice::Formats V4L2Subdevice::formats(const Stream &stream)\n> >  {\n> >         Formats formats;\n> >  \n> > -       if (pad >= entity_->pads().size()) {\n> > -               LOG(V4L2, Error) << \"Invalid pad: \" << pad;\n> > +       if (stream.pad >= entity_->pads().size()) {\n> > +               LOG(V4L2, Error) << \"Invalid pad: \" << stream.pad;\n> >                 return {};\n> >         }\n> >  \n> > -       for (unsigned int code : enumPadCodes(pad)) {\n> > -               std::vector<SizeRange> sizes = enumPadSizes(pad, code);\n> > +       for (unsigned int code : enumPadCodes(stream)) {\n> > +               std::vector<SizeRange> sizes = enumPadSizes(stream, code);\n> >                 if (sizes.empty())\n> >                         return {};\n> >  \n> > @@ -1034,7 +1117,7 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> >                 if (!inserted.second) {\n> >                         LOG(V4L2, Error)\n> >                                 << \"Could not add sizes for media bus code \"\n> > -                               << code << \" on pad \" << pad;\n> > +                               << code << \" on pad \" << stream.pad;\n> >                         return {};\n> >                 }\n> >         }\n> > @@ -1042,6 +1125,17 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)\n> >         return formats;\n> >  }\n> >  \n> > +/**\n> > + * \\fn V4L2Subdevice::formats(unsigned int pad)\n> > + * \\brief Enumerate all media bus codes and frame sizes on a \\a pad\n> > + * \\param[in] pad The 0-indexed pad number to enumerate formats on\n> > + *\n> > + * Enumerate all media bus codes and frame sizes supported by the subdevice on\n> > + * a \\a pad\n> > + *\n> \n>  \\sa V4L2Subdevice::formats(Stream)\n> \n> or is that overkill? Probably.\n\nA little bit I think :-)\n\n> > + * \\return A list of the supported device formats\n> > + */\n> > +\n> >  std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const\n> >  {\n> >         /*\n> > @@ -1073,25 +1167,26 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &\n> >  }\n> >  \n> >  /**\n> > - * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > - * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > + * \\brief Retrieve the image format set on one of the V4L2 subdevice streams\n> > + * \\param[in] stream The stream the format is to be retrieved from\n> >   * \\param[out] format The image bus format\n> >   * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> >   * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > -int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > +int V4L2Subdevice::getFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> >                              Whence whence)\n> >  {\n> >         struct v4l2_subdev_format subdevFmt = {};\n> >         subdevFmt.which = whence;\n> > -       subdevFmt.pad = pad;\n> > +       subdevFmt.pad = stream.pad;\n> > +       subdevFmt.stream = stream.stream;\n> >  \n> >         int ret = ioctl(VIDIOC_SUBDEV_G_FMT, &subdevFmt);\n> >         if (ret) {\n> >                 LOG(V4L2, Error)\n> > -                       << \"Unable to get format on pad \" << pad\n> > -                       << \": \" << strerror(-ret);\n> > +                       << \"Unable to get format on pad \" << stream << \": \"\n> > +                       << strerror(-ret);\n> >                 return ret;\n> >         }\n> >  \n> > @@ -1104,6 +1199,66 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >  }\n> >  \n> >  /**\n> > + * \\fn V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > + * Whence whence)\n> > + * \\brief Retrieve the image format set on one of the V4L2 subdevice pads\n> > + * \\param[in] pad The 0-indexed pad number the format is to be retrieved from\n> > + * \\param[out] format The image bus format\n> > + * \\param[in] whence The format to get, \\ref V4L2Subdevice::ActiveFormat\n> > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\brief Set an image format on one of the V4L2 subdevice pads\n> > + * \\param[in] stream The stream the format is to be applied to\n> > + * \\param[inout] format The image bus format to apply to the stream\n> > + * \\param[in] whence The format to set, \\ref V4L2Subdevice::ActiveFormat\n> > + * \"ActiveFormat\" or \\ref V4L2Subdevice::TryFormat \"TryFormat\"\n> > + *\n> > + * Apply the requested image format to the desired stream and return the\n> > + * actually applied format parameters, as getFormat() would do.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2Subdevice::setFormat(const Stream &stream, V4L2SubdeviceFormat *format,\n> > +                            Whence whence)\n> > +{\n> > +       struct v4l2_subdev_format subdevFmt = {};\n> > +       subdevFmt.which = whence;\n> > +       subdevFmt.pad = stream.pad;\n> > +       subdevFmt.stream = stream.stream;\n> > +       subdevFmt.format.width = format->size.width;\n> > +       subdevFmt.format.height = format->size.height;\n> > +       subdevFmt.format.code = format->code;\n> > +       subdevFmt.format.field = V4L2_FIELD_NONE;\n> > +       if (format->colorSpace) {\n> > +               fromColorSpace(format->colorSpace, subdevFmt.format);\n> > +\n> > +               /* The CSC flag is only applicable to source pads. */\n> > +               if (entity_->pads()[stream.pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > +                       subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > +       }\n> > +\n> > +       int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > +       if (ret) {\n> > +               LOG(V4L2, Error)\n> > +                       << \"Unable to set format on pad \" << stream << \": \"\n> > +                       << strerror(-ret);\n> > +               return ret;\n> > +       }\n> > +\n> > +       format->size.width = subdevFmt.format.width;\n> > +       format->size.height = subdevFmt.format.height;\n> > +       format->code = subdevFmt.format.code;\n> > +       format->colorSpace = toColorSpace(subdevFmt.format);\n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > + * Whence whence)\n> >   * \\brief Set an image format on one of the V4L2 subdevice pads\n> >   * \\param[in] pad The 0-indexed pad number the format is to be applied to\n> >   * \\param[inout] format The image bus format to apply to the subdevice's pad\n> > @@ -1115,39 +1270,6 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >   *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > -int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > -                            Whence whence)\n> > -{\n> > -       struct v4l2_subdev_format subdevFmt = {};\n> > -       subdevFmt.which = whence;\n> > -       subdevFmt.pad = pad;\n> > -       subdevFmt.format.width = format->size.width;\n> > -       subdevFmt.format.height = format->size.height;\n> > -       subdevFmt.format.code = format->code;\n> > -       subdevFmt.format.field = V4L2_FIELD_NONE;\n> > -       if (format->colorSpace) {\n> > -               fromColorSpace(format->colorSpace, subdevFmt.format);\n> > -\n> > -               /* The CSC flag is only applicable to source pads. */\n> > -               if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> > -                       subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> > -       }\n> > -\n> > -       int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> > -       if (ret) {\n> > -               LOG(V4L2, Error)\n> > -                       << \"Unable to set format on pad \" << pad\n> > -                       << \": \" << strerror(-ret);\n> > -               return ret;\n> > -       }\n> > -\n> > -       format->size.width = subdevFmt.format.width;\n> > -       format->size.height = subdevFmt.format.height;\n> > -       format->code = subdevFmt.format.code;\n> > -       format->colorSpace = toColorSpace(subdevFmt.format);\n> > -\n> > -       return 0;\n> > -}\n> >  \n> >  /**\n> >   * \\brief Retrieve the subdevice's internal routing table\n> > @@ -1310,14 +1432,15 @@ std::string V4L2Subdevice::logPrefix() const\n> >         return \"'\" + entity_->name() + \"'\";\n> >  }\n> >  \n> > -std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n> > +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(const Stream &stream)\n> >  {\n> >         std::vector<unsigned int> codes;\n> >         int ret;\n> >  \n> >         for (unsigned int index = 0; ; index++) {\n> >                 struct v4l2_subdev_mbus_code_enum mbusEnum = {};\n> > -               mbusEnum.pad = pad;\n> > +               mbusEnum.pad = stream.pad;\n> > +               mbusEnum.stream = stream.stream;\n> >                 mbusEnum.index = index;\n> >                 mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >  \n> > @@ -1330,7 +1453,7 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n> >  \n> >         if (ret < 0 && ret != -EINVAL) {\n> >                 LOG(V4L2, Error)\n> > -                       << \"Unable to enumerate formats on pad \" << pad\n> > +                       << \"Unable to enumerate formats on pad \" << stream\n> >                         << \": \" << strerror(-ret);\n> >                 return {};\n> >         }\n> > @@ -1338,7 +1461,7 @@ std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)\n> >         return codes;\n> >  }\n> >  \n> > -std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n> > +std::vector<SizeRange> V4L2Subdevice::enumPadSizes(const Stream &stream,\n> >                                                    unsigned int code)\n> >  {\n> >         std::vector<SizeRange> sizes;\n> > @@ -1347,7 +1470,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n> >         for (unsigned int index = 0;; index++) {\n> >                 struct v4l2_subdev_frame_size_enum sizeEnum = {};\n> >                 sizeEnum.index = index;\n> > -               sizeEnum.pad = pad;\n> > +               sizeEnum.pad = stream.pad;\n> > +               sizeEnum.stream = stream.stream;\n> >                 sizeEnum.code = code;\n> >                 sizeEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;\n> >  \n> > @@ -1361,7 +1485,7 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,\n> >  \n> >         if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {\n> >                 LOG(V4L2, Error)\n> > -                       << \"Unable to enumerate sizes on pad \" << pad\n> > +                       << \"Unable to enumerate sizes on pad \" << stream\n> \n> Should this now say \"on stream\"? Or do we really want to still say \"on\n> pad\" here?\n\nI want back and forth between the two. Given that most callers won't\ncare about streams, and given that V4L2 tools such as media-ctl still\nsay pad but print x/y, I've kept the word pad here.\n\n> Fine either way though and everything above is all optional too so:\n> \n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> >                         << \": \" << strerror(-ret);\n> >                 return {};\n> >         }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 68D1EBD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Mar 2024 10:44:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C4FE62C90;\n\tFri, 15 Mar 2024 11:44:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CE8F162C80\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Mar 2024 11:44:51 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AACACB3;\n\tFri, 15 Mar 2024 11:44:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sVe1VzDV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710499467;\n\tbh=SjKtfNH81A5G4XGOm9bQwATsg2+bxJT5eNVToBACTnU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sVe1VzDVRWXC2i5TAqwhmdiSZbLEYylI5RqU7etm3mJH0vqKODQNNpHUg0/QRVtjb\n\tShf0hO43CDRLobx07ALcYyQ7CsTWhiLzOZsZphNdV8XEsToSJNheWBo6R2NMqA7HKk\n\tIcoigSCp2Z/FZSYuulYLh2QqzmCByRFoXIAwtChY=","Date":"Fri, 15 Mar 2024 12:44:49 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v2 07/14] libcamera: v4l2_subdevice: Add stream support\n\tto get/set functions","Message-ID":"<20240315104449.GF2441@pendragon.ideasonboard.com>","References":"<20240315001613.2033-1-laurent.pinchart@ideasonboard.com>\n\t<20240315001613.2033-8-laurent.pinchart@ideasonboard.com>\n\t<171049919958.1011926.5152771841307159861@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171049919958.1011926.5152771841307159861@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]