[libcamera-devel] [RFC 4/6] libcamera: PixelFormat: Add operations to operate on names
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Feb 29 11:57:18 CET 2020
Hi Niklas,
Thank you for the patch.
On Fri, Feb 28, 2020 at 04:29:11AM +0100, Niklas Söderlund wrote:
> Add a name to each pixel format and extend PixelFormat to be constructed
> from a name and to retrieve the name for printing. Make use of the new
> functionality to demonstrate it.
>
> - Update the cam utility to read and print the pixel format name instead
> of the fourcc integer number.
>
> - Update log messages to print the name instead of the fourcc integer
> number.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/pixelformats.h | 3 ++
> src/cam/main.cpp | 9 ++---
> src/libcamera/pipeline/uvcvideo.cpp | 4 +-
> src/libcamera/pixelformats.cpp | 58 ++++++++++++++++++++---------
> src/libcamera/stream.cpp | 2 +-
> 5 files changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> index f0951e983192d5e8..8cea3a90ef2cc7ae 100644
> --- a/include/libcamera/pixelformats.h
> +++ b/include/libcamera/pixelformats.h
> @@ -24,6 +24,7 @@ public:
> PixelFormat();
> PixelFormat(const PixelFormat &other);
> explicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);
> + explicit PixelFormat(const std::string &name);
Shouldn't this take modifiers too ?
> PixelFormat &operator=(const PixelFormat &other);
>
> @@ -31,6 +32,7 @@ public:
> bool operator!=(const PixelFormat &other) const;
> bool operator<(const PixelFormat &other) const;
>
> + const std::string &name() const;
> uint32_t v4l2() const;
> uint32_t fourcc() const;
> const std::set<uint32_t> &modifiers() const;
> @@ -47,6 +49,7 @@ protected:
> 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 *fromName(const std::string &name) const;
>
> const PixelFormatEntry *format_;
> };
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index c8ef79daea37d8b6..0a08e362294fc9ee 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -159,7 +159,7 @@ int CamApp::parseOptions(int argc, char *argv[])
> ArgumentRequired);
> streamKeyValue.addOption("height", OptionInteger, "Height in pixels",
> ArgumentRequired);
> - streamKeyValue.addOption("pixelformat", OptionInteger, "Pixel format",
> + streamKeyValue.addOption("pixelformat", OptionString, "Pixel format",
> ArgumentRequired);
>
> OptionsParser parser;
> @@ -247,9 +247,9 @@ int CamApp::prepareConfig()
> if (opt.isSet("height"))
> cfg.size.height = opt["height"];
>
> - /* TODO: Translate 4CC string to ID. */
> if (opt.isSet("pixelformat"))
> - cfg.pixelFormat = PixelFormat(opt["pixelformat"], {});
> + cfg.pixelFormat =
> + PixelFormat(opt["pixelformat"].toString());
> }
> }
>
> @@ -282,8 +282,7 @@ int CamApp::infoConfiguration()
>
> const StreamFormats &formats = cfg.formats();
> for (PixelFormat pixelformat : formats.pixelformats()) {
> - std::cout << " * Pixelformat: 0x" << std::hex
> - << std::setw(8) << pixelformat.fourcc() << " "
> + std::cout << " * Pixelformat: " << pixelformat.name() << " "
> << formats.range(pixelformat).toString()
> << std::endl;
Would it be useful to print both the name and the hex fourcc value ?
When debugging we often see the numerical value, being able to compare
it with printed messages could be interesting. We could add a toString()
to PixelFormat to combine the two:
std::string PixeFormat::toString()
{
std::stringstream ss;
ss << name() << " (" << utils::hex(fourcc(), 8) << ")";
return ss.str();
}
This could be extended to also take modifiers into account.
We don't necessarily need to print both everywhere, but in the cam tool
it may be useful.
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5f3e52f691aaeae4..75fbfe1eb9145424 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.fourcc()
> - << " to " << cfg.pixelFormat.fourcc();
> + << "Adjusting pixel format from " << pixelFormat.name()
> + << " to " << cfg.pixelFormat.name();
> status = Adjusted;
> }
>
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index b2aacbc39b9ca16a..70f41d86a23baceb 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -35,6 +35,7 @@ LOG_DEFINE_CATEGORY(PixelFormats)
> */
>
> struct PixelFormatEntry {
> + std::string name;
Would const char * be a bit more efficient ?
> uint32_t v4l2;
> uint32_t drm;
> std::set<uint32_t> modifiers;
> @@ -42,27 +43,27 @@ struct PixelFormatEntry {
>
> static const std::vector<PixelFormatEntry> pixelFormats = {
> /* Invalid format, important to be first in list. */
> - { .v4l2 = 0, .drm = DRM_FORMAT_INVALID, .modifiers = {} },
> + { .name = "INVALID", .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 = {} },
> + { .name = "RGR888", .v4l2 = V4L2_PIX_FMT_RGB24, .drm = DRM_FORMAT_BGR888, .modifiers = {} },
> + { .name = "RGB888", .v4l2 = V4L2_PIX_FMT_BGR24, .drm = DRM_FORMAT_RGB888, .modifiers = {} },
> + { .name = "BGRA8888", .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 = {} },
> + { .name = "YUYV", .v4l2 = V4L2_PIX_FMT_YUYV, .drm = DRM_FORMAT_YUYV, .modifiers = {} },
> + { .name = "YVYU", .v4l2 = V4L2_PIX_FMT_YVYU, .drm = DRM_FORMAT_YVYU, .modifiers = {} },
> + { .name = "UYVY", .v4l2 = V4L2_PIX_FMT_UYVY, .drm = DRM_FORMAT_UYVY, .modifiers = {} },
> + { .name = "VYUY", .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 = {} },
> + { .name = "NV16", .v4l2 = V4L2_PIX_FMT_NV16, .drm = DRM_FORMAT_NV16, .modifiers = {} },
> + { .name = "NV16M", .v4l2 = V4L2_PIX_FMT_NV16M, .drm = DRM_FORMAT_NV16, .modifiers = {} },
> + { .name = "NV61", .v4l2 = V4L2_PIX_FMT_NV61, .drm = DRM_FORMAT_NV61, .modifiers = {} },
> + { .name = "NV61M", .v4l2 = V4L2_PIX_FMT_NV61M, .drm = DRM_FORMAT_NV61, .modifiers = {} },
> + { .name = "NV12", .v4l2 = V4L2_PIX_FMT_NV12, .drm = DRM_FORMAT_NV12, .modifiers = {} },
> + { .name = "NV12M", .v4l2 = V4L2_PIX_FMT_NV12M, .drm = DRM_FORMAT_NV12, .modifiers = {} },
> + { .name = "NV21", .v4l2 = V4L2_PIX_FMT_NV21, .drm = DRM_FORMAT_NV21, .modifiers = {} },
> + { .name = "NV21M", .v4l2 = V4L2_PIX_FMT_NV21M, .drm = DRM_FORMAT_NV21, .modifiers = {} },
> /* Compressed formats. */
> - { .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} },
> + { .name = "MJPEG", .v4l2 = V4L2_PIX_FMT_MJPEG, .drm = DRM_FORMAT_MJPEG, .modifiers = {} },
> };
>
> PixelFormat::PixelFormat()
> @@ -85,6 +86,11 @@ PixelFormat::PixelFormat(uint32_t v4l2_fourcc)
> {
> }
>
> +PixelFormat::PixelFormat(const std::string &name)
> + : format_(fromName(name))
> +{
> +}
> +
> PixelFormat &PixelFormat::operator=(const PixelFormat &other)
> {
> format_ = other.format_;
> @@ -107,6 +113,11 @@ bool PixelFormat::operator<(const PixelFormat &other) const
> return format_ > other.format_;
> }
>
> +const std::string &PixelFormat::name() const
> +{
> + return format_->name;
> +}
> +
> uint32_t PixelFormat::v4l2() const
> {
> return format_->v4l2;
> @@ -149,4 +160,17 @@ const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const
> return &pixelFormats[0];
> }
>
> +const PixelFormatEntry *PixelFormat::fromName(const std::string &name) const
> +{
> + for (const PixelFormatEntry &entry : pixelFormats)
for (const PixelFormatEntry &entry : pixelFormats) {
> + if (entry.name == name)
> + return &entry;
}
> +
> + LOG(PixelFormats, Error)
> + << "Unsupported name for pixel format "
> + << name;
> +
> + return &pixelFormats[0];
I think nullptr would be better, and I'm not sure I would log a message
here. I'll review 3/6, we can discuss the issue there.
> +}
> +
> } /* namespace libcamera */
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index dbce550ca8d0b7b1..e82ffd8f3b211925 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> std::string StreamConfiguration::toString() const
> {
> std::stringstream ss;
> - ss << size.toString() << "-" << utils::hex(pixelFormat.fourcc());
> + ss << size.toString() << "-" << pixelFormat.name();
> return ss.str();
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list