[libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Feb 29 13:57:48 CET 2020
Hi Niklas,
Thank you for the patch.
On Fri, Feb 28, 2020 at 12:36:17PM +0100, Niklas Söderlund wrote:
> On 2020-02-28 11:27:37 +0100, Jacopo Mondi wrote:
> > 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
"is that it disallows creation of PixelFormat instances from a V4L2
fourcc for classes other than the explicitly designed ones
(V4L2VideoDevice and V4L2CameraProxy). This limits the risk ..."
> > > 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
s/quiet/quite/
s/have/has/
> > > different ares of the code to simultaneously switch to the new class
> > > based PixelFormat implementation.
s/class based/class-based/
> >
> > 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;
> > >
I'm afraid this is a showstopper. I think it's bad enough to expose V4L2
below in the public libcamera API, but exposing an internal class of the
V4L2 compatibility layer, which isn't part of the libcamera.so, is even
worse.
> > > 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);
No need for explicit, a constructor with two arguments is always
explicit. However, you may want to set a default value for the modifiers
parameter (= {}), in which case the explicit keyword would make sense.
> > > +
> > > + PixelFormat &operator=(const PixelFormat &other);
> > > +
> > > + bool operator==(const PixelFormat &other) const;
> > > + bool operator!=(const PixelFormat &other) const;
You can define this inline as
{
return !(*this == other);
}
> > > + bool operator<(const PixelFormat &other) const;
A recommended practice is to define those operators as non-member
function:
bool operator==(const PixelFormat &lhs, const PixelFormat &rhs) const;
bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs) const;
bool operator<(const PixelFormat &lhs, const PixelFormat &rhs) const;
The reason is that, otherwise, implicit conversions would work
differently on the left- and right-hand side (although in this case all
constructors are explicit, but I think it's still a good idea to follow
best practice rules).
Compare those two usages of operator== (assuming the constructor that
takes a DRM 4CC loses its explicit keyword, and gets a default value for
the last parameter).
PixelFormat fmt = ...;
if (fmt == DRM_FORMAT_NV12)
...
if (DRM_FORMAT_NV12 == fmt)
...
With operator== defined as a member function, the first comparison would
compile fine with PixelFormat(DRM_FORMAT_NV12) being called implicitly,
while the second would fail to compile. With non-member comparison, both
will compile.
> > > +
> > > + 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);
This should be moved somewhere else, V4L2 should not appear in this
header at all. I'd even rename drm_fourcc to fourcc for this reason (as
well as drm_modifiers to modifiers), and fromDRM to fromFourcc (or
from4CC) for the same reason.
> > > +
> > > +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"], {});
Yes, a default value for the second parameter is useful. I know it will
not allow having a V4L2 constructor taking an unsigned int, but that
should be removed anyway.
> > > }
> > > }
> > >
> > > @@ -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>;
Looks like we should possibly split the class in two. I think
refactoring is needed in any case, this doesn't feel right.
> > >
> > > } /* 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, {});
Should this constructor (with a default second parameter) really be
explicit ?
cfg.pixelFormat = DRM_FORMAT_NV12;
doesn't seem bad.
> > >
> > > 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.
I'm sorry, but I don't see why we need to move V4L2 4CC handling to the
PixelFormat class. Keeping it in internal V4L2 helpers is better, it
should really not be part of the public API.
> > ?
> >
> > > 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.
We could create a V4L2PixelFormat struct to fix this
struct V4L2PixelFormat
{
uint32_t fourcc;
};
(with a constructor taking an uint32_t if needed).
You could extend that with a constructor that takes a PixelFormat, and
add a toPixelFormat() method. I think this would be better than trying
to use PixelFormat for both. It will also make the PixelFormat class
lighter, removing the lookup from the pixelFormats array every time a
PixelFormat is constructed. The array will still be needed to get names,
but it shouldn't contain V4L2 4CCs, and shouldn't contain modifiers
either.
> > > +
> > > +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_;
Should this be < ?
> > > +}
> > > +
> > > +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];
I think we should drop validation, and store the fourcc and modifiers
directly in PixelFormat. The class should really be a lightweight
wrapper.
This being said, I may be convinced that validation at construction time
is a good thing, but not for the purpose of supporting V4L2. I also
think the current API would be awkward for application, which
application would really write
PixelFormat format(DRM_FORMAT_NV12);
if (format.fourcc() == 0) {
/* Handle error. */
}
?
Making PixelFormat just a wrapper around fourcc + modifiers seems better
to me. The name() function will need to be adjusted to perform the
lookup on demand (possibly caching it ?), and returning a name
constructed from the fourcc if no name is found in the database.
> > > +}
> > > +
> > > +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.
>
> > >
> > > } /* 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), }))
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list