[libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Feb 28 12:36:17 CET 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-02-28 11:27:37 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Fri, Feb 28, 2020 at 04:29:10AM +0100, Niklas Söderlund wrote:
> > Create a class to represent the pixel formats. This is done to prepare
> > to add support for modifiers for the formats. One bonus with this change
> > it that it's allows it to make it impossible for other then designated
> > classes (V4L2VideoDevice and V4L2CameraProxy) to create PixelFormat
> > instances from a V4L2 fourcc limiting the risk of users of the class to
> > mix the two fourcc namespaces unintentionally.
> >
> > The patch is unfortunately quiet large as it have to touch a lot of
> > different ares of the code to simultaneously switch to the new class
> > based PixelFormat implementation.
> >
>
> I prasie the effort of making PixelFormat more descriptive and
> centralize there the conversion between different 4cc sets, but I'm
> not sure I like the way pipeline handlers can now construct
> PixelFormat from both DRM and V4L2 fourcc. Please see below.
No they can't the constructor to create a PixelFormat from a V4L2 fourcc
is protected and only allowed to be called from V4L2VideoDevice and
V4L2CameraProxy. This I think is fair as they really are the two
locations where we have V4L2 fourc and need to create a PixelFormat from
them :-)
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/pixelformats.h | 40 +++++++-
> > src/cam/main.cpp | 4 +-
> > src/libcamera/formats.cpp | 3 +
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 22 ++--
> > src/libcamera/pipeline/uvcvideo.cpp | 4 +-
> > src/libcamera/pipeline/vimc.cpp | 12 +--
> > src/libcamera/pixelformats.cpp | 124 +++++++++++++++++++++++
> > src/libcamera/stream.cpp | 6 +-
> > src/libcamera/v4l2_videodevice.cpp | 101 +-----------------
> > src/qcam/format_converter.cpp | 2 +-
> > src/qcam/viewfinder.cpp | 2 +-
> > src/v4l2/v4l2_camera_proxy.cpp | 49 ++++-----
> > test/stream/stream_formats.cpp | 33 +++---
> > 14 files changed, 236 insertions(+), 172 deletions(-)
> >
> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> > index 6e25b8d8b76e5961..f0951e983192d5e8 100644
> > --- a/include/libcamera/pixelformats.h
> > +++ b/include/libcamera/pixelformats.h
> > @@ -7,11 +7,49 @@
> > #ifndef __LIBCAMERA_PIXEL_FORMATS_H__
> > #define __LIBCAMERA_PIXEL_FORMATS_H__
> >
> > +#include <set>
> > #include <stdint.h>
> > +#include <string>
> > +
> > +/* Needs to be forward declared in the global namespace. */
> > +class V4L2CameraProxy;
> >
> > namespace libcamera {
> >
> > -using PixelFormat = uint32_t;
> > +struct PixelFormatEntry;
> > +
> > +class PixelFormat
> > +{
> > +public:
> > + PixelFormat();
> > + PixelFormat(const PixelFormat &other);
> > + explicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);
> > +
> > + PixelFormat &operator=(const PixelFormat &other);
> > +
> > + bool operator==(const PixelFormat &other) const;
> > + bool operator!=(const PixelFormat &other) const;
> > + bool operator<(const PixelFormat &other) const;
> > +
> > + uint32_t v4l2() const;
> > + uint32_t fourcc() const;
> > + const std::set<uint32_t> &modifiers() const;
> > +
> > +protected:
> > + /* Needed so V4L2VideoDevice can create PixelFormat from V4L2 fourcc. */
> > + friend class V4L2VideoDevice;
> > +
> > + /* Needed so V4L2CameraProxy can create PixelFormat from V4L2 fourcc. */
> > + friend V4L2CameraProxy;
> > +
> > + explicit PixelFormat(uint32_t v4l2_fourcc);
> > +
> > +private:
> > + const PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;
> > + const PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;
> > +
> > + const PixelFormatEntry *format_;
> > +};
> >
> > } /* namespace libcamera */
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index ad4be55fc114fe22..c8ef79daea37d8b6 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -249,7 +249,7 @@ int CamApp::prepareConfig()
> >
> > /* TODO: Translate 4CC string to ID. */
> > if (opt.isSet("pixelformat"))
> > - cfg.pixelFormat = opt["pixelformat"];
> > + cfg.pixelFormat = PixelFormat(opt["pixelformat"], {});
> > }
> > }
> >
> > @@ -283,7 +283,7 @@ int CamApp::infoConfiguration()
> > const StreamFormats &formats = cfg.formats();
> > for (PixelFormat pixelformat : formats.pixelformats()) {
> > std::cout << " * Pixelformat: 0x" << std::hex
> > - << std::setw(8) << pixelformat << " "
> > + << std::setw(8) << pixelformat.fourcc() << " "
> > << formats.range(pixelformat).toString()
> > << std::endl;
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 98817deee2b54c84..e3a89121e3c60151 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -9,6 +9,8 @@
> >
> > #include <errno.h>
> >
> > +#include <libcamera/pixelformats.h>
> > +
> > /**
> > * \file formats.h
> > * \brief Types and helper methods to handle libcamera image formats
> > @@ -110,5 +112,6 @@ const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const
> > }
> >
> > template class ImageFormats<unsigned int>;
> > +template class ImageFormats<PixelFormat>;
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 33f97b340716abd0..71ac0ae33921f548 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -247,7 +247,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> > void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> > {
> > /* The only pixel format the driver supports is NV12. */
> > - cfg.pixelFormat = DRM_FORMAT_NV12;
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});
>
> This is good. However you use the second parameter to differentiate
> between constructing a PixelFormat from DRM or V4L2, which seems a bit
> fragile and prevents you from defaulting the second parameter to an
> empty modifier. What is the point of requesting to provide {} if no
> modifier is associated with the format ?
I agree it would be nice to find a way to make the modifiers argument
optional. I'm toying with the idea of replacing the
PixelFormat(v4l2_fourcc) with a protected factory function as it can
only be called from two classes to make it more explicit that it is
dealing with special cases.
>
> >
> > if (scale) {
> > /*
> > @@ -402,7 +402,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > StreamConfiguration cfg = {};
> > IPU3Stream *stream = nullptr;
> >
> > - cfg.pixelFormat = DRM_FORMAT_NV12;
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});
> >
> > switch (role) {
> > case StreamRole::StillCapture:
> > @@ -1142,7 +1142,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > return 0;
> >
> > V4L2DeviceFormat outputFormat = {};
> > - outputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);
> > + outputFormat.fourcc = PixelFormat(DRM_FORMAT_NV12, {}).v4l2();
>
> Have you consider providing a static method to perform the DRM<->v4l2
> fourcc conversion ? Here you create a PixelFormat instance just to
> call .v4l2() on it. The only thing you care about is the translation
> between the two 4cc codes, wouldn't this be better expressed as
> PixelFormat::toV4L2(DRM_FORMAT_NV12);
I think this is a bad idea. What we want is a type to represent
PixelFormat that (from most places) only can be constructed from DRM
fourccs.
I toyed with the possibility of makeing PixelFormat::v4l2() protected to
"hide" it form all but the two V4L2 centric classes. This would be nice
expect then we would need DRM fourcc for all Meta formats which is
probably not what we want.
> ?
>
> > outputFormat.size = cfg.size;
> > outputFormat.planesCount = 2;
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 13433b216747cb8b..323fa3596c6ee242 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -433,14 +433,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >
> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > {
> > - static const std::array<unsigned int, 8> formats{
> > - DRM_FORMAT_YUYV,
> > - DRM_FORMAT_YVYU,
> > - DRM_FORMAT_VYUY,
> > - DRM_FORMAT_NV16,
> > - DRM_FORMAT_NV61,
> > - DRM_FORMAT_NV21,
> > - DRM_FORMAT_NV12,
> > + static const std::array<PixelFormat, 8> formats{
> > + PixelFormat(DRM_FORMAT_YUYV, {}),
> > + PixelFormat(DRM_FORMAT_YVYU, {}),
> > + PixelFormat(DRM_FORMAT_VYUY, {}),
> > + PixelFormat(DRM_FORMAT_NV16, {}),
> > + PixelFormat(DRM_FORMAT_NV61, {}),
> > + PixelFormat(DRM_FORMAT_NV21, {}),
> > + PixelFormat(DRM_FORMAT_NV12, {}),
> > /* \todo Add support for 8-bit greyscale to DRM formats */
> > };
> >
> > @@ -462,7 +462,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> > formats.end()) {
>
> I am not sure what we get by making pipeline handler use the
> PixelFormat class here just to enumerate and match 4cc codes.
cfg.pixelFormat comes from the application and is a PixelFormat. It may
contains modifiers so we can not just comapre cfg.pixelFormat.fourcc()
with the V4L2 fourcc we need to compare the whole thing.
>
> > LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > - cfg.pixelFormat = DRM_FORMAT_NV12,
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});
>
> Here instead is good. Application should be provided a PixelFormat
> class
>
> > status = Adjusted;
> > }
> >
> > @@ -541,7 +541,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> > return config;
> >
> > StreamConfiguration cfg{};
> > - cfg.pixelFormat = DRM_FORMAT_NV12;
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});
> > cfg.size = data->sensor_->resolution();
> >
> > config->addConfiguration(cfg);
> > @@ -796,7 +796,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > /* Inform IPA of stream configuration and sensor controls. */
> > std::map<unsigned int, IPAStream> streamConfig;
> > streamConfig[0] = {
> > - .pixelFormat = data->stream_.configuration().pixelFormat,
> > + .pixelFormat = data->stream_.configuration().pixelFormat.fourcc(),
> > .size = data->stream_.configuration().size,
> > };
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 8efd188e75a3d135..5f3e52f691aaeae4 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -115,8 +115,8 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> > if (iter == pixelFormats.end()) {
> > cfg.pixelFormat = pixelFormats.front();
> > LOG(UVC, Debug)
> > - << "Adjusting pixel format from " << pixelFormat
> > - << " to " << cfg.pixelFormat;
> > + << "Adjusting pixel format from " << pixelFormat.fourcc()
> > + << " to " << cfg.pixelFormat.fourcc();
> > status = Adjusted;
> > }
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 9fbe33c626e327d4..a591c424919b0783 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -106,10 +106,10 @@ private:
> >
> > namespace {
> >
> > -constexpr std::array<unsigned int, 3> pixelformats{
> > - DRM_FORMAT_RGB888,
> > - DRM_FORMAT_BGR888,
> > - DRM_FORMAT_BGRA8888,
> > +static const std::array<PixelFormat, 3> pixelformats{
> > + PixelFormat(DRM_FORMAT_RGB888, {}),
> > + PixelFormat(DRM_FORMAT_BGR888, {}),
> > + PixelFormat(DRM_FORMAT_BGRA8888, {}),
> > };
> >
> > } /* namespace */
> > @@ -138,7 +138,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> > if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==
> > pixelformats.end()) {
> > LOG(VIMC, Debug) << "Adjusting format to RGB24";
> > - cfg.pixelFormat = DRM_FORMAT_BGR888;
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});
> > status = Adjusted;
> > }
> >
> > @@ -187,7 +187,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >
> > StreamConfiguration cfg(formats.data());
> >
> > - cfg.pixelFormat = DRM_FORMAT_BGR888;
> > + cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});
> > cfg.size = { 1920, 1080 };
> > cfg.bufferCount = 4;
> >
> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> > index c03335400b709d9b..b2aacbc39b9ca16a 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -7,6 +7,13 @@
> >
> > #include <libcamera/pixelformats.h>
> >
> > +#include <vector>
> > +
> > +#include <linux/drm_fourcc.h>
> > +#include <linux/videodev2.h>
> > +
> > +#include "log.h"
> > +
> > /**
> > * \file pixelformats.h
> > * \brief libcamera pixel formats
> > @@ -14,6 +21,8 @@
> >
> > namespace libcamera {
> >
> > +LOG_DEFINE_CATEGORY(PixelFormats)
> > +
> > /**
> > * \typedef PixelFormat
> > * \brief libcamera image pixel format
> > @@ -25,4 +34,119 @@ namespace libcamera {
> > * \todo Add support for format modifiers
> > */
> >
> > +struct PixelFormatEntry {
> > + uint32_t v4l2;
> > + uint32_t drm;
> > + std::set<uint32_t> modifiers;
> > +};
> > +
> > +static const std::vector<PixelFormatEntry> pixelFormats = {
> > + /* Invalid format, important to be first in list. */
> > + { .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} },
> > + /* RGB formats. */
> > + { .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_ARGB32, .drm = DRM_FORMAT_BGRA8888, .modifiers = {} },
> > + /* YUV packed formats. */
> > + { .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_VYUY, .drm = DRM_FORMAT_VYUY, .modifiers = {} },
> > + /* YUY planar formats. */
> > + { .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} },
> > + { .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} },
> > + /* Compressed formats. */
> > + { .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} },
> > +};
> > +
> > +PixelFormat::PixelFormat()
> > + : format_(&pixelFormats[0])
> > +{
> > +}
> > +
> > +PixelFormat::PixelFormat(const PixelFormat &other)
> > + : format_(other.format_)
> > +{
> > +}
> > +
> > +PixelFormat::PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers)
> > + : format_(fromDRM(drm_fourcc, drm_modifiers))
> > +{
> > +}
> > +
> > +PixelFormat::PixelFormat(uint32_t v4l2_fourcc)
> > + : format_(fromV4L2(v4l2_fourcc))
> > +{
> > +}
>
> This seems very fragile. The distinction between constructing a
> PixelFormat with a DRM 4cc or a V4L2 fourcc (which are both uint32_t)
> only depends on the presence of the second, right now empty,
> parameter.
>
> What I would do is instead make it mandatory to construct a
> PixelFormat with a valid DRM 4cc, and provide to pipeline handler a
> static method(s) to convert between drm/v4l2 whenever they consider it
> oppotune.
It is already mandatory to create it from a DRM fourcc.
I do not like the static methods that allow converting between drm/v4l2
as the conversion is not as simple fourcc to fourcc it's also modifiers.
By not having them we have one way to do things. The static methods of
the past IMHO have contributed to the mess we have today where one is
not sure what namespace the fourcc is in as it's converted back and
forth at different parts of the code.
>
> > +
> > +PixelFormat &PixelFormat::operator=(const PixelFormat &other)
> > +{
> > + format_ = other.format_;
> > +
> > + return *this;
> > +}
> > +
> > +bool PixelFormat::operator==(const PixelFormat &other) const
> > +{
> > + return format_ == other.format_;
> > +}
> > +
> > +bool PixelFormat::operator!=(const PixelFormat &other) const
> > +{
> > + return format_ != other.format_;
> > +}
> > +
> > +bool PixelFormat::operator<(const PixelFormat &other) const
> > +{
> > + return format_ > other.format_;
> > +}
> > +
> > +uint32_t PixelFormat::v4l2() const
> > +{
> > + return format_->v4l2;
> > +}
> > +
> > +uint32_t PixelFormat::fourcc() const
> > +{
> > + return format_->drm;
> > +}
> > +
> > +const std::set<uint32_t> &PixelFormat::modifiers() const
> > +{
> > + return format_->modifiers;
> > +}
> > +
> > +const PixelFormatEntry *PixelFormat::fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const
> > +{
> > + for (const PixelFormatEntry &entry : pixelFormats)
> > + if (entry.drm == drm_fourcc &&
> > + entry.modifiers == drm_modifiers)
> > + return &entry;
> > +
> > + LOG(PixelFormats, Error)
> > + << "Unsupported DRM pixel format "
> > + << utils::hex(drm_fourcc);
> > +
> > + return &pixelFormats[0];
> > +}
> > +
> > +const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const
> > +{
> > + for (const PixelFormatEntry &entry : pixelFormats)
> > + if (entry.v4l2 == v4l2_fourcc)
> > + return &entry;
> > +
> > + LOG(PixelFormats, Error)
> > + << "Unsupported V4L2 pixel format "
> > + << utils::hex(v4l2_fourcc);
> > +
> > + return &pixelFormats[0];
> > +}
> > +
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index 13789e9eb344f95c..dbce550ca8d0b7b1 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(PixelFormat pixelformat) const
> > * handlers provied StreamFormats.
> > */
> > StreamConfiguration::StreamConfiguration()
> > - : pixelFormat(0), stream_(nullptr)
> > + : pixelFormat(), stream_(nullptr)
> > {
> > }
> >
> > @@ -287,7 +287,7 @@ StreamConfiguration::StreamConfiguration()
> > * \brief Construct a configuration with stream formats
> > */
> > StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> > - : pixelFormat(0), stream_(nullptr), formats_(formats)
> > + : pixelFormat(), stream_(nullptr), formats_(formats)
> > {
> > }
> >
> > @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> > std::string StreamConfiguration::toString() const
> > {
> > std::stringstream ss;
> > - ss << size.toString() << "-" << utils::hex(pixelFormat);
> > + ss << size.toString() << "-" << utils::hex(pixelFormat.fourcc());
> > return ss.str();
> > }
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index f84bd00570afa38c..e9d3e60198e140a0 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -846,7 +846,7 @@ ImageFormats<PixelFormat> V4L2VideoDevice::formats()
> > if (sizes.empty())
> > return {};
> >
> > - if (formats.addFormat(pixelformat, sizes)) {
> > + if (formats.addFormat(PixelFormat(pixelformat), sizes)) {
> > LOG(V4L2, Error)
> > << "Could not add sizes for pixel format "
> > << pixelformat;
> > @@ -1417,56 +1417,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> > */
> > PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
> > {
> > - switch (v4l2Fourcc) {
> > - /* RGB formats. */
> > - case V4L2_PIX_FMT_RGB24:
> > - return DRM_FORMAT_BGR888;
> > - case V4L2_PIX_FMT_BGR24:
> > - return DRM_FORMAT_RGB888;
> > - case V4L2_PIX_FMT_ARGB32:
> > - return DRM_FORMAT_BGRA8888;
> > -
> > - /* YUV packed formats. */
> > - case V4L2_PIX_FMT_YUYV:
> > - return DRM_FORMAT_YUYV;
> > - case V4L2_PIX_FMT_YVYU:
> > - return DRM_FORMAT_YVYU;
> > - case V4L2_PIX_FMT_UYVY:
> > - return DRM_FORMAT_UYVY;
> > - case V4L2_PIX_FMT_VYUY:
> > - return DRM_FORMAT_VYUY;
> > -
> > - /* YUY planar formats. */
> > - case V4L2_PIX_FMT_NV16:
> > - case V4L2_PIX_FMT_NV16M:
> > - return DRM_FORMAT_NV16;
> > - case V4L2_PIX_FMT_NV61:
> > - case V4L2_PIX_FMT_NV61M:
> > - return DRM_FORMAT_NV61;
> > - case V4L2_PIX_FMT_NV12:
> > - case V4L2_PIX_FMT_NV12M:
> > - return DRM_FORMAT_NV12;
> > - case V4L2_PIX_FMT_NV21:
> > - case V4L2_PIX_FMT_NV21M:
> > - return DRM_FORMAT_NV21;
> > -
> > - /* Compressed formats. */
> > - case V4L2_PIX_FMT_MJPEG:
> > - return DRM_FORMAT_MJPEG;
> > -
> > - /* V4L2 formats not yet supported by DRM. */
> > - case V4L2_PIX_FMT_GREY:
> > - default:
> > - /*
> > - * \todo We can't use LOG() in a static method of a Loggable
> > - * class. Until we fix the logger, work around it.
> > - */
> > - libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),
> > - LogError).stream()
> > - << "Unsupported V4L2 pixel format "
> > - << utils::hex(v4l2Fourcc);
> > - return 0;
> > - }
> > + return PixelFormat(v4l2Fourcc);
> > }
>
> Shouldn't we aim to replace all usages of this (and the following)
> methods by centralizing the conversion in PixelFormat ? This just
> served as an utility function for that purpose. Same for the V4L2
> wrapper.
Please see patch 5/6 and 6/6.
>
> If I recall correctly, the single plane/multiplane capabilities of the
> V4L2 device should be taken into account to perform the conversion.
> Could this be an additional paramter to
> PixelFormat::toDRM4cc(uint32_t v4l24cc, bool molutplanar) ?
It could but we have no consumer of such information today and the goal
of this series is not to add it but to allow for format enumeration with
modifiers to be built on top. When we need multiplane information to
convert from V4L2 fourcc we can add it on top.
>
> >
> > /**
> > @@ -1500,53 +1451,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)
> > */
> > uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)
> > {
> > - switch (pixelFormat) {
> > - /* RGB formats. */
> > - case DRM_FORMAT_BGR888:
> > - return V4L2_PIX_FMT_RGB24;
> > - case DRM_FORMAT_RGB888:
> > - return V4L2_PIX_FMT_BGR24;
> > - case DRM_FORMAT_BGRA8888:
> > - return V4L2_PIX_FMT_ARGB32;
> > -
> > - /* YUV packed formats. */
> > - case DRM_FORMAT_YUYV:
> > - return V4L2_PIX_FMT_YUYV;
> > - case DRM_FORMAT_YVYU:
> > - return V4L2_PIX_FMT_YVYU;
> > - case DRM_FORMAT_UYVY:
> > - return V4L2_PIX_FMT_UYVY;
> > - case DRM_FORMAT_VYUY:
> > - return V4L2_PIX_FMT_VYUY;
> > -
> > - /*
> > - * YUY planar formats.
> > - * \todo Add support for non-contiguous memory planes
> > - * \todo Select the format variant not only based on \a multiplanar but
> > - * also take into account the formats supported by the device.
> > - */
> > - case DRM_FORMAT_NV16:
> > - return V4L2_PIX_FMT_NV16;
> > - case DRM_FORMAT_NV61:
> > - return V4L2_PIX_FMT_NV61;
> > - case DRM_FORMAT_NV12:
> > - return V4L2_PIX_FMT_NV12;
> > - case DRM_FORMAT_NV21:
> > - return V4L2_PIX_FMT_NV21;
> > -
> > - /* Compressed formats. */
> > - case DRM_FORMAT_MJPEG:
> > - return V4L2_PIX_FMT_MJPEG;
> > - }
> > -
> > - /*
> > - * \todo We can't use LOG() in a static method of a Loggable
> > - * class. Until we fix the logger, work around it.
> > - */
> > - libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), LogError).stream()
> > - << "Unsupported V4L2 pixel format "
> > - << utils::hex(pixelFormat);
> > - return 0;
> > + return pixelFormat.v4l2();
> > }
> >
> > /**
> > diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
> > index 368cb43fbf17ae4d..c071ea9b4022750c 100644
> > --- a/src/qcam/format_converter.cpp
> > +++ b/src/qcam/format_converter.cpp
> > @@ -30,7 +30,7 @@
> > int FormatConverter::configure(libcamera::PixelFormat format, unsigned int width,
> > unsigned int height)
> > {
> > - switch (format) {
> > + switch (format.fourcc()) {
> > case DRM_FORMAT_NV12:
> > formatFamily_ = NV;
> > horzSubSample_ = 2;
> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> > index 0ebb8edd49efd1b1..d86c97dab8374d5e 100644
> > --- a/src/qcam/viewfinder.cpp
> > +++ b/src/qcam/viewfinder.cpp
> > @@ -14,7 +14,7 @@
> > #include "viewfinder.h"
> >
> > ViewFinder::ViewFinder(QWidget *parent)
> > - : QWidget(parent), format_(0), width_(0), height_(0), image_(nullptr)
> > + : QWidget(parent), format_(), width_(0), height_(0), image_(nullptr)
> > {
> > }
> >
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 622520479be01f58..e9d7bfd8ee243b78 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -525,7 +525,6 @@ struct PixelFormatPlaneInfo {
> > };
> >
> > struct PixelFormatInfo {
> > - PixelFormat format;
> > uint32_t v4l2Format;
> > unsigned int numPlanes;
> > std::array<PixelFormatPlaneInfo, 3> planes;
> > @@ -533,24 +532,24 @@ struct PixelFormatInfo {
> >
> > namespace {
> >
> > -constexpr std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
> > +static const std::array<PixelFormatInfo, 13> pixelFormatInfo = { {
> > /* RGB formats. */
> > - { DRM_FORMAT_RGB888, V4L2_PIX_FMT_BGR24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_BGR888, V4L2_PIX_FMT_RGB24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_BGRA8888, V4L2_PIX_FMT_ARGB32, 1, {{ { 32, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_BGR24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_RGB24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_ARGB32, 1, {{ { 32, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > /* YUV packed formats. */
> > - { DRM_FORMAT_UYVY, V4L2_PIX_FMT_UYVY, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_VYUY, V4L2_PIX_FMT_VYUY, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_YUYV, V4L2_PIX_FMT_YUYV, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_YVYU, V4L2_PIX_FMT_YVYU, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_UYVY, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_VYUY, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_YUYV, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_YVYU, 1, {{ { 16, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> > /* YUY planar formats. */
> > - { DRM_FORMAT_NV12, V4L2_PIX_FMT_NV12, 2, {{ { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_NV21, V4L2_PIX_FMT_NV21, 2, {{ { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_NV16, V4L2_PIX_FMT_NV16, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_NV61, V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_NV24, V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > - { DRM_FORMAT_NV42, V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} },
> > -}};
> > + { V4L2_PIX_FMT_NV12, 2, {{ { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_NV21, 2, {{ { 8, 1, 1 }, { 16, 2, 2 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_NV16, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} },
> > + { V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} },
> > +} };
>
> Shouldn't we aim to centralize all of this in PixelFormat ?
I thought about it and then decided against it, the information recorded
here is specific to the v4l2 wrapper and can be indexed directly on V4L2
fourcc so I think we should leave it where it is.
>
> Thanks
> j
>
> >
> > } /* namespace */
> >
> > @@ -588,24 +587,10 @@ unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,
> >
> > PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)
> > {
> > - auto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),
> > - [format](const PixelFormatInfo &info) {
> > - return info.v4l2Format == format;
> > - });
> > - if (info == pixelFormatInfo.end())
> > - return format;
> > -
> > - return info->format;
> > + return PixelFormat(format);
> > }
> >
> > uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)
> > {
> > - auto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),
> > - [format](const PixelFormatInfo &info) {
> > - return info.format == format;
> > - });
> > - if (info == pixelFormatInfo.end())
> > - return format;
> > -
> > - return info->v4l2Format;
> > + return format.v4l2();
> > }
> > diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp
> > index a391f5cd087d3872..a904d681c1691889 100644
> > --- a/test/stream/stream_formats.cpp
> > +++ b/test/stream/stream_formats.cpp
> > @@ -7,6 +7,8 @@
> >
> > #include <iostream>
> >
> > +#include <linux/drm_fourcc.h>
> > +
> > #include <libcamera/geometry.h>
> > #include <libcamera/stream.h>
> >
> > @@ -53,42 +55,49 @@ protected:
> >
> > int run()
> > {
> > + std::vector<PixelFormat> pixelFormats = {
> > + PixelFormat(DRM_FORMAT_YUYV, {}),
> > + PixelFormat(DRM_FORMAT_YVYU, {}),
> > + PixelFormat(DRM_FORMAT_UYVY, {}),
> > + PixelFormat(DRM_FORMAT_VYUY, {}),
> > + };
> > +
> > /* Test discrete sizes */
> > StreamFormats discrete({
> > - { 1, { SizeRange(100, 100), SizeRange(200, 200) } },
> > - { 2, { SizeRange(300, 300), SizeRange(400, 400) } },
> > + { pixelFormats[0], { SizeRange(100, 100), SizeRange(200, 200) } },
> > + { pixelFormats[1], { SizeRange(300, 300), SizeRange(400, 400) } },
> > });
> >
> > - if (testSizes("discrete 1", discrete.sizes(1),
> > + if (testSizes("discrete 1", discrete.sizes(pixelFormats[0]),
> > { Size(100, 100), Size(200, 200) }))
> > return TestFail;
> > - if (testSizes("discrete 2", discrete.sizes(2),
> > + if (testSizes("discrete 2", discrete.sizes(pixelFormats[1]),
> > { Size(300, 300), Size(400, 400) }))
> > return TestFail;
> >
> > /* Test range sizes */
> > StreamFormats range({
> > - { 1, { SizeRange(640, 480, 640, 480) } },
> > - { 2, { SizeRange(640, 480, 800, 600, 8, 8) } },
> > - { 3, { SizeRange(640, 480, 800, 600, 16, 16) } },
> > - { 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },
> > + { pixelFormats[0], { SizeRange(640, 480, 640, 480) } },
> > + { pixelFormats[1], { SizeRange(640, 480, 800, 600, 8, 8) } },
> > + { pixelFormats[2], { SizeRange(640, 480, 800, 600, 16, 16) } },
> > + { pixelFormats[3], { SizeRange(128, 128, 4096, 4096, 128, 128) } },
> > });
> >
> > - if (testSizes("range 1", range.sizes(1), { Size(640, 480) }))
> > + if (testSizes("range 1", range.sizes(pixelFormats[0]), { Size(640, 480) }))
> > return TestFail;
> >
> > - if (testSizes("range 2", range.sizes(2), {
> > + if (testSizes("range 2", range.sizes(pixelFormats[1]), {
> > Size(640, 480), Size(720, 480),
> > Size(720, 576), Size(768, 480),
> > Size(800, 600) }))
> > return TestFail;
> >
> > - if (testSizes("range 3", range.sizes(3), {
> > + if (testSizes("range 3", range.sizes(pixelFormats[2]), {
> > Size(640, 480), Size(720, 480),
> > Size(720, 576), Size(768, 480) }))
> > return TestFail;
> >
> > - if (testSizes("range 4", range.sizes(4), {
> > + if (testSizes("range 4", range.sizes(pixelFormats[3]), {
> > Size(1024, 768), Size(1280, 1024),
> > Size(2048, 1152), Size(2048, 1536),
> > Size(2560, 2048), Size(3200, 2048), }))
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list