| Message ID | 20211118151933.15627-4-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, The patch looks good to me overall, just one comment On 11/18/21 8:49 PM, David Plowman wrote: > These methods are added to the base V4L2Device class so that they can > be shared both by the video device class and subdevices. > > With the ColorSpace class, the color space and related other fields > are stored together, corresponding to a number of fields in the > various different V4L2 format structures. Template methods are > therefore a convenient implementation, and we must explicitly > instantiate the templates that will be needed. > > Note that unset color spaces are converted to requests for the > device's "default" color space. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/v4l2_device.h | 7 + > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > 2 files changed, 197 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index f21bc370..89ff6120 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -17,6 +17,7 @@ > #include <libcamera/base/signal.h> > #include <libcamera/base/span.h> > > +#include <libcamera/color_space.h> > #include <libcamera/controls.h> > > namespace libcamera { > @@ -44,6 +45,12 @@ public: > > void updateControlInfo(); > > + template<typename T> > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > + > + template<typename T> > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > + > protected: > V4L2Device(const std::string &deviceNode); > ~V4L2Device(); > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 9c783c9c..4d6985aa 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -16,6 +16,8 @@ > #include <sys/syscall.h> > #include <unistd.h> > > +#include <linux/v4l2-mediabus.h> > + > #include <libcamera/base/event_notifier.h> > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > frameStart.emit(event.u.frame_sync.frame_sequence); > } > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > +}; > + > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > +}; > + > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > +}; > + > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > +}; > + > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > +}; > + > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > +}; > + > +/** > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > + * \param[in] v4l2Format A V4L2 format containing color space information > + * > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > + * V4L2 format structure are converted to a corresponding ColorSpace. > + * > + * If any V4L2 fields are not recognised then we return an "unset" > + * color space. > + * > + * \return The ColorSpace corresponding to the input V4L2 format > + */ > +template<typename T> > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > +{ > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > + if (itColor == v4l2ToColorSpace.end()) > + return std::nullopt; > + > + /* This sets all the color space fields to the correct "default" values. */ > + ColorSpace colorSpace = itColor->second; > + > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > + return std::nullopt; > + > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > + } > + > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > + if (itTransfer == v4l2ToTransferFunction.end()) > + return std::nullopt; > + > + colorSpace.transferFunction = itTransfer->second; > + } > + > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > + if (itRange == v4l2ToRange.end()) > + return std::nullopt; > + > + colorSpace.range = itRange->second; > + } > + > + return colorSpace; > +} > + > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > + > +/** > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > + * \param[in] colorSpace The ColorSpace to be converted > + * \param[out] v4l2Format A V4L2 format containing color space information > + * > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > + * V4L2 format structure are filled in from a corresponding ColorSpace. > + * > + * An error is returned if any of the V4L2 fields do not support the > + * value given in the ColorSpace. Such fields are set to the V4L2 > + * "default" values, but all other fields are still filled in where > + * possible. > + * > + * If the color space is completely unset, "default" V4L2 values are used > + * everywhere, so a driver would then choose its preferred color space. > + * > + * \return 0 on success or a negative error code otherwise I think we are not returning any error codes (ergo errno here) here. All I see -1 returned on error paths. > + */ > +template<typename T> > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > +{ > + int ret = 0; > + > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > + > + if (!colorSpace) > + return ret; > + > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > + [&colorSpace](const auto &item) { > + return colorSpace == item.first; > + }); > + if (itColor != colorSpaceToV4l2.end()) { > + v4l2Format.colorspace = itColor->second; > + /* Leaving all the other fields as "default" should be fine. */ > + return ret; > + } > + > + /* > + * If the colorSpace doesn't precisely match a standard color space, > + * then we must choose a V4L2 colorspace with matching primaries. > + */ > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > + if (itPrimaries != primariesToV4l2.end()) > + v4l2Format.colorspace = itPrimaries->second; > + else > + ret = -1; > + > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > + else > + ret = -1; > + > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > + if (itTransfer != transferFunctionToV4l2.end()) > + v4l2Format.xfer_func = itTransfer->second; > + else > + ret = -1; > + > + auto itRange = rangeToV4l2.find(colorSpace->range); > + if (itRange != rangeToV4l2.end()) > + v4l2Format.quantization = itRange->second; > + else > + ret = -1; > + > + return ret; > +} > + > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > + > } /* namespace libcamera */
Hi Umang Thanks for reviewing this! On Tue, 23 Nov 2021 at 14:49, Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi David, > > The patch looks good to me overall, just one comment > > On 11/18/21 8:49 PM, David Plowman wrote: > > These methods are added to the base V4L2Device class so that they can > > be shared both by the video device class and subdevices. > > > > With the ColorSpace class, the color space and related other fields > > are stored together, corresponding to a number of fields in the > > various different V4L2 format structures. Template methods are > > therefore a convenient implementation, and we must explicitly > > instantiate the templates that will be needed. > > > > Note that unset color spaces are converted to requests for the > > device's "default" color space. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/v4l2_device.h | 7 + > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > 2 files changed, 197 insertions(+) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index f21bc370..89ff6120 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -17,6 +17,7 @@ > > #include <libcamera/base/signal.h> > > #include <libcamera/base/span.h> > > > > +#include <libcamera/color_space.h> > > #include <libcamera/controls.h> > > > > namespace libcamera { > > @@ -44,6 +45,12 @@ public: > > > > void updateControlInfo(); > > > > + template<typename T> > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > + > > + template<typename T> > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > + > > protected: > > V4L2Device(const std::string &deviceNode); > > ~V4L2Device(); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 9c783c9c..4d6985aa 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -16,6 +16,8 @@ > > #include <sys/syscall.h> > > #include <unistd.h> > > > > +#include <linux/v4l2-mediabus.h> > > + > > #include <libcamera/base/event_notifier.h> > > #include <libcamera/base/log.h> > > #include <libcamera/base/utils.h> > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > frameStart.emit(event.u.frame_sync.frame_sequence); > > } > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > +}; > > + > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > +}; > > + > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > +}; > > + > > +/** > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > + * \param[in] v4l2Format A V4L2 format containing color space information > > + * > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > + * > > + * If any V4L2 fields are not recognised then we return an "unset" > > + * color space. > > + * > > + * \return The ColorSpace corresponding to the input V4L2 format > > + */ > > +template<typename T> > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > +{ > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > + if (itColor == v4l2ToColorSpace.end()) > > + return std::nullopt; > > + > > + /* This sets all the color space fields to the correct "default" values. */ > > + ColorSpace colorSpace = itColor->second; > > + > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > + return std::nullopt; > > + > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > + } > > + > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > + if (itTransfer == v4l2ToTransferFunction.end()) > > + return std::nullopt; > > + > > + colorSpace.transferFunction = itTransfer->second; > > + } > > + > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > + if (itRange == v4l2ToRange.end()) > > + return std::nullopt; > > + > > + colorSpace.range = itRange->second; > > + } > > + > > + return colorSpace; > > +} > > + > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > + > > +/** > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > + * \param[in] colorSpace The ColorSpace to be converted > > + * \param[out] v4l2Format A V4L2 format containing color space information > > + * > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > + * > > + * An error is returned if any of the V4L2 fields do not support the > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > + * "default" values, but all other fields are still filled in where > > + * possible. > > + * > > + * If the color space is completely unset, "default" V4L2 values are used > > + * everywhere, so a driver would then choose its preferred color space. > > + * > > + * \return 0 on success or a negative error code otherwise > > I think we are not returning any error codes (ergo errno here) here. > All I see -1 returned on error paths. Just to clarify, are you simply implying a change to the documentation here, i.e. "\return 0 on success or -1 otherwise" ? Thanks! David > > > > + */ > > +template<typename T> > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > +{ > > + int ret = 0; > > + > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > + > > + if (!colorSpace) > > + return ret; > > + > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > + [&colorSpace](const auto &item) { > > + return colorSpace == item.first; > > + }); > > + if (itColor != colorSpaceToV4l2.end()) { > > + v4l2Format.colorspace = itColor->second; > > + /* Leaving all the other fields as "default" should be fine. */ > > + return ret; > > + } > > + > > + /* > > + * If the colorSpace doesn't precisely match a standard color space, > > + * then we must choose a V4L2 colorspace with matching primaries. > > + */ > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > + if (itPrimaries != primariesToV4l2.end()) > > + v4l2Format.colorspace = itPrimaries->second; > > + else > > + ret = -1; > > + > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > + else > > + ret = -1; > > + > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > + if (itTransfer != transferFunctionToV4l2.end()) > > + v4l2Format.xfer_func = itTransfer->second; > > + else > > + ret = -1; > > + > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > + if (itRange != rangeToV4l2.end()) > > + v4l2Format.quantization = itRange->second; > > + else > > + ret = -1; > > + > > + return ret; > > +} > > + > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > + > > } /* namespace libcamera */
Quoting David Plowman (2021-11-24 11:08:23) > Hi Umang > > Thanks for reviewing this! > > On Tue, 23 Nov 2021 at 14:49, Umang Jain <umang.jain@ideasonboard.com> wrote: > > > > Hi David, > > > > The patch looks good to me overall, just one comment > > > > On 11/18/21 8:49 PM, David Plowman wrote: > > > These methods are added to the base V4L2Device class so that they can > > > be shared both by the video device class and subdevices. > > > > > > With the ColorSpace class, the color space and related other fields > > > are stored together, corresponding to a number of fields in the > > > various different V4L2 format structures. Template methods are > > > therefore a convenient implementation, and we must explicitly > > > instantiate the templates that will be needed. > > > > > > Note that unset color spaces are converted to requests for the > > > device's "default" color space. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/v4l2_device.h | 7 + > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > > 2 files changed, 197 insertions(+) > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > index f21bc370..89ff6120 100644 > > > --- a/include/libcamera/internal/v4l2_device.h > > > +++ b/include/libcamera/internal/v4l2_device.h > > > @@ -17,6 +17,7 @@ > > > #include <libcamera/base/signal.h> > > > #include <libcamera/base/span.h> > > > > > > +#include <libcamera/color_space.h> > > > #include <libcamera/controls.h> > > > > > > namespace libcamera { > > > @@ -44,6 +45,12 @@ public: > > > > > > void updateControlInfo(); > > > > > > + template<typename T> > > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > > + > > > + template<typename T> > > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > > + > > > protected: > > > V4L2Device(const std::string &deviceNode); > > > ~V4L2Device(); > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 9c783c9c..4d6985aa 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -16,6 +16,8 @@ > > > #include <sys/syscall.h> > > > #include <unistd.h> > > > > > > +#include <linux/v4l2-mediabus.h> > > > + > > > #include <libcamera/base/event_notifier.h> > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/utils.h> > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > > frameStart.emit(event.u.frame_sync.frame_sequence); > > > } > > > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > > +}; > > > + > > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > > +}; > > > + > > > +/** > > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > > + * \param[in] v4l2Format A V4L2 format containing color space information > > > + * > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > > + * > > > + * If any V4L2 fields are not recognised then we return an "unset" > > > + * color space. > > > + * > > > + * \return The ColorSpace corresponding to the input V4L2 format > > > + */ > > > +template<typename T> > > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > > +{ > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > > + if (itColor == v4l2ToColorSpace.end()) > > > + return std::nullopt; > > > + > > > + /* This sets all the color space fields to the correct "default" values. */ > > > + ColorSpace colorSpace = itColor->second; > > > + > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > > + return std::nullopt; > > > + > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > > + } > > > + > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > > + if (itTransfer == v4l2ToTransferFunction.end()) > > > + return std::nullopt; > > > + > > > + colorSpace.transferFunction = itTransfer->second; > > > + } > > > + > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > > + if (itRange == v4l2ToRange.end()) > > > + return std::nullopt; > > > + > > > + colorSpace.range = itRange->second; > > > + } > > > + > > > + return colorSpace; > > > +} > > > + > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > > + > > > +/** > > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > > + * \param[in] colorSpace The ColorSpace to be converted > > > + * \param[out] v4l2Format A V4L2 format containing color space information > > > + * > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > > + * > > > + * An error is returned if any of the V4L2 fields do not support the > > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > > + * "default" values, but all other fields are still filled in where > > > + * possible. > > > + * > > > + * If the color space is completely unset, "default" V4L2 values are used > > > + * everywhere, so a driver would then choose its preferred color space. > > > + * > > > + * \return 0 on success or a negative error code otherwise > > > > I think we are not returning any error codes (ergo errno here) here. > > All I see -1 returned on error paths. > > Just to clarify, are you simply implying a change to the documentation > here, i.e. "\return 0 on success or -1 otherwise" ? Generally thoughout libcamera we use errno values to return errors. Can we change the -1's to something more explicit like -EINVAL? or -ENOENT? or something that conveys what happened in some form? We often try to use strerror() to print 'more helpful' error reports. In this instance, it means we tried to construct a V4L2 colourspace from the defined types but couldn't make an exact match ... so I think -EINVAL works? At which point, the documentation could say: \return 0 on success or a negative error code otherwise \return -EINVAL A valid colour space could not be constructed Or something along those lines. -- Kieran > > Thanks! > David > > > > > > > > + */ > > > +template<typename T> > > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > > +{ > > > + int ret = 0; > > > + > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > > + > > > + if (!colorSpace) > > > + return ret; > > > + > > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > > + [&colorSpace](const auto &item) { > > > + return colorSpace == item.first; > > > + }); > > > + if (itColor != colorSpaceToV4l2.end()) { > > > + v4l2Format.colorspace = itColor->second; > > > + /* Leaving all the other fields as "default" should be fine. */ > > > + return ret; > > > + } > > > + > > > + /* > > > + * If the colorSpace doesn't precisely match a standard color space, > > > + * then we must choose a V4L2 colorspace with matching primaries. > > > + */ > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > > + if (itPrimaries != primariesToV4l2.end()) > > > + v4l2Format.colorspace = itPrimaries->second; > > > + else > > > + ret = -1; > > > + > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > > + else > > > + ret = -1; > > > + > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > > + if (itTransfer != transferFunctionToV4l2.end()) > > > + v4l2Format.xfer_func = itTransfer->second; > > > + else > > > + ret = -1; > > > + > > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > > + if (itRange != rangeToV4l2.end()) > > > + v4l2Format.quantization = itRange->second; > > > + else > > > + ret = -1; > > > + > > > + return ret; > > > +} > > > + > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > > + > > > } /* namespace libcamera */
Hi David On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote: > These methods are added to the base V4L2Device class so that they can > be shared both by the video device class and subdevices. > > With the ColorSpace class, the color space and related other fields > are stored together, corresponding to a number of fields in the > various different V4L2 format structures. Template methods are > therefore a convenient implementation, and we must explicitly > instantiate the templates that will be needed. > > Note that unset color spaces are converted to requests for the > device's "default" color space. You seem to use "unset" in place of default in this version. Also, this is a nice explanation about the implementation but what something like the following as first paragraph ? Add functions to the V4L2Device class to convert to and from libcamera ColorSpace. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/v4l2_device.h | 7 + > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > 2 files changed, 197 insertions(+) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index f21bc370..89ff6120 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -17,6 +17,7 @@ > #include <libcamera/base/signal.h> > #include <libcamera/base/span.h> > > +#include <libcamera/color_space.h> > #include <libcamera/controls.h> > > namespace libcamera { > @@ -44,6 +45,12 @@ public: > > void updateControlInfo(); > > + template<typename T> > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > + > + template<typename T> > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > + Can't we have this protected and the template specializations as public in the appropriate classes ? Not sure if it's possible, but I guess it's desirable ? > protected: > V4L2Device(const std::string &deviceNode); > ~V4L2Device(); > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 9c783c9c..4d6985aa 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -16,6 +16,8 @@ > #include <sys/syscall.h> > #include <unistd.h> > > +#include <linux/v4l2-mediabus.h> > + Is this just for the template specialization ? > #include <libcamera/base/event_notifier.h> > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > frameStart.emit(event.u.frame_sync.frame_sequence); > } > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > +}; > + > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > +}; > + > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > +}; > + > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > +}; > + > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > +}; > + > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > +}; > + > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > +}; > + > +/** > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > + * \param[in] v4l2Format A V4L2 format containing color space information > + * > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > + * V4L2 format structure are converted to a corresponding ColorSpace. > + * > + * If any V4L2 fields are not recognised then we return an "unset" > + * color space. > + * > + * \return The ColorSpace corresponding to the input V4L2 format You can return nullptr, this should be documented > + */ > +template<typename T> > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > +{ > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > + if (itColor == v4l2ToColorSpace.end()) > + return std::nullopt; > + > + /* This sets all the color space fields to the correct "default" values. */ > + ColorSpace colorSpace = itColor->second; > + > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > + return std::nullopt; > + > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > + } > + > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > + if (itTransfer == v4l2ToTransferFunction.end()) > + return std::nullopt; > + > + colorSpace.transferFunction = itTransfer->second; > + } > + > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > + if (itRange == v4l2ToRange.end()) > + return std::nullopt; Should all of these cases be associated with an error message, asking to update the ColorSpace class to add the new encoding/xfer_func/quantization methods not yet supported ? > + > + colorSpace.range = itRange->second; > + } > + > + return colorSpace; > +} > + > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > + > +/** > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > + * \param[in] colorSpace The ColorSpace to be converted > + * \param[out] v4l2Format A V4L2 format containing color space information I'm a bit uncertain about the two functions having different prototypes... > + * > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > + * V4L2 format structure are filled in from a corresponding ColorSpace. > + * > + * An error is returned if any of the V4L2 fields do not support the > + * value given in the ColorSpace. Such fields are set to the V4L2 > + * "default" values, but all other fields are still filled in where > + * possible. > + * > + * If the color space is completely unset, "default" V4L2 values are used > + * everywhere, so a driver would then choose its preferred color space. We're translating to libcamera::ColorSpace to V4L2 fields. The returned color space is adjusted if nothing in V4L2 matches a libcamera defined colorspace/ecoding/quantization/xfer_func. Do you think this could ever happen ? > + * > + * \return 0 on success or a negative error code otherwise > + */ > +template<typename T> > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > +{ > + int ret = 0; > + > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > + > + if (!colorSpace) > + return ret; return 0 and declare ret later ? > + > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > + [&colorSpace](const auto &item) { > + return colorSpace == item.first; > + }); > + if (itColor != colorSpaceToV4l2.end()) { > + v4l2Format.colorspace = itColor->second; > + /* Leaving all the other fields as "default" should be fine. */ > + return ret; > + } I again wonder when this could happen (no matching V4L2 colorspace). I understand if we were matching on the device supported colorospace, but we're comparing against the ones defined by v4l2 in general. Thanks j > + > + /* > + * If the colorSpace doesn't precisely match a standard color space, > + * then we must choose a V4L2 colorspace with matching primaries. > + */ > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > + if (itPrimaries != primariesToV4l2.end()) > + v4l2Format.colorspace = itPrimaries->second; > + else > + ret = -1; > + > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > + else > + ret = -1; > + > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > + if (itTransfer != transferFunctionToV4l2.end()) > + v4l2Format.xfer_func = itTransfer->second; > + else > + ret = -1; > + > + auto itRange = rangeToV4l2.find(colorSpace->range); > + if (itRange != rangeToV4l2.end()) > + v4l2Format.quantization = itRange->second; > + else > + ret = -1; > + > + return ret; > +} > + > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > + > } /* namespace libcamera */ > -- > 2.20.1 >
Hi Jacopo Thanks for reviewing this! On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote: > > These methods are added to the base V4L2Device class so that they can > > be shared both by the video device class and subdevices. > > > > With the ColorSpace class, the color space and related other fields > > are stored together, corresponding to a number of fields in the > > various different V4L2 format structures. Template methods are > > therefore a convenient implementation, and we must explicitly > > instantiate the templates that will be needed. > > > > Note that unset color spaces are converted to requests for the > > device's "default" color space. > > You seem to use "unset" in place of default in this version. Yes, as a result of our discussion I went back to using std::optional so there is a genuine notion of it being "unset". Of course, there's then the question of what that means and it's true that when you request a ColorSpace that is "unset", our V4L2 layer will ask the drivers for their "default" colour space. So it's the same. Does that make sense? > > Also, this is a nice explanation about the implementation but > what something like the following as first paragraph ? > > Add functions to the V4L2Device class to convert to and from > libcamera ColorSpace. Yes, good idea. > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/v4l2_device.h | 7 + > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > 2 files changed, 197 insertions(+) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index f21bc370..89ff6120 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -17,6 +17,7 @@ > > #include <libcamera/base/signal.h> > > #include <libcamera/base/span.h> > > > > +#include <libcamera/color_space.h> > > #include <libcamera/controls.h> > > > > namespace libcamera { > > @@ -44,6 +45,12 @@ public: > > > > void updateControlInfo(); > > > > + template<typename T> > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > + > > + template<typename T> > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > + > > Can't we have this protected and the template specializations as > public in the appropriate classes ? Not sure if it's possible, but I > guess it's desirable ? I'll see if I can make those protected. > > > protected: > > V4L2Device(const std::string &deviceNode); > > ~V4L2Device(); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 9c783c9c..4d6985aa 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -16,6 +16,8 @@ > > #include <sys/syscall.h> > > #include <unistd.h> > > > > +#include <linux/v4l2-mediabus.h> > > + > > Is this just for the template specialization ? Not sure that I remember any more! But I'll double-check if I really need it. > > > #include <libcamera/base/event_notifier.h> > > #include <libcamera/base/log.h> > > #include <libcamera/base/utils.h> > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > frameStart.emit(event.u.frame_sync.frame_sequence); > > } > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > +}; > > + > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > +}; > > + > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > +}; > > + > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > +}; > > + > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > +}; > > + > > +/** > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > + * \param[in] v4l2Format A V4L2 format containing color space information > > + * > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > + * > > + * If any V4L2 fields are not recognised then we return an "unset" > > + * color space. > > + * > > + * \return The ColorSpace corresponding to the input V4L2 format > > You can return nullptr, this should be documented It can return a nullopt. If it couldn't return a nullopt I wouldn't have returned a std::optional at all, so I'm thinking this is OK? > > > + */ > > +template<typename T> > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > +{ > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > + if (itColor == v4l2ToColorSpace.end()) > > + return std::nullopt; > > + > > + /* This sets all the color space fields to the correct "default" values. */ > > + ColorSpace colorSpace = itColor->second; > > + > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > + return std::nullopt; > > + > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > + } > > + > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > + if (itTransfer == v4l2ToTransferFunction.end()) > > + return std::nullopt; > > + > > + colorSpace.transferFunction = itTransfer->second; > > + } > > + > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > + if (itRange == v4l2ToRange.end()) > > + return std::nullopt; > > Should all of these cases be associated with an error message, asking > to update the ColorSpace class to add the new > encoding/xfer_func/quantization methods not yet supported ? Currently I'm signalling errors where these values get used. I'm always a bit nervous about outputting error messages at the lowest level unless you're really sure, on the grounds that they can be hard to turn off! But I don't feel very strongly... > > + > > + colorSpace.range = itRange->second; > > + } > > + > > + return colorSpace; > > +} > > + > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > + > > +/** > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > + * \param[in] colorSpace The ColorSpace to be converted > > + * \param[out] v4l2Format A V4L2 format containing color space information > > I'm a bit uncertain about the two functions having different > prototypes... Yes, the catch with filling in V4L2 fields is that there's no way to say "I couldn't do it", so there's an extra return code to signal that. > > > + * > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > + * > > + * An error is returned if any of the V4L2 fields do not support the > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > + * "default" values, but all other fields are still filled in where > > + * possible. > > + * > > + * If the color space is completely unset, "default" V4L2 values are used > > + * everywhere, so a driver would then choose its preferred color space. > > We're translating to libcamera::ColorSpace to V4L2 fields. > > The returned color space is adjusted if nothing in V4L2 matches a > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you > think this could ever happen ? Not for the time being, though possibly further in the future. I think it's probably fair enough if you can't get the colour space that you want? > > > + * > > + * \return 0 on success or a negative error code otherwise > > + */ > > +template<typename T> > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > +{ > > + int ret = 0; > > + > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > + > > + if (!colorSpace) > > + return ret; > > > return 0 and declare ret later ? > > > + > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > + [&colorSpace](const auto &item) { > > + return colorSpace == item.first; > > + }); > > + if (itColor != colorSpaceToV4l2.end()) { > > + v4l2Format.colorspace = itColor->second; > > + /* Leaving all the other fields as "default" should be fine. */ > > + return ret; > > + } > > I again wonder when this could happen (no matching V4L2 colorspace). > I understand if we were matching on the device supported colorospace, > but we're comparing against the ones defined by v4l2 in general. As above, at the moment this can't happen, though maybe one day... Returning an error code seems reasonable to me in this case so that a problem gets flagged up. It's unlikely that you're going to want to "fix" V4L2, or a driver, to give you the colour space you want, but you should still know that there are some problems that may need attention. Thanks! David > > Thanks > j > > > + > > + /* > > + * If the colorSpace doesn't precisely match a standard color space, > > + * then we must choose a V4L2 colorspace with matching primaries. > > + */ > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > + if (itPrimaries != primariesToV4l2.end()) > > + v4l2Format.colorspace = itPrimaries->second; > > + else > > + ret = -1; > > + > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > + else > > + ret = -1; > > + > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > + if (itTransfer != transferFunctionToV4l2.end()) > > + v4l2Format.xfer_func = itTransfer->second; > > + else > > + ret = -1; > > + > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > + if (itRange != rangeToV4l2.end()) > > + v4l2Format.quantization = itRange->second; > > + else > > + ret = -1; > > + > > + return ret; > > +} > > + > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > + > > } /* namespace libcamera */ > > -- > > 2.20.1 > >
Quoting David Plowman (2021-11-25 11:06:08) > Hi Jacopo > > Thanks for reviewing this! > > On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David > > > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote: > > > These methods are added to the base V4L2Device class so that they can > > > be shared both by the video device class and subdevices. > > > > > > With the ColorSpace class, the color space and related other fields > > > are stored together, corresponding to a number of fields in the > > > various different V4L2 format structures. Template methods are > > > therefore a convenient implementation, and we must explicitly > > > instantiate the templates that will be needed. > > > > > > Note that unset color spaces are converted to requests for the > > > device's "default" color space. > > > > You seem to use "unset" in place of default in this version. > > Yes, as a result of our discussion I went back to using std::optional > so there is a genuine notion of it being "unset". Of course, there's > then the question of what that means and it's true that when you > request a ColorSpace that is "unset", our V4L2 layer will ask the > drivers for their "default" colour space. So it's the same. Does that > make sense? > > > > > Also, this is a nice explanation about the implementation but > > what something like the following as first paragraph ? > > > > Add functions to the V4L2Device class to convert to and from > > libcamera ColorSpace. > > Yes, good idea. > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/v4l2_device.h | 7 + > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > > 2 files changed, 197 insertions(+) > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > index f21bc370..89ff6120 100644 > > > --- a/include/libcamera/internal/v4l2_device.h > > > +++ b/include/libcamera/internal/v4l2_device.h > > > @@ -17,6 +17,7 @@ > > > #include <libcamera/base/signal.h> > > > #include <libcamera/base/span.h> > > > > > > +#include <libcamera/color_space.h> > > > #include <libcamera/controls.h> > > > > > > namespace libcamera { > > > @@ -44,6 +45,12 @@ public: > > > > > > void updateControlInfo(); > > > > > > + template<typename T> > > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > > + > > > + template<typename T> > > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > > + > > > > Can't we have this protected and the template specializations as > > public in the appropriate classes ? Not sure if it's possible, but I > > guess it's desirable ? > > I'll see if I can make those protected. > > > > > > protected: > > > V4L2Device(const std::string &deviceNode); > > > ~V4L2Device(); > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 9c783c9c..4d6985aa 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -16,6 +16,8 @@ > > > #include <sys/syscall.h> > > > #include <unistd.h> > > > > > > +#include <linux/v4l2-mediabus.h> > > > + > > > > Is this just for the template specialization ? > > Not sure that I remember any more! But I'll double-check if I really need it. > > > > > > #include <libcamera/base/event_notifier.h> > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/utils.h> > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > > frameStart.emit(event.u.frame_sync.frame_sequence); > > > } > > > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > > +}; > > > + > > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > > +}; > > > + > > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > > +}; > > > + > > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > > +}; > > > + > > > +/** > > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > > + * \param[in] v4l2Format A V4L2 format containing color space information > > > + * > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > > + * > > > + * If any V4L2 fields are not recognised then we return an "unset" > > > + * color space. > > > + * > > > + * \return The ColorSpace corresponding to the input V4L2 format > > > > You can return nullptr, this should be documented > > It can return a nullopt. If it couldn't return a nullopt I wouldn't > have returned a std::optional at all, so I'm thinking this is OK? I think something like this should be added (after the existing return) \retval std::nullopt One or more V4L2 color space fields were not recognised It duplicates what you've written above, but it directly documents the return values in the Doxygen output. > > > > > + */ > > > +template<typename T> > > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > > +{ > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > > + if (itColor == v4l2ToColorSpace.end()) > > > + return std::nullopt; > > > + > > > + /* This sets all the color space fields to the correct "default" values. */ > > > + ColorSpace colorSpace = itColor->second; > > > + > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > > + return std::nullopt; > > > + > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > > + } > > > + > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > > + if (itTransfer == v4l2ToTransferFunction.end()) > > > + return std::nullopt; > > > + > > > + colorSpace.transferFunction = itTransfer->second; > > > + } > > > + > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > > + if (itRange == v4l2ToRange.end()) > > > + return std::nullopt; > > > > Should all of these cases be associated with an error message, asking > > to update the ColorSpace class to add the new > > encoding/xfer_func/quantization methods not yet supported ? > > Currently I'm signalling errors where these values get used. I'm > always a bit nervous about outputting error messages at the lowest > level unless you're really sure, on the grounds that they can be hard > to turn off! But I don't feel very strongly... > > > > + > > > + colorSpace.range = itRange->second; > > > + } > > > + > > > + return colorSpace; > > > +} > > > + > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > > + > > > +/** > > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > > + * \param[in] colorSpace The ColorSpace to be converted > > > + * \param[out] v4l2Format A V4L2 format containing color space information > > > > I'm a bit uncertain about the two functions having different > > prototypes... > > Yes, the catch with filling in V4L2 fields is that there's no way to > say "I couldn't do it", so there's an extra return code to signal > that. > > > > > > + * > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > > + * > > > + * An error is returned if any of the V4L2 fields do not support the > > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > > + * "default" values, but all other fields are still filled in where > > > + * possible. > > > + * > > > + * If the color space is completely unset, "default" V4L2 values are used > > > + * everywhere, so a driver would then choose its preferred color space. > > > > We're translating to libcamera::ColorSpace to V4L2 fields. > > > > The returned color space is adjusted if nothing in V4L2 matches a > > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you > > think this could ever happen ? > > Not for the time being, though possibly further in the future. I think > it's probably fair enough if you can't get the colour space that you > want? > > > > > > + * > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > +template<typename T> > > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > > +{ > > > + int ret = 0; > > > + > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > > + > > > + if (!colorSpace) > > > + return ret; > > > > > > return 0 and declare ret later ? > > > > > + > > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > > + [&colorSpace](const auto &item) { > > > + return colorSpace == item.first; > > > + }); > > > + if (itColor != colorSpaceToV4l2.end()) { > > > + v4l2Format.colorspace = itColor->second; > > > + /* Leaving all the other fields as "default" should be fine. */ > > > + return ret; > > > + } > > > > I again wonder when this could happen (no matching V4L2 colorspace). > > I understand if we were matching on the device supported colorospace, > > but we're comparing against the ones defined by v4l2 in general. > > As above, at the moment this can't happen, though maybe one day... > Returning an error code seems reasonable to me in this case so that a > problem gets flagged up. It's unlikely that you're going to want to > "fix" V4L2, or a driver, to give you the colour space you want, but I disagree ;-) If the Kernel has a fault, or is missing a feature - it should be fixed or added. That's the point of it being open source. We're not reliant on waiting for some external company to provide it to us. The user/developer who hits the issue is exactly the one who should be pushing the kernel side too. I know lots of people get scared of the kernel, but even just reporting it to the linux-media list (or libcamera) would be enough to make it noticed. In this instance, if somehow we have added a colour space that is not supported by V4L2 ... then either libcamera needs to convert it (correctly?) or V4L2 needs to support it if it gets used? At this part, we're only mapping a libcamera colour space to a v4l2 color space. So that should always be possible in some form or it's a bug that needs to be fixed. If the driver doesn't support the color space, that's separate of course, and should be handled when setting/getting. But we should always be able to map a libcamera description to an equivalent kernel description. And if we can't - we can and should report an error. > you should still know that there are some problems that may need > attention. > > Thanks! > David > > > > > Thanks > > j > > > > > + > > > + /* > > > + * If the colorSpace doesn't precisely match a standard color space, > > > + * then we must choose a V4L2 colorspace with matching primaries. > > > + */ > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > > + if (itPrimaries != primariesToV4l2.end()) > > > + v4l2Format.colorspace = itPrimaries->second; > > > + else > > > + ret = -1; > > > + > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > > + else > > > + ret = -1; > > > + > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > > + if (itTransfer != transferFunctionToV4l2.end()) > > > + v4l2Format.xfer_func = itTransfer->second; > > > + else > > > + ret = -1; > > > + > > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > > + if (itRange != rangeToV4l2.end()) > > > + v4l2Format.quantization = itRange->second; > > > + else > > > + ret = -1; Hrm ... I think I've already commented on those, but just in case, I think these should be -EINVAL. We could also have an explicit \retval too. > > > + > > > + return ret; > > > +} > > > + > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > > + > > > } /* namespace libcamera */ > > > -- > > > 2.20.1 > > >
Hi David, Kieran, On Thu, Nov 25, 2021 at 11:40:03AM +0000, Kieran Bingham wrote: > Quoting David Plowman (2021-11-25 11:06:08) > > Hi Jacopo > > > > Thanks for reviewing this! > > > > On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi David > > > > > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote: > > > > These methods are added to the base V4L2Device class so that they can > > > > be shared both by the video device class and subdevices. > > > > > > > > With the ColorSpace class, the color space and related other fields > > > > are stored together, corresponding to a number of fields in the > > > > various different V4L2 format structures. Template methods are > > > > therefore a convenient implementation, and we must explicitly > > > > instantiate the templates that will be needed. > > > > > > > > Note that unset color spaces are converted to requests for the > > > > device's "default" color space. > > > > > > You seem to use "unset" in place of default in this version. > > > > Yes, as a result of our discussion I went back to using std::optional > > so there is a genuine notion of it being "unset". Of course, there's > > then the question of what that means and it's true that when you > > request a ColorSpace that is "unset", our V4L2 layer will ask the > > drivers for their "default" colour space. So it's the same. Does that > > make sense? > > > > > > > > Also, this is a nice explanation about the implementation but > > > what something like the following as first paragraph ? > > > > > > Add functions to the V4L2Device class to convert to and from > > > libcamera ColorSpace. > > > > Yes, good idea. > > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/internal/v4l2_device.h | 7 + > > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > > > 2 files changed, 197 insertions(+) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > > index f21bc370..89ff6120 100644 > > > > --- a/include/libcamera/internal/v4l2_device.h > > > > +++ b/include/libcamera/internal/v4l2_device.h > > > > @@ -17,6 +17,7 @@ > > > > #include <libcamera/base/signal.h> > > > > #include <libcamera/base/span.h> > > > > > > > > +#include <libcamera/color_space.h> > > > > #include <libcamera/controls.h> > > > > > > > > namespace libcamera { > > > > @@ -44,6 +45,12 @@ public: > > > > > > > > void updateControlInfo(); > > > > > > > > + template<typename T> > > > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > > > + > > > > + template<typename T> > > > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > > > + > > > > > > Can't we have this protected and the template specializations as > > > public in the appropriate classes ? Not sure if it's possible, but I > > > guess it's desirable ? > > > > I'll see if I can make those protected. > > > > > > > > > protected: > > > > V4L2Device(const std::string &deviceNode); > > > > ~V4L2Device(); > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index 9c783c9c..4d6985aa 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -16,6 +16,8 @@ > > > > #include <sys/syscall.h> > > > > #include <unistd.h> > > > > > > > > +#include <linux/v4l2-mediabus.h> > > > > + > > > > > > Is this just for the template specialization ? > > > > Not sure that I remember any more! But I'll double-check if I really need it. > > > > > > > > > #include <libcamera/base/event_notifier.h> > > > > #include <libcamera/base/log.h> > > > > #include <libcamera/base/utils.h> > > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > > > frameStart.emit(event.u.frame_sync.frame_sequence); > > > > } > > > > > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > > > +}; > > > > + > > > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > > > +}; > > > > + > > > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > > > +}; > > > > + > > > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > > > +}; > > > > + > > > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > > +}; > > > > + > > > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > > +}; > > > > + > > > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > > > +}; > > > > + > > > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > > > +}; > > > > + > > > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > > > +}; > > > > + > > > > +/** > > > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > > > + * \param[in] v4l2Format A V4L2 format containing color space information > > > > + * > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > > > + * > > > > + * If any V4L2 fields are not recognised then we return an "unset" > > > > + * color space. > > > > + * > > > > + * \return The ColorSpace corresponding to the input V4L2 format > > > > > > You can return nullptr, this should be documented > > > > It can return a nullopt. If it couldn't return a nullopt I wouldn't > > have returned a std::optional at all, so I'm thinking this is OK? > > I think something like this should be added (after the existing return) > > \retval std::nullopt One or more V4L2 color space fields were not recognised > > It duplicates what you've written above, but it directly documents the > return values in the Doxygen output. > > > > > > > > + */ > > > > +template<typename T> > > > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > > > +{ > > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > > > + if (itColor == v4l2ToColorSpace.end()) > > > > + return std::nullopt; > > > > + > > > > + /* This sets all the color space fields to the correct "default" values. */ > > > > + ColorSpace colorSpace = itColor->second; > > > > + > > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > > > + return std::nullopt; > > > > + > > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > > > + } > > > > + > > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > > > + if (itTransfer == v4l2ToTransferFunction.end()) > > > > + return std::nullopt; > > > > + > > > > + colorSpace.transferFunction = itTransfer->second; > > > > + } > > > > + > > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > > > + if (itRange == v4l2ToRange.end()) > > > > + return std::nullopt; > > > > > > Should all of these cases be associated with an error message, asking > > > to update the ColorSpace class to add the new > > > encoding/xfer_func/quantization methods not yet supported ? > > > > Currently I'm signalling errors where these values get used. I'm > > always a bit nervous about outputting error messages at the lowest > > level unless you're really sure, on the grounds that they can be hard > > to turn off! But I don't feel very strongly... > > > > > > + > > > > + colorSpace.range = itRange->second; > > > > + } > > > > + > > > > + return colorSpace; > > > > +} > > > > + > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > > > + > > > > +/** > > > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > > > + * \param[in] colorSpace The ColorSpace to be converted > > > > + * \param[out] v4l2Format A V4L2 format containing color space information > > > > > > I'm a bit uncertain about the two functions having different > > > prototypes... > > > > Yes, the catch with filling in V4L2 fields is that there's no way to > > say "I couldn't do it", so there's an extra return code to signal > > that. > > > > > > > > > + * > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > > > + * > > > > + * An error is returned if any of the V4L2 fields do not support the > > > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > > > + * "default" values, but all other fields are still filled in where > > > > + * possible. > > > > + * > > > > + * If the color space is completely unset, "default" V4L2 values are used > > > > + * everywhere, so a driver would then choose its preferred color space. > > > > > > We're translating to libcamera::ColorSpace to V4L2 fields. > > > > > > The returned color space is adjusted if nothing in V4L2 matches a > > > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you > > > think this could ever happen ? > > > > Not for the time being, though possibly further in the future. I think > > it's probably fair enough if you can't get the colour space that you > > want? > > > > > > > > > + * > > > > + * \return 0 on success or a negative error code otherwise > > > > + */ > > > > +template<typename T> > > > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > > > +{ > > > > + int ret = 0; > > > > + > > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > > > + > > > > + if (!colorSpace) > > > > + return ret; > > > > > > > > > return 0 and declare ret later ? > > > > > > > + > > > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > > > + [&colorSpace](const auto &item) { > > > > + return colorSpace == item.first; > > > > + }); > > > > + if (itColor != colorSpaceToV4l2.end()) { > > > > + v4l2Format.colorspace = itColor->second; > > > > + /* Leaving all the other fields as "default" should be fine. */ > > > > + return ret; > > > > + } > > > > > > I again wonder when this could happen (no matching V4L2 colorspace). > > > I understand if we were matching on the device supported colorospace, > > > but we're comparing against the ones defined by v4l2 in general. > > > > As above, at the moment this can't happen, though maybe one day... > > Returning an error code seems reasonable to me in this case so that a > > problem gets flagged up. It's unlikely that you're going to want to > > "fix" V4L2, or a driver, to give you the colour space you want, but > > I disagree ;-) > > If the Kernel has a fault, or is missing a feature - it should be fixed > or added. That's the point of it being open source. We're not reliant on > waiting for some external company to provide it to us. The > user/developer who hits the issue is exactly the one who should be > pushing the kernel side too. > > I know lots of people get scared of the kernel, but even just reporting > it to the linux-media list (or libcamera) would be enough to make it > noticed. > > In this instance, if somehow we have added a colour space that is not > supported by V4L2 ... then either libcamera needs to convert it > (correctly?) or V4L2 needs to support it if it gets used? > > At this part, we're only mapping a libcamera colour space to a v4l2 > color space. So that should always be possible in some form or it's a > bug that needs to be fixed. > > If the driver doesn't support the color space, that's separate of > course, and should be handled when setting/getting. > > But we should always be able to map a libcamera description to an > equivalent kernel description. And if we can't - we can and should > report an error. > I concur. I'm not getting why we're so sensitive about color spaces mismatches. If anything needs to be supported in kernel or libcamera to have a device working properly, we should be loud and warn users they need to update the faulty side. We do the same for CameraSensor required controls, where so far we've been complaining about missing features, but we should sooner or later refuse to operate to have the sensor drivers updated to the minimum required features set. I know it might sound harsh, but I don't see other ways to actually reach a standardization. > > > you should still know that there are some problems that may need > > attention. > > > > Thanks! > > David > > > > > > > > Thanks > > > j > > > > > > > + > > > > + /* > > > > + * If the colorSpace doesn't precisely match a standard color space, > > > > + * then we must choose a V4L2 colorspace with matching primaries. > > > > + */ > > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > > > + if (itPrimaries != primariesToV4l2.end()) > > > > + v4l2Format.colorspace = itPrimaries->second; > > > > + else > > > > + ret = -1; > > > > + > > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > > > + else > > > > + ret = -1; > > > > + > > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > > > + if (itTransfer != transferFunctionToV4l2.end()) > > > > + v4l2Format.xfer_func = itTransfer->second; > > > > + else > > > > + ret = -1; > > > > + > > > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > > > + if (itRange != rangeToV4l2.end()) > > > > + v4l2Format.quantization = itRange->second; > > > > + else > > > > + ret = -1; > > Hrm ... I think I've already commented on those, but just in case, I > think these should be -EINVAL. > > We could also have an explicit \retval too. > > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > > > + > > > > } /* namespace libcamera */ > > > > -- > > > > 2.20.1 > > > >
Hi everyone On Thu, 25 Nov 2021 at 11:53, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, Kieran, > > On Thu, Nov 25, 2021 at 11:40:03AM +0000, Kieran Bingham wrote: > > Quoting David Plowman (2021-11-25 11:06:08) > > > Hi Jacopo > > > > > > Thanks for reviewing this! > > > > > > On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi David > > > > > > > > On Thu, Nov 18, 2021 at 03:19:29PM +0000, David Plowman wrote: > > > > > These methods are added to the base V4L2Device class so that they can > > > > > be shared both by the video device class and subdevices. > > > > > > > > > > With the ColorSpace class, the color space and related other fields > > > > > are stored together, corresponding to a number of fields in the > > > > > various different V4L2 format structures. Template methods are > > > > > therefore a convenient implementation, and we must explicitly > > > > > instantiate the templates that will be needed. > > > > > > > > > > Note that unset color spaces are converted to requests for the > > > > > device's "default" color space. > > > > > > > > You seem to use "unset" in place of default in this version. > > > > > > Yes, as a result of our discussion I went back to using std::optional > > > so there is a genuine notion of it being "unset". Of course, there's > > > then the question of what that means and it's true that when you > > > request a ColorSpace that is "unset", our V4L2 layer will ask the > > > drivers for their "default" colour space. So it's the same. Does that > > > make sense? > > > > > > > > > > > Also, this is a nice explanation about the implementation but > > > > what something like the following as first paragraph ? > > > > > > > > Add functions to the V4L2Device class to convert to and from > > > > libcamera ColorSpace. > > > > > > Yes, good idea. > > > > > > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > include/libcamera/internal/v4l2_device.h | 7 + > > > > > src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ > > > > > 2 files changed, 197 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > > > > index f21bc370..89ff6120 100644 > > > > > --- a/include/libcamera/internal/v4l2_device.h > > > > > +++ b/include/libcamera/internal/v4l2_device.h > > > > > @@ -17,6 +17,7 @@ > > > > > #include <libcamera/base/signal.h> > > > > > #include <libcamera/base/span.h> > > > > > > > > > > +#include <libcamera/color_space.h> > > > > > #include <libcamera/controls.h> > > > > > > > > > > namespace libcamera { > > > > > @@ -44,6 +45,12 @@ public: > > > > > > > > > > void updateControlInfo(); > > > > > > > > > > + template<typename T> > > > > > + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > > > > + > > > > > + template<typename T> > > > > > + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); > > > > > + > > > > > > > > Can't we have this protected and the template specializations as > > > > public in the appropriate classes ? Not sure if it's possible, but I > > > > guess it's desirable ? > > > > > > I'll see if I can make those protected. > > > > > > > > > > > > protected: > > > > > V4L2Device(const std::string &deviceNode); > > > > > ~V4L2Device(); > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > > index 9c783c9c..4d6985aa 100644 > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > @@ -16,6 +16,8 @@ > > > > > #include <sys/syscall.h> > > > > > #include <unistd.h> > > > > > > > > > > +#include <linux/v4l2-mediabus.h> > > > > > + > > > > > > > > Is this just for the template specialization ? > > > > > > Not sure that I remember any more! But I'll double-check if I really need it. > > > > > > > > > > > > #include <libcamera/base/event_notifier.h> > > > > > #include <libcamera/base/log.h> > > > > > #include <libcamera/base/utils.h> > > > > > @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() > > > > > frameStart.emit(event.u.frame_sync.frame_sequence); > > > > > } > > > > > > > > > > +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { > > > > > + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, > > > > > + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, > > > > > + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, > > > > > + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, > > > > > + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, > > > > > + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, > > > > > +}; > > > > > + > > > > > +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { > > > > > + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, > > > > > + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, > > > > > + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, > > > > > +}; > > > > > + > > > > > +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { > > > > > + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, > > > > > + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, > > > > > + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, > > > > > +}; > > > > > + > > > > > +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { > > > > > + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, > > > > > + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, > > > > > +}; > > > > > + > > > > > +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { > > > > > + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, > > > > > + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, > > > > > + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, > > > > > + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > > > + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, > > > > > + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > > > +}; > > > > > + > > > > > +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { > > > > > + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, > > > > > + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, > > > > > + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, > > > > > + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, > > > > > +}; > > > > > + > > > > > +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { > > > > > + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, > > > > > + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, > > > > > + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, > > > > > +}; > > > > > + > > > > > +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { > > > > > + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, > > > > > + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, > > > > > + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, > > > > > +}; > > > > > + > > > > > +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { > > > > > + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, > > > > > + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, > > > > > +}; > > > > > + > > > > > +/** > > > > > + * \brief Convert the color space fields in a V4L2 format to a ColorSpace > > > > > + * \param[in] v4l2Format A V4L2 format containing color space information > > > > > + * > > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > > > + * V4L2 format structure are converted to a corresponding ColorSpace. > > > > > + * > > > > > + * If any V4L2 fields are not recognised then we return an "unset" > > > > > + * color space. > > > > > + * > > > > > + * \return The ColorSpace corresponding to the input V4L2 format > > > > > > > > You can return nullptr, this should be documented > > > > > > It can return a nullopt. If it couldn't return a nullopt I wouldn't > > > have returned a std::optional at all, so I'm thinking this is OK? > > > > I think something like this should be added (after the existing return) > > > > \retval std::nullopt One or more V4L2 color space fields were not recognised > > > > It duplicates what you've written above, but it directly documents the > > return values in the Doxygen output. Yes, I like that. Shows how much I know about Doxygen (i.e. nothing!). > > > > > > > > > > > + */ > > > > > +template<typename T> > > > > > +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) > > > > > +{ > > > > > + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); > > > > > + if (itColor == v4l2ToColorSpace.end()) > > > > > + return std::nullopt; > > > > > + > > > > > + /* This sets all the color space fields to the correct "default" values. */ > > > > > + ColorSpace colorSpace = itColor->second; > > > > > + > > > > > + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { > > > > > + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); > > > > > + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) > > > > > + return std::nullopt; > > > > > + > > > > > + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; > > > > > + } > > > > > + > > > > > + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { > > > > > + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); > > > > > + if (itTransfer == v4l2ToTransferFunction.end()) > > > > > + return std::nullopt; > > > > > + > > > > > + colorSpace.transferFunction = itTransfer->second; > > > > > + } > > > > > + > > > > > + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { > > > > > + auto itRange = v4l2ToRange.find(v4l2Format.quantization); > > > > > + if (itRange == v4l2ToRange.end()) > > > > > + return std::nullopt; > > > > > > > > Should all of these cases be associated with an error message, asking > > > > to update the ColorSpace class to add the new > > > > encoding/xfer_func/quantization methods not yet supported ? > > > > > > Currently I'm signalling errors where these values get used. I'm > > > always a bit nervous about outputting error messages at the lowest > > > level unless you're really sure, on the grounds that they can be hard > > > to turn off! But I don't feel very strongly... > > > > > > > > + > > > > > + colorSpace.range = itRange->second; > > > > > + } > > > > > + > > > > > + return colorSpace; > > > > > +} > > > > > + > > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); > > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); > > > > > +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); > > > > > + > > > > > +/** > > > > > + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace > > > > > + * \param[in] colorSpace The ColorSpace to be converted > > > > > + * \param[out] v4l2Format A V4L2 format containing color space information > > > > > > > > I'm a bit uncertain about the two functions having different > > > > prototypes... > > > > > > Yes, the catch with filling in V4L2 fields is that there's no way to > > > say "I couldn't do it", so there's an extra return code to signal > > > that. > > > > > > > > > > > > + * > > > > > + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a > > > > > + * V4L2 format structure are filled in from a corresponding ColorSpace. > > > > > + * > > > > > + * An error is returned if any of the V4L2 fields do not support the > > > > > + * value given in the ColorSpace. Such fields are set to the V4L2 > > > > > + * "default" values, but all other fields are still filled in where > > > > > + * possible. > > > > > + * > > > > > + * If the color space is completely unset, "default" V4L2 values are used > > > > > + * everywhere, so a driver would then choose its preferred color space. > > > > > > > > We're translating to libcamera::ColorSpace to V4L2 fields. > > > > > > > > The returned color space is adjusted if nothing in V4L2 matches a > > > > libcamera defined colorspace/ecoding/quantization/xfer_func. Do you > > > > think this could ever happen ? > > > > > > Not for the time being, though possibly further in the future. I think > > > it's probably fair enough if you can't get the colour space that you > > > want? > > > > > > > > > > > > + * > > > > > + * \return 0 on success or a negative error code otherwise > > > > > + */ > > > > > +template<typename T> > > > > > +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) > > > > > +{ > > > > > + int ret = 0; > > > > > + > > > > > + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; > > > > > + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > > > + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > > + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; > > > > > + > > > > > + if (!colorSpace) > > > > > + return ret; > > > > > > > > > > > > return 0 and declare ret later ? > > > > > > > > > + > > > > > + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), > > > > > + [&colorSpace](const auto &item) { > > > > > + return colorSpace == item.first; > > > > > + }); > > > > > + if (itColor != colorSpaceToV4l2.end()) { > > > > > + v4l2Format.colorspace = itColor->second; > > > > > + /* Leaving all the other fields as "default" should be fine. */ > > > > > + return ret; > > > > > + } > > > > > > > > I again wonder when this could happen (no matching V4L2 colorspace). > > > > I understand if we were matching on the device supported colorospace, > > > > but we're comparing against the ones defined by v4l2 in general. > > > > > > As above, at the moment this can't happen, though maybe one day... > > > Returning an error code seems reasonable to me in this case so that a > > > problem gets flagged up. It's unlikely that you're going to want to > > > "fix" V4L2, or a driver, to give you the colour space you want, but > > > > I disagree ;-) > > > > If the Kernel has a fault, or is missing a feature - it should be fixed > > or added. That's the point of it being open source. We're not reliant on > > waiting for some external company to provide it to us. The > > user/developer who hits the issue is exactly the one who should be > > pushing the kernel side too. > > > > I know lots of people get scared of the kernel, but even just reporting > > it to the linux-media list (or libcamera) would be enough to make it > > noticed. > > > > In this instance, if somehow we have added a colour space that is not > > supported by V4L2 ... then either libcamera needs to convert it > > (correctly?) or V4L2 needs to support it if it gets used? > > > > At this part, we're only mapping a libcamera colour space to a v4l2 > > color space. So that should always be possible in some form or it's a > > bug that needs to be fixed. > > > > If the driver doesn't support the color space, that's separate of > > course, and should be handled when setting/getting. > > > > But we should always be able to map a libcamera description to an > > equivalent kernel description. And if we can't - we can and should > > report an error. > > > > I concur. I'm not getting why we're so sensitive about color spaces > mismatches. If anything needs to be supported in kernel or libcamera > to have a device working properly, we should be loud and warn users > they need to update the faulty side. We do the same for CameraSensor > required controls, where so far we've been complaining about missing > features, but we should sooner or later refuse to operate to have the > sensor drivers updated to the minimum required features set. I know it > might sound harsh, but I don't see other ways to actually reach a > standardization. I agree that fixing the thing that is broken is the right thing to do, though I'm still a bit unconvinced what will happen in practice. Will a random developer with some random device that they need to make work on an old kernel go to this trouble? I don't think I'm as optimistic as you! But I'm fine to flag errors when things are missing - indeed this is exactly what it already does (though maybe with "Warning" rather than "Error", but that's trivial to change). So I think this is all in order. David > > > > > > you should still know that there are some problems that may need > > > attention. > > > > > > Thanks! > > > David > > > > > > > > > > > Thanks > > > > j > > > > > > > > > + > > > > > + /* > > > > > + * If the colorSpace doesn't precisely match a standard color space, > > > > > + * then we must choose a V4L2 colorspace with matching primaries. > > > > > + */ > > > > > + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); > > > > > + if (itPrimaries != primariesToV4l2.end()) > > > > > + v4l2Format.colorspace = itPrimaries->second; > > > > > + else > > > > > + ret = -1; > > > > > + > > > > > + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); > > > > > + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) > > > > > + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; > > > > > + else > > > > > + ret = -1; > > > > > + > > > > > + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); > > > > > + if (itTransfer != transferFunctionToV4l2.end()) > > > > > + v4l2Format.xfer_func = itTransfer->second; > > > > > + else > > > > > + ret = -1; > > > > > + > > > > > + auto itRange = rangeToV4l2.find(colorSpace->range); > > > > > + if (itRange != rangeToV4l2.end()) > > > > > + v4l2Format.quantization = itRange->second; > > > > > + else > > > > > + ret = -1; > > > > Hrm ... I think I've already commented on those, but just in case, I > > think these should be -EINVAL. > > > > We could also have an explicit \retval too. > > > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); > > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); > > > > > +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); > > > > > + > > > > > } /* namespace libcamera */ > > > > > -- > > > > > 2.20.1 > > > > >
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index f21bc370..89ff6120 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -17,6 +17,7 @@ #include <libcamera/base/signal.h> #include <libcamera/base/span.h> +#include <libcamera/color_space.h> #include <libcamera/controls.h> namespace libcamera { @@ -44,6 +45,12 @@ public: void updateControlInfo(); + template<typename T> + static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); + + template<typename T> + static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format); + protected: V4L2Device(const std::string &deviceNode); ~V4L2Device(); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 9c783c9c..4d6985aa 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -16,6 +16,8 @@ #include <sys/syscall.h> #include <unistd.h> +#include <linux/v4l2-mediabus.h> + #include <libcamera/base/event_notifier.h> #include <libcamera/base/log.h> #include <libcamera/base/utils.h> @@ -731,4 +733,192 @@ void V4L2Device::eventAvailable() frameStart.emit(event.u.frame_sync.frame_sequence); } +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = { + { V4L2_COLORSPACE_RAW, ColorSpace::Raw }, + { V4L2_COLORSPACE_JPEG, ColorSpace::Jpeg }, + { V4L2_COLORSPACE_SRGB, ColorSpace::Srgb }, + { V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m }, + { V4L2_COLORSPACE_REC709, ColorSpace::Rec709 }, + { V4L2_COLORSPACE_BT2020, ColorSpace::Rec2020 }, +}; + +static const std::map<uint32_t, ColorSpace::YcbcrEncoding> v4l2ToYcbcrEncoding = { + { V4L2_YCBCR_ENC_601, ColorSpace::YcbcrEncoding::Rec601 }, + { V4L2_YCBCR_ENC_709, ColorSpace::YcbcrEncoding::Rec709 }, + { V4L2_YCBCR_ENC_BT2020, ColorSpace::YcbcrEncoding::Rec2020 }, +}; + +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = { + { V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::Linear }, + { V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::Srgb }, + { V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::Rec709 }, +}; + +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = { + { V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::Full }, + { V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::Limited }, +}; + +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = { + { ColorSpace::Raw, V4L2_COLORSPACE_RAW }, + { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG }, + { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB }, + { ColorSpace::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, + { ColorSpace::Rec709, V4L2_COLORSPACE_REC709 }, + { ColorSpace::Rec2020, V4L2_COLORSPACE_BT2020 }, +}; + +static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = { + { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW }, + { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M }, + { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 }, + { ColorSpace::Primaries::Rec2020, V4L2_COLORSPACE_BT2020 }, +}; + +static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = { + { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 }, + { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 }, + { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 }, +}; + +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = { + { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE }, + { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB }, + { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 }, +}; + +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = { + { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE }, + { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE }, +}; + +/** + * \brief Convert the color space fields in a V4L2 format to a ColorSpace + * \param[in] v4l2Format A V4L2 format containing color space information + * + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a + * V4L2 format structure are converted to a corresponding ColorSpace. + * + * If any V4L2 fields are not recognised then we return an "unset" + * color space. + * + * \return The ColorSpace corresponding to the input V4L2 format + */ +template<typename T> +std::optional<ColorSpace> V4L2Device::toColorSpace(const T &v4l2Format) +{ + auto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace); + if (itColor == v4l2ToColorSpace.end()) + return std::nullopt; + + /* This sets all the color space fields to the correct "default" values. */ + ColorSpace colorSpace = itColor->second; + + if (v4l2Format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT) { + auto itYcbcrEncoding = v4l2ToYcbcrEncoding.find(v4l2Format.ycbcr_enc); + if (itYcbcrEncoding == v4l2ToYcbcrEncoding.end()) + return std::nullopt; + + colorSpace.ycbcrEncoding = itYcbcrEncoding->second; + } + + if (v4l2Format.xfer_func != V4L2_XFER_FUNC_DEFAULT) { + auto itTransfer = v4l2ToTransferFunction.find(v4l2Format.xfer_func); + if (itTransfer == v4l2ToTransferFunction.end()) + return std::nullopt; + + colorSpace.transferFunction = itTransfer->second; + } + + if (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) { + auto itRange = v4l2ToRange.find(v4l2Format.quantization); + if (itRange == v4l2ToRange.end()) + return std::nullopt; + + colorSpace.range = itRange->second; + } + + return colorSpace; +} + +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format &); +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &); +template std::optional<ColorSpace> V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &); + +/** + * \brief Fill in the color space fields of a V4L2 format from a ColorSpace + * \param[in] colorSpace The ColorSpace to be converted + * \param[out] v4l2Format A V4L2 format containing color space information + * + * The colorspace, ycbcr_enc, xfer_func and quantization fields within a + * V4L2 format structure are filled in from a corresponding ColorSpace. + * + * An error is returned if any of the V4L2 fields do not support the + * value given in the ColorSpace. Such fields are set to the V4L2 + * "default" values, but all other fields are still filled in where + * possible. + * + * If the color space is completely unset, "default" V4L2 values are used + * everywhere, so a driver would then choose its preferred color space. + * + * \return 0 on success or a negative error code otherwise + */ +template<typename T> +int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format) +{ + int ret = 0; + + v4l2Format.colorspace = V4L2_COLORSPACE_DEFAULT; + v4l2Format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; + v4l2Format.xfer_func = V4L2_XFER_FUNC_DEFAULT; + v4l2Format.quantization = V4L2_QUANTIZATION_DEFAULT; + + if (!colorSpace) + return ret; + + auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), + [&colorSpace](const auto &item) { + return colorSpace == item.first; + }); + if (itColor != colorSpaceToV4l2.end()) { + v4l2Format.colorspace = itColor->second; + /* Leaving all the other fields as "default" should be fine. */ + return ret; + } + + /* + * If the colorSpace doesn't precisely match a standard color space, + * then we must choose a V4L2 colorspace with matching primaries. + */ + auto itPrimaries = primariesToV4l2.find(colorSpace->primaries); + if (itPrimaries != primariesToV4l2.end()) + v4l2Format.colorspace = itPrimaries->second; + else + ret = -1; + + auto itYcbcrEncoding = ycbcrEncodingToV4l2.find(colorSpace->ycbcrEncoding); + if (itYcbcrEncoding != ycbcrEncodingToV4l2.end()) + v4l2Format.ycbcr_enc = itYcbcrEncoding->second; + else + ret = -1; + + auto itTransfer = transferFunctionToV4l2.find(colorSpace->transferFunction); + if (itTransfer != transferFunctionToV4l2.end()) + v4l2Format.xfer_func = itTransfer->second; + else + ret = -1; + + auto itRange = rangeToV4l2.find(colorSpace->range); + if (itRange != rangeToV4l2.end()) + v4l2Format.quantization = itRange->second; + else + ret = -1; + + return ret; +} + +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format &); +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_pix_format_mplane &); +template int V4L2Device::fromColorSpace(const std::optional<ColorSpace> &, struct v4l2_mbus_framefmt &); + } /* namespace libcamera */
These methods are added to the base V4L2Device class so that they can be shared both by the video device class and subdevices. With the ColorSpace class, the color space and related other fields are stored together, corresponding to a number of fields in the various different V4L2 format structures. Template methods are therefore a convenient implementation, and we must explicitly instantiate the templates that will be needed. Note that unset color spaces are converted to requests for the device's "default" color space. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/internal/v4l2_device.h | 7 + src/libcamera/v4l2_device.cpp | 190 +++++++++++++++++++++++ 2 files changed, 197 insertions(+)