[libcamera-devel] [PATCH v6 3/7] libcamera: Convert between ColorSpace class and V4L2 formats
David Plowman
david.plowman at raspberrypi.com
Thu Nov 25 13:14:05 CET 2021
Hi everyone
On Thu, 25 Nov 2021 at 11:53, Jacopo Mondi <jacopo at 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 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?
> >
> > 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
> > > > >
More information about the libcamera-devel
mailing list