[libcamera-devel] [PATCH v6 3/7] libcamera: Convert between ColorSpace class and V4L2 formats
David Plowman
david.plowman at raspberrypi.com
Thu Nov 25 12:06:08 CET 2021
Hi Jacopo
Thanks for reviewing this!
On Wed, 24 Nov 2021 at 16:05, Jacopo Mondi <jacopo at 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 at 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
> >
More information about the libcamera-devel
mailing list